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

ADAP-381: adds query tagging support and materializations to dbt-redshift #379

Closed
wants to merge 3 commits into from

Conversation

trouze
Copy link

@trouze trouze commented Mar 21, 2023

resolves #376

Description

This PR will add the ability to tag queries ran against Redshift with a string field added by a user in yaml or sql configs, much like the dbt-snowflake adapter supports.

Tradeoffs:

  • To support this, I've also added materialization files to the adapter since the base dbt-core materializations don't support query tagging (if it is desired to go the other direction and have dbt-core support this please let me know).
  • The decision was made to keep the naming of query_tag consistent with that of dbt-snowflake, despite AWS naming this feature query_group. We can certainly go back on this convention, but obviously would recommend keeping the macros consistent with that of dbt-snowflake to support multiple dispatch.

Pertinent links to this functionality and dbt-snowflake's implementation of this feature can be found in the issue discussion.

Checklist

@VersusFacit
Copy link
Contributor

VersusFacit commented Apr 12, 2023

Heya, thanks for kicking up a PR! A big one at that!

I see you're adding several local implementations of macros in order to get this done. That's potentially all right, but that also means the reviews going to take longer because I need to evaluate some design considerations.

The actual feature itself seems simple enough and as you pointed out, has precedence in snowflake.

@boxysean
Copy link

boxysean commented May 5, 2023

Hi @trouze @VersusFacit, I reviewed this for about 15 minutes. Agreed it's a large PR primarily because of the requirement of bringing in redshift__* materializations into the dbt-redshift project. As @trouze pointed out, the decision we must make is whether to take this route, or instead bring int the query-tagging concept into dbt-core and utilize it much easier in dbt-redshift (i.e., this PR changed line size would drop by ~500 lines).

Check out this thread going on dbt-labs/dbt-bigquery#685 -- @dbeatty10 do you think this PR from @trouze is sufficiently similar?

@trouze
Copy link
Author

trouze commented May 15, 2023

@boxysean thank you for having a look at this! I think it makes sense to have dbt-core utilize the query tagging concept in the base materializations there, and then have each adapter implement macros specific to how their database implements the query tagging functionality. Despite @dbeatty10 mention that redshift__* materializations will be introduced with #204, I think this approach is preferable. I'd be happy to open an issue over on dbt-core for this and cut down this PR to match if that's the direction we'd like to go, but will wait for further guidance here.

@boxysean
Copy link

Hi @trouze from my position, I think it's a good idea to raise your suggestion in dbt-labs/dbt-core and reference out to this issue and the BigQuery one. At least you will get some attention on your PR here, and a decision can be made faster, which I believe is your goal.

Copy link
Contributor

github-actions bot commented Feb 8, 2024

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 the Stale label Feb 8, 2024
@dbeatty10 dbeatty10 changed the title ADAP-381: adds query tagging support and materializations to dbt-reds… ADAP-381: adds query tagging support and materializations to dbt-redshift Feb 9, 2024
@dbeatty10 dbeatty10 added ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering and removed Stale labels Feb 9, 2024
@ddors3y
Copy link

ddors3y commented May 1, 2024

This feature has came back up as something we are really interested in. Looks like it hasn't got much activity lately so wanted to check and see if there was another place this got moved or if there was alternative.

@trouze
Copy link
Author

trouze commented May 1, 2024

@ddors3y I believe we were all in agreement with moving this to dbt-core, I opened a draft illustration of what it might look like there but just never found the time to bring it to completion. I may get back around to it coming up here though.

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 the Stale label Oct 29, 2024
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Although we are closing this PR as stale, it can still be reopened to continue development. Just add a comment to notify the maintainers.

@github-actions github-actions bot closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ok to test ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ADAP-381] Enable tagging of query_group in configs
7 participants