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

ct-2587: adds query tag macros and updates table materialization #7770

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

trouze
Copy link
Contributor

@trouze trouze commented Jun 2, 2023

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() and unset_query_tag() macros.

Checklist

@cla-bot cla-bot bot added the cla:yes label Jun 2, 2023
@jtcohen6 jtcohen6 added the Team:Adapters Issues designated for the adapter area of the code label Jun 25, 2023
Copy link
Contributor

@jtcohen6 jtcohen6 left a 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:

Comment on lines +31 to +37
{% 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%}
Copy link
Contributor

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() %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want this everywhere that dbt-snowflake currently has it implemented:

  • table
  • view
  • incremental
  • seed
  • snapshot
  • test
  • MV / dynamic table

Then we can totally delete & remove the Snowflake-specific implementations of test, snapshot, and seed — which only exist to support setting/unsetting query tags.

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (991618d) 86.23% compared to head (074dfde) 65.13%.
Report is 263 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

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.

@github-actions github-actions bot added stale Issues that have gone stale and removed stale Issues that have gone stale labels Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2587] [Feature] Support Query Tagging in Base Materializations
2 participants