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

Replace references to tables to relations. Add include param to union #178

Merged
merged 6 commits into from
Dec 11, 2019

Conversation

clrcrl
Copy link
Contributor

@clrcrl clrcrl commented Dec 2, 2019

Fixes #90

Started off by adding an include parameter to union. But then when I was updating the docs I decided that having this macro named union_tables is silly so did some work to tidy that up. And then I did the same for get_tables_by_prefix and unpivot. Pretty happy with this pattern!

(This is probably the way we should have done this when we upgraded to 0.14.0, but ah well! This provides us a path to deprecate some macros though!)

I also tidied up some docs along the way.

Deprecating the get_tables_by_prefix and union_tables macros:

Here's what happens when you use the old macros:

$ dbt run -m test_get_tables_by_prefix_and_union
Running with dbt=0.15.0
Found 35 models, 53 tests, 0 snapshots, 0 analyses, 260 macros, 0 operations, 43 seed files, 0 sources

14:22:57 | Concurrency: 1 threads (target='redshift')
14:22:57 |
14:22:57 | 1 of 1 START table model integration_tests_dev.test_get_tables_by_prefix_and_union [RUN]
Warning: the `get_tables_by_prefix` macro is no longer supported and will be deprecated in a future release of dbt-utils. Use the `get_relations_by_prefix` macro instead
Warning: the `union_tables` macro is no longer supported and will be deprecated in a future release of dbt-utils. Use the `union_relations` macro instead
14:22:58 | 1 of 1 OK created table model integration_tests_dev.test_get_tables_by_prefix_and_union [SELECT in 1.20s]
14:22:59 |
14:22:59 | Finished running 1 table model in 2.81s.

Completed successfully

Done. PASS=1 WARN=0 ERROR=0 SKIP=0 TOTAL=1

These warnings don't show up when you use the relation versions of these macros:

Warning: the union_tables macro is no longer supported and will be deprecated in a future release of dbt-utils. Use the union_relations macro instead

Replacing the table arg with relation in unpivot:

This one was a little tricker to implement, but it works! LMK if you want me to go through this.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Big fan of these changes! I like the pattern you've laid out here for refactoring + renaming macros. I read through the diffs, though I haven't tested locally. The unpivot conditional logic to accept table as a keyword argument is clever/thorny, but it makes sense to me as code that we'll pull (along with the warning) in a future minor release.

macros/sql/union.sql Outdated Show resolved Hide resolved
@clrcrl clrcrl mentioned this pull request Dec 3, 2019
@clrcrl clrcrl force-pushed the refactor/tables-to-relations branch from 97d9f21 to e3a7de8 Compare December 6, 2019 20:12
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Just a couple of comments around the updated (really good) documentation here. I really like the approach you used to support either relation or table for some of these macros. FYI, we'll have the ability to raise proper warnings in dbt v0.15.1. Those escalate to errors if dbt is run with --warn-error. dbt-labs/dbt-core#1977

I didn't test any of these changed macros out, but I can do that if it would be helpful. All of the code here looks very good to me! Nice work on this one :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
macros/sql/union.sql Outdated Show resolved Hide resolved
@clrcrl
Copy link
Contributor Author

clrcrl commented Dec 11, 2019

Yeah I saw that PR from Jake -- my mind was 🤯at how few lines of code were involved to implement that! I'll set a reminder to myself to change this later :)

@clrcrl clrcrl merged commit 18ab2fb into master Dec 11, 2019
@clrcrl clrcrl deleted the refactor/tables-to-relations branch December 11, 2019 12:06
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.

None yet

3 participants