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.

Sign up for GitHub

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

Jump to bottom

Add where parameter to union_relations #554

Merged
merged 3 commits into from
May 12, 2022
Merged

Add where parameter to union_relations #554

merged 3 commits into from
May 12, 2022

Conversation

LewisDavies
Copy link
Contributor

@LewisDavies LewisDavies commented Apr 19, 2022 •
edited by dbeatty10
Loading

Resolves #464, resolves #382

This is a:

  • bug fix PR with no breaking changes — please ensure the base branch is main
  • new functionality — please ensure the base branch is the latest dev/ branch
  • a breaking change — please ensure the base branch is the latest dev/ branch

Description & motivation

I often want to add basic filters when using union_relations, such as excluding obsolete records or low-quality data. Currently, I have to create two models to do so: one to union the data then another to filter it. This PR adds a where parameter to union_relations so models are filtered before being unioned.

More context & discussion can be found in #464 and #382.

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@LewisDavies LewisDavies changed the base branch from next/minor to main April 19, 2022 08:45
dbeatty10
dbeatty10 requested changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this and for including both integration tests plus documentation 🏆

Thank you also for alerting about the branch names within the merge request template being out-of-date; those are updated now.

Review

Only feedback:

  • I'd like to move the where parameter to be at the end of the argument list so we can guarantee no accidental breakage for any consumers.

Is there any reason why it would be beneficial or necessary to be the first keyword argument?

Question:

  • It looks like the same filter specified by the where parameter will be applied to every relation; I'm assuming you haven't needed unique filters per relation as often?

Aside

At first, I was wondering if this would conflict or overlap with the where config, but it looks like it is only for test resources.

@LewisDavies LewisDavies changed the base branch from main to next/minor April 27, 2022 11:13
@LewisDavies
Copy link
Contributor Author

LewisDavies commented Apr 27, 2022 •
edited
Loading

@dbeatty10 Thanks for the feedback! I've made the changes and tests are passing.

I rebased locally onto next/minor, pushed my changes, and changed the target branch. Should I go ahead and update CHANGELOG.md too?

@dbeatty10
Copy link
Contributor

@dbeatty10 Thanks for the feedback! I've made the changes and tests are passing.

I rebased locally onto next/minor, pushed my changes, and changed the target branch. Should I go ahead and update CHANGELOG.md too?

Sorry for the dramatic delay in conversation @LewisDavies

Could I bother you for two more changes and then we can merge this in?

  1. Rebase to main -- we just switched to all pull requests basing off main for simplicity sake
  2. Yes, go ahead and update CHANGELOG.md within the "Unreleased" section.

If you don't have the time, just let me know and I can do those two steps too.

Thank you so much for your detailed solution on this!

@LewisDavies LewisDavies changed the base branch from next/minor to main May 12, 2022 16:08
@LewisDavies
Copy link
Contributor Author

@dbeatty10 No worries, the changes are all done. Since I rebased off main I updated the target branch too.

With all my testing and rebasing the commits were an absolute mess, so I cleaned them up and force pushed again. Should be good to go now!

dbeatty10
dbeatty10 approved these changes May 12, 2022
View reviewed changes
Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

Looks awesome, @LewisDavies ! This rocks 🎸

@dbeatty10 dbeatty10 merged commit 7e3da7c into dbt-labs:main May 12, 2022
@plittlewood-rpt plittlewood-rpt mentioned this pull request May 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbeatty10 dbeatty10 dbeatty10 approved these changes

Assignees
No one assigned
Labels
pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add WHERE filter inside the dbt_utils.union macro Add optional where condition in dbt-utils.union macro
2 participants
@LewisDavies @dbeatty10