dbt-redshift
dbt-redshift copied to clipboard
ADAP-381: adds query tagging support and materializations to dbt-redshift
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 featurequery_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
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.
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 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.
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.
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.
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 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.