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.

Sign up for GitHub

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

Jump to bottom

feat: lakeformation tags for columns support #185

Merged
merged 7 commits into from
Mar 27, 2023
Merged

feat: lakeformation tags for columns support #185

merged 7 commits into from
Mar 27, 2023

Conversation

svdimchenko
Copy link
Contributor

@svdimchenko svdimchenko commented Mar 22, 2023 •
edited
Loading

Description

Extend lake formation tags adding feature from PR with possibility to set LF tags for columns.

Part of #22

Models used to test - Optional

Here is example model config, lf_tags_columns config is same for all materialisation options: table, incremental, view, seed.

{{
  config(
    materialized='incremental',
    incremental_strategy='append',
    on_schema_change='append_new_columns',
    lf_tags_columns={'my_tag_key': {'my_tag_value': ['column_name_1']}}
  )
}}

Checklist

  • You followed contributing section
  • You added unit testing when necessary
  • You added functional testing when necessary

@svdimchenko svdimchenko requested a review from nicor88 March 23, 2023 09:45
@nicor88
Copy link
Contributor

nicor88 commented Mar 23, 2023

@svdimchenko great work here, I tested the feature and it works just fine :) after you address this: #185 (comment) we can merge it.

nicor88
nicor88 previously approved these changes Mar 23, 2023
View reviewed changes
Copy link
Contributor

@nicor88 nicor88 left a comment

Choose a reason for hiding this comment

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

💯

@svdimchenko svdimchenko requested a review from nicor88 March 23, 2023 14:26
@svdimchenko
Copy link
Contributor Author

@nicor88 could you advice regarding ETA to merge this PR and release new version ?

@nicor88
Copy link
Contributor

nicor88 commented Mar 25, 2023

@svdimchenko I didn't rush to merge this as I would like to run another round of checks ;)
The idea is to merge it beginning of next week.
Regarding release, could happen next week or the one after if we have enough changes to release - I need to sync with the other maintainers.

@nicor88 nicor88 merged commit faa37ff into dbt-labs:main Mar 27, 2023
@henriblancke
Copy link

@nicor88 @svdimchenko do you think it'd be possible to define tags on columns in the properties.yml file of a model; similar to tests? Or is that type of config not accessible from the adapter?

@nicor88
Copy link
Contributor

nicor88 commented Mar 27, 2023

@henriblancke I believe that they could be added from dbt_project.yml, for a specific model or specific group of models.
Generally the config in the dbt_project.yml are interpolated with local model config, in case of overlap specific model properties will have priority.
On a side note, picking the columns tags from schema.yml could require some more work, as it requires fetching the tags from target_relation. Worth a try.

@svdimchenko svdimchenko deleted the feature/lf-tags-columns-support branch April 28, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicor88 nicor88 nicor88 approved these changes

@jessedobbelaere jessedobbelaere Awaiting requested review from jessedobbelaere

@Jrmyy Jrmyy Awaiting requested review from Jrmyy

@mattiamatrix mattiamatrix Awaiting requested review from mattiamatrix

@thenaturalist thenaturalist Awaiting requested review from thenaturalist

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@svdimchenko @nicor88 @henriblancke