-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
set default materialized
for test node configs
#2902
Conversation
Looks like a great easy win! |
Just adding my two cents here, feel free to ignore if I missed something in the big picture. In my opinion materialized = test can make great sense for data tests, even if it means extending the "formal" materialized meaning (as noted by @kwigley ) For schema test I personally think that materialized should be set to the model materialization. I can see two main benefits:
I don't know if it is possible to do so, I don't have enough knowledge of the codebase What do you think? |
@rsella Appreciate you weighing in! A lot of our thinking for the release after the next is around tests: how can we rationalize them, make them a bit more fully-featured, and reconcile confusing differences between data tests and schema tests. Per the proposal in #2593, we want a bit more heft to tests such that they can wrap SQL (either from data test file or schema test macro), apply a common set of configs (severity, limit), calculate the number of failures, and optionally write those failing results to a table. This PR has helped me realize that the right wrapper may in fact be a formal Jinja materialization, named
This already works, based on the way that test selection works today— With this code change: |
@jtcohen6 all clear, seems like the materialized = test proposal is perfect then Thanks for the explanation btw |
materialized
for test node configsmaterialized
for test node configs
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.
Let's do it.
This PR has made me realize that it's possible to do things like:
seeds:
+materialized: view
This results in a database error, as dbt tries to wrap a lot of missing information in a create view
statement. Or:
seeds:
+materialized: asdfghjkl
Which does raise a compilation error, but perhaps not the one you'd expect:
No materialization 'asdfghjkl' was found for adapter postgres! (searched types 'default' and 'postgres')
I don't know if we should consider that user error and do nothing, or actively raise exceptions when users set the materialized
config for non-models. In any case, it's an ongoing question and outside the scope of this PR to fix.
(possibly) resolves #2806
Description
After noticing similar behavior for seed and snapshot node configs, this change sets the default
materialized
for test node configs totest
. It aligns selection behavior, but this is definitely extending the meaning ofmaterialized
.Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.