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

Test parsing context: package + macro deps + dispatch, oh my #3362

Closed
jtcohen6 opened this issue May 17, 2021 · 1 comment · Fixed by #3363
Closed

Test parsing context: package + macro deps + dispatch, oh my #3362

jtcohen6 opened this issue May 17, 2021 · 1 comment · Fixed by #3363
Labels
bug Something isn't working
Milestone

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented May 17, 2021

Follow-on from #3345 (I know, I hoped we were done with this one too!)

For better and for worse (definitely better!), dbt-utils does a lot of things with macro nesting and dispatching within custom schema test definitions. So, it's pretty good surface area for testing this functionality. This is on me for not testing #3345 against dbt-utils before approving #3345.

The problem seems to be when a package macro's dispatched implementation calls, in turn, another dispatched package macro. The second macro isn't included in the test parsing context. Currently, we're seeing this issue with dbt_utils.test_recency, the default implementation of which in turn calls the dbt_utils.current_timestamp macro:

Encountered an error:
Compilation Error in test dbt_utils_recency_test_recency_day__today__1 (models/schema_tests/schema.yml)
  'dict object' has no attribute 'current_timestamp'
  > in macro test_recency (macros/schema_tests/recency.sql)
  > called by macro default__test_recency (macros/schema_tests/recency.sql)
  > called by test dbt_utils_recency_test_recency_day__today__1 (models/schema_tests/schema.yml)

I've opened a draft PR (#3363) that adds a reproduction case to 008_schema_tests_test. I'm hopeful that this is as simple as altering the logic we've already put in place to recurse through depends_on.macros and add to the test parsing context.

Once we have a fix for that, we'll also need to cherry-pick it (and the fix from #3345) onto 0.19.latest for inclusion in v0.19.2-rc2.

@jtcohen6 jtcohen6 added the bug Something isn't working label May 17, 2021
@jtcohen6 jtcohen6 added this to the 0.19.2 milestone May 17, 2021
jtcohen6 added a commit that referenced this issue May 17, 2021
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented May 17, 2021

After discussing with @gshank, we've realized:

  • adapter.dispatch() needs to be statically analyzable in order to establish macro dependencies (in depends_on.macros)
  • dispatch() is written in python (good!), but it's possible to pass it non-static arguments (not as good!) This is a common pattern established by dbt-utils. The value of packages is statically knowable at parse time,
{% macro _get_utils_namespaces() %}
  {% set override_namespaces = var('dbt_utils_dispatch_list', []) %}
  {% do return(override_namespaces + ['dbt_utils']) %}
{% endmacro %}

{%- macro type_string() -%} 
  {{ return(adapter.dispatch('type_string', packages = dbt_utils._get_utils_namespaces())()) }}
{%- endmacro -%}

For now, we're thinking about ways to call/render the _get_utils_namespaces() macro from python at parse time. In the next minor version, we should formalize the _get_utils_namespaces() convention (which other package maintainers have adopted) by moving this into a python method that expects a variable name following a set convention (<package_name>_dispatch_list). This would require no change for end users, and minimal change for package maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant