-
Notifications
You must be signed in to change notification settings - Fork 504
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
Refactor deduplicate()
arguments
#548
Conversation
8f01c53
to
a8f8513
Compare
adcae93
to
35e4354
Compare
593ac80
to
3c27a18
Compare
I'm not sure why the Redshift test is failing with
given that |
3c27a18
to
e441901
Compare
Ah, didn't realize Redshift defaults to Postgres rather than default. |
7b1e0c2
to
a60dac6
Compare
deduplicate
macrodeduplicate()
arguments
c4c5430
to
0641bfe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@judahrand love the direction this is heading! Thank you for your detailed coverage of a wide range of databases, especially as it relates to using natural joins.
The updates are definitely more readable.
I left a few comments regarding the documentation -- appeared that the renaming to partition_by
was overlooked in the usage examples.
I'm wondering if we can do any fancy footwork in the background so that the changes aren't breaking (but still make all the updates we want). I think I've seen some examples in past merge requests, so I'm going to try looking for those, and I'll follow-up in this conversation if/when I find them.
@dbeatty10 I'll deal with these comments now 😄 I also wasn't sure if you thought this would have to wait for a minor release due to breaking changes even though the macro is very very new? But I suspect a breaking change is a breaking change. Maybe it would be worth in future marking things as 'alpha' for their first few releases to hash out this kind of feedback in a way which is easier to fix?
Oh, I shall look forward to seeing the fancy footwork! |
@dbeatty10 Any luck here? |
8f806b9
to
bae387a
Compare
Hey @dbeatty10, I think that I've managed to overcome both of these barriers thanks to your pointers over Slack. Are you able to re-review this? Once this is merged it would also be great to try to get #550 and #551 in too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @judahrand ! Thanks for your documentation of how to handle the deprecated signature and for adding an integration test for that case.
I left two comments on very minor things to update within the documentation.
Looks good to me to merge after that.
@dbeatty10 Sorted! |
This is a:
main
dev/
branchdev/
branchDescription & motivation
This Pull Request aims to improve the
deduplicate
macro based on community feedback from its first release. It is stacked on top of #549 and introduces breaking changes.relation_alias
to reference CTEs is confusing. This prompted me to investigate whether this is really needed - it was only needed due to the use ofdbt_utils.star
which can be avoided by using anatural join
which is supported by most SQL syntaxes.Checklist
star()
source)limit_zero()
macro in place of the literal string:limit 0
dbt_utils.type_*
macros instead of explicit datatypes (e.g.dbt_utils.type_timestamp()
instead ofTIMESTAMP
Resolves #542. Resolves #543.