dbt-redshift icon indicating copy to clipboard operation
dbt-redshift copied to clipboard

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

Open trouze opened this issue 1 year ago • 7 comments

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

  • [x] I have read the contributing guide and understand what's expected of me
  • [x] I have signed the CLA
  • [x] I have run this code in development and it appears to resolve the stated issue
  • [x] This PR includes tests, or tests are not required/relevant for this PR
  • [x] I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
  • [x] I have run changie new to create a changelog entry

trouze avatar Mar 21 '23 23:03 trouze

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.

VersusFacit avatar Apr 12 '23 22:04 VersusFacit

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 https://github.com/dbt-labs/dbt-bigquery/issues/685 -- @dbeatty10 do you think this PR from @trouze is sufficiently similar?

boxysean avatar May 05 '23 08:05 boxysean

@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.

trouze avatar May 15 '23 17:05 trouze

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.

boxysean avatar May 17 '23 08:05 boxysean

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[bot] avatar Feb 08 '24 01:02 github-actions[bot]

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.

ddors3y avatar May 01 '24 21:05 ddors3y

@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.

trouze avatar May 01 '24 21:05 trouze