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

dispatch-ify ALL THE MACROS! #312

Merged
merged 30 commits into from
Jan 10, 2021

Conversation

dataders
Copy link
Contributor

@dataders dataders commented Dec 23, 2020

This is a:

  • bug fix PR with no breaking changes — please ensure the base branch is master
  • 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

see #311
cc: @alittlesliceoftom

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

@dataders dataders requested a review from clrcrl as a code owner December 23, 2020 09:58
@dataders
Copy link
Contributor Author

@jtcohen6 not sure what I've done here...

Encountered an error:
Compilation Error in test dbt_utils_equality_test_generate_series_ref_data_generate_series_ (models/sql/schema.yml)
  macro 'dbt_macro__test_equality' takes no keyword argument 'arg'
  
  > in macro test_equality (macros/schema_tests/equality.sql)
  > called by test dbt_utils_equality_test_generate_series_ref_data_generate_series_ (models/sql/schema.yml)

@dataders dataders marked this pull request as draft December 23, 2020 10:09
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.

Nice PR! I think this is the way to go.

You need to allow for passing kwargs through the macro dispatcher. It's a bit tricky.

macros/schema_tests/at_least_one.sql Outdated Show resolved Hide resolved
@dataders dataders changed the title dispatch-ify schema tests dispatch-ify ALL THE MACROS! Jan 4, 2021
@clrcrl clrcrl removed their request for review January 4, 2021 19:14
@clrcrl
Copy link
Contributor

clrcrl commented Jan 4, 2021

Removing myself as reviewer, as Jeremy has it covered :)

@dataders
Copy link
Contributor Author

dataders commented Jan 4, 2021

onto the next thing... I don't have a postgres target handy (but can get one running without too much trouble probably). @jtcohen6 any shoves in the right direction here? I haven't the slightest how the dispatch addition would make a test fail...

Failure in test dbt_utils_expression_is_true_data_test_expression_is_true_col_b_0_5__col_a_0_5 (models/schema_tests/schema.yml)
  Got 2 results, expected 0.

  compiled SQL at target/compiled/dbt_utils_integration_tests/models/schema_tests/schema.yml/schema_test/dbt_utils_expression_is_true_data_test_expression_is_true_col_b_0_5__col_a_0_5.sql

macros/sql/unpivot.sql Outdated Show resolved Hide resolved
@dataders dataders requested a review from jtcohen6 January 5, 2021 22:20
@dataders dataders marked this pull request as ready for review January 5, 2021 22:28
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.

Thanks for all the hard work here @swanderz @chaerinlee1! Truly a public service on behalf of all adapter plugin users

macros/schema_tests/at_least_one.sql Show resolved Hide resolved
@jtcohen6 jtcohen6 mentioned this pull request Jan 6, 2021
11 tasks
@clrcrl
Copy link
Contributor

clrcrl commented Jan 6, 2021

If my memory serves me correctly, we don't want to pass through the defaults twice. e.g. instead of:

{% macro test_expression_is_true(model, condition='true') %}
This conversation was marked as resolved by swanderz
  {{ return(adapter.dispatch('test_expression_is_true', packages = dbt_utils._get_utils_namespaces())(model, condition, **kwargs)) }}
{% endmacro %}

{% macro default__test_expression_is_true(model, condition='true') %}
...

We only want:

...
{% macro default__test_expression_is_true(model, condition) %}
...

See #279

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jan 6, 2021

@clrcrl It really could be an issue! I was trying to remember earlier today.

The problem in #279 is that we passed the default args into a macro call—not explicitly an issue with repeating them across two macro signatures.

@clrcrl
Copy link
Contributor

clrcrl commented Jan 6, 2021

oh right, yes I see the difference now!

@dataders
Copy link
Contributor Author

dataders commented Jan 8, 2021

let this comment stand in for what should have been marking my draft PR as ready for review!

@clrcrl
Copy link
Contributor

clrcrl commented Jan 8, 2021

@jtcohen6 — I'm going to let you do the honors here (unless you'd prefer me to!)

0.6.4 seems imminent!

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.

Let's ship it!!

@swanderz Could you add a changelog entry (I think "under the hood"), and add yourself + @chaerinlee1 as contributors?

@@ -7,6 +7,7 @@
- Speed up CI via threads, workflows ([#315](https://github.com/fishtown-analytics/dbt-utils/pull/315), [#316](https://github.com/fishtown-analytics/dbt-utils/pull/316))
- Fix `equality` test when used with ephemeral models + explicit column set ([#321](https://github.com/fishtown-analytics/dbt-utils/pull/321))
- Fix `get_query_results_as_dict` integration test with consistent ordering ([#322](https://github.com/fishtown-analytics/dbt-utils/pull/322))
- All macros are now properly dispatched, making it possible for non-core adapters to implement a shim package for dbt-utils ([#312](https://github.com/fishtown-analytics/dbt-utils/pull/312)) Thanks [@chaerinlee1](https://github.com/chaerinlee1) and [@swanderz](https://github.com/swanderz)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

comme ça?

Copy link
Contributor

Choose a reason for hiding this comment

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

merci !

@jtcohen6 jtcohen6 merged commit c64b520 into dbt-labs:master Jan 10, 2021
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.

4 participants