-
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
ct-2587: adds query tag macros and updates table materialization #7770
base: main
Are you sure you want to change the base?
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.
Thanks for this @trouze! I'm supportive of moving forward here with your proposal.
Depending on how deep you're willing to go here, I'd love to see follow-on PRs in the adapter repos for each of the implementation:
- Snowflake PR (just removing code): TK
- Redshift PR: ADAP-381: adds query tagging support and materializations to dbt-redshift dbt-redshift#379
- BigQuery issue (no PR yet): [ADAP-501] [Feature] Leverage Bigquery Comments at Model level dbt-bigquery#685
{% macro default__set_query_tag() %} | ||
{{ log("Couldn't set query tag, current adapter does not have get_current_query_tag() macro implemented.",info=True) }} | ||
{% endmacro%} | ||
|
||
{% macro default__unset_query_tag() %} | ||
{{ log("Couldn't unset query tag, current adapter does not have get_current_query_tag() macro implemented.",info=True) }} | ||
{% endmacro%} |
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.
This is fine during development, but I think we'd want this to be an actual noop by default (= just return an empty string). No need to clutter the logs for Postgres users with a bunch of these messages.
@@ -1,5 +1,6 @@ | |||
{% materialization table, default %} | |||
|
|||
{% set original_query_tag = set_query_tag() %} |
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7770 +/- ##
===========================================
- Coverage 86.23% 65.13% -21.10%
===========================================
Files 174 174
Lines 25518 25518
===========================================
- Hits 22005 16622 -5383
- Misses 3513 8896 +5383 ☔ View full report in Codecov by Sentry. |
This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days. |
resolves #7648
Description
Idea is to support query tagging out of the box with dbt-core, and let each adapter implement their own version of how this will work via dispatching of the
set_query_tag()
andunset_query_tag()
macros.Checklist
changie new
to create a changelog entry