Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reports_order for RT compliance to intended checks #2844

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

owades
Copy link
Member

@owades owades commented Jul 26, 2023

Description

One change I forgot to make before: adding reports_order for RT checks to intended checks. This is very straightforward.

Also fixed a typo in a check name.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

How has this been tested?

I spot checked to confirm that there's only one of each number added (from 0 to 15). Do you think I ought to rerun the whole model to test this?

Post-merge follow-ups

Document any actions that must be taken post-merge to deploy or otherwise implement the changes in this PR (for example, running a full refresh of some incremental model in dbt). If these actions will take more than a few hours after the merge or if they will be completed by someone other than the PR author, please create a dedicated follow-up issue and link it here to track resolution.

  • No action required
  • Actions required (specified below)

@github-actions
Copy link

Warehouse report 📦

DAG

Legend (in order of precedence)

Resource type Indicator Resolution
Large table-materialized model Orange Make the model incremental
Large model without partitioning or clustering Orange Add partitioning and/or clustering
View with more than one child Yellow Materialize as a table or incremental
Incremental Light green
Table Green
View White

@owades owades marked this pull request as ready for review July 28, 2023 00:28
@owades
Copy link
Member Author

owades commented Jul 28, 2023

@lauriemerrell I'd love a quick review on this!

Copy link
Contributor

@lauriemerrell lauriemerrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, re: testing you could add a few new tests here:

  • not_null with a where clause and the feature is any of the features included in the reports site
  • unique_combination_of_columns for feature / reports_order where reports_order is not null

And combined those tests would I think guarantee what you want.

@owades
Copy link
Member Author

owades commented Jul 31, 2023

Thanks @lauriemerrell, I added the simpler of the two checks you suggested for now (the other one required some workarounds as it would require using a custom macro in a place where dbt doesn't like macros. i'm happy with just this for now).

@owades owades merged commit 83a6872 into main Jul 31, 2023
5 of 6 checks passed
@owades owades deleted the reports-add-rt-check-order branch July 31, 2023 19:26
atvaccaro pushed a commit that referenced this pull request Aug 1, 2023
* reports_order for RT compliance to intended checks

* add one check for reports order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants