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

Snowflake model configs for tests, too #23

Closed
jtcohen6 opened this issue Apr 12, 2021 · 5 comments
Closed

Snowflake model configs for tests, too #23

jtcohen6 opened this issue Apr 12, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@jtcohen6
Copy link
Contributor

picks up from dbt-labs/dbt-core#2981

Describe the feature

Many model configs are model only, but some are also relevant for tests! There are two Snowflake examples that come immediately to mind:

  1. snowflake_warehouse: Run this test with a different warehouse (= compute resource) from the target.warehouse configured in the profile
  2. query_tag: Slap a query tag on this test that differs from the default query tag

I like these because each represents a different type of change to achieve parity:

  1. The snowflake_warehouse config is executed as a python hook before and after calling the materialization. (I'm not entirely sure why). I think we'd just need to add these lines to the test task:

https://github.com/fishtown-analytics/dbt/blob/7ec5c122e17ab7e1a5dd1e1cab36527159564a0f/core/dbt/task/run.py#L246-L250

(In all likelihood, we should rename those hooks to something more generic, and add them to all the other task types, too.)

  1. Model materializations call the set_query_tag() and unset_query_tag() macros. The Snowflake test materialization could handle this with ease:

https://github.com/fishtown-analytics/dbt/blob/7ec5c122e17ab7e1a5dd1e1cab36527159564a0f/plugins/snowflake/dbt/include/snowflake/macros/materializations/table.sql#L3
https://github.com/fishtown-analytics/dbt/blob/7ec5c122e17ab7e1a5dd1e1cab36527159564a0f/plugins/snowflake/dbt/include/snowflake/macros/materializations/table.sql#L38

Of course, in order for this to work, tests first need to be able to "carry" these configs. Then, in all the ways described by dbt-labs/dbt-core#3253, those configs can be set in-file config() blocks, inherited, set in dbt_project.yml, etc.

@jtcohen6 jtcohen6 transferred this issue from dbt-labs/dbt-core Oct 12, 2021
@matt-winkler
Copy link
Contributor

  • Where are generic test yamls parsed? I’m able to modify the TestConfig class here to expect snowflake_warehouse and query_tag, but those values aren’t getting assigned despite us already having the test config concept (for row limits and such)

    • Related, is this the right place to be doing this (in Core) since these are snowflake specific?
  • The snowflake materializations don’t include one for tests, so I’m thinking that would need to be added and include the macro to set / unset the query tag instead of doing that in Core. Thoughts?

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Jan 4, 2022

You're right, this would require a mix of dbt-core and dbt-snowflake changes. There's two questions:

  1. How to get the test to "accept" adapter-specific configs? (requires dbt-core change)
  2. How to actually use that config when the test is being executed / materialized? (changes in dbt-core + dbt-snowflake)

For the first question: If it's a singular test, you can supply any arguments you want to config() / dbt_project.yml. If it's a specific instance of a generic test (i.e. most tests), dbt-core limits the set of acceptable configs to a strict set of options. That was important to avoid overlap with test arguments. Now that a test can accept the config: property (since v0.21), and thereby clearly delineate between test configs and test args, we should make it possible to pass whatever configs you want into config:, rather than only pulling out those specified values.

version: 2

models:
  - name: my_model
    columns:
      - name: id
        tests:
          - unique:
              config:
                whatever_i_want: custom_value # i.e. for use in a custom 'test' materialization

For the second, it varies by config:

  • query_tag: We could implement this similarly to how we support query tags for seeds and snapshots, by wrapping the default test materialization
  • snowflake_warehouse: This one is trickier, since it's implemented as a pre_model_hook, rather than a macro. In dbt-core, the pre_model_hook method is called by the ModelRunner (which is inherited by SeedRunner and SnapshotRunner), but not by TestRunner. So that would require a core change, too.

@pmalafosse
Copy link

pmalafosse commented Mar 1, 2022

Would it be possible to also use the Snowflake warehouse defined for the model when we use run_query too? e.g. https://getdbt.slack.com/archives/CJN7XRF1B/p1633667936442800?thread_ts=1633666825.441600&cid=CJN7XRF1B

@jtcohen6
Copy link
Contributor Author

My comment above described two components needed for this change:

  • Generic tests should accept any config values. We've since opened a dbt-core issue for this: [CT-908] Allow generic tests to accept arbitrary configs dbt-core#5532
  • adapter.pre_model_hook + adapter.post_model_hook are the mechanism for custom snowflake_warehouse config (and they are, despite the name, not the same as user-supplied pre-hook + post-hook, which run mid-materialization). These should run before tests, too. Ideally, per @pmalafosse's comment, they should also wrap a model's compilation step, so as to include any introspective queries that model runs ahead of time (via run_query). Let's open a dbt-core issue for this.

If both of those changes are made in dbt-core, I don't actually think there are any other changes needed in dbt-snowflake.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Sep 6, 2022

I've turned the comment above into a new issue in dbt-core: dbt-labs/dbt-adapters#212

I'm going to close this issue, since I believe all the remaining changes are required in dbt-core, not dbt-snowflake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants