-
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
Adding test materialization, implement for data tests #3181
Conversation
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.
I know you're still finalizing the last few pieces, but I couldn't help myself, so I took this for a spin. Everything worked the way I'd hoped! I was able to override the test
materialization locally, with both default and adapter-specific implementations, and dbt preferred my own each time.
@@ -0,0 +1,10 @@ | |||
{%- materialization test, default -%} |
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.
I notice that the file name is materializations/test/data.sql
, and I think this is rightly just materializations/test.sql
. We'll want to use one & the same materialization for both data one-off and schema generic/reusable tests. (I know that's one of the next issues to come.)
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.
No, please help yourself! Thanks for taking it for a spin. I'll update the name accordingly :)
bfe080e
to
3b8c814
Compare
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.
All good in my book!
f"Returned {num_rows} rows and {num_cols} cols, but expected " | ||
f"1 row and 1 column" | ||
) | ||
return int(table[0][0]) |
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.
Rather than just returning a single number to the execute
method, I imagine the materialization-calling method should construct and return the full RunResult
, similar to _build_run_model_result
. It seems like that will enable us to manage the dynamic message
interpolation, as well as recording the adapter_response
.
Of course, those can and should be new issues! It's cool to see the path taking shape.
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.
LGTM
f2cb813
to
5841713
Compare
5841713
to
12e281f
Compare
core/dbt/task/test.py
Outdated
.format(context) | ||
) | ||
|
||
MacroGenerator(materialization_macro, context)() |
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 document what this line with MacroGenerator does. If you don't already know it might be confusing.
resolves #3154
Description
This PR pulls data test logic out of a CTE injected during compilation and puts it in a new
test
materialization. This mimics existing data test logic (simple checking ofcount(*)
), subsequent changes will add more functionality.Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.