Added support for setting policy tags for columns in BigQuery #2589

azhard · 2020-06-25T14:19:10Z

resolves #2586

Description

Enables users to set policy tags on BigQuery columns by specifying the policy tag in the resource file for their models.

Checklist

I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

drewbanin

I took a quick first pass here. Looks like there's one question to answer still about the specifics of the column-level config. Overall, this looks very good, and I appreciate you taking the time on this PR! The tests look great :)

drewbanin · 2020-07-01T13:38:46Z

docker/requirements/requirements.0.18.0b1.txt

@@ -17,7 +17,7 @@ decorator==4.4.2
 docutils==0.15.2
 google-api-core==1.16.0
 google-auth==1.16.1
-google-cloud-bigquery==1.24.0
+google-cloud-bigquery==1.25.0


does 1.25 specifically add support for policy tags? We had some issues with recent releases of a couple of Google libraries, so I just wanted to double check that bumping this version number is required for this change

Hey @drewbanin, support for policy tags was added in 1.25.0 which is why I updated the version
https://googleapis.dev/python/bigquery/latest/changelog.html

drewbanin · 2020-07-01T13:48:35Z

plugins/bigquery/dbt/adapters/bigquery/impl.py

@@ -610,6 +610,10 @@ def update_column_descriptions(self, relation, columns):
                column_config = columns[column.name]
                column_dict = column.to_api_repr()
                column_dict['description'] = column_config.get('description')
+                if column_config.get('tags'):


Let's not use tags directly -- can we instead create a new config called policy_tags and use it here? That might involve reaching into the Column contracts defined in dbt.

Alternatively, I do like the idea of repurposing a property in the meta dict for this config. I know @jtcohen6 voiced his opposition because meta is intended to be purely informational, which is correct, but I'm looking through the code now and I think adding a new column-level property specific to BigQuery might end up being a can of worms!

@jtcohen6 @beckjake - what do you two think?

I do agree with @jtcohen6 that meta should stay informational/user-controlled.

I think we are going to have to deal with the idea of adapter-specific column fields (and adapter-specific schema.yml entries), much like we did with config. Maybe as a first pass, we could change UnparsedColumn (or even its ancestor class HasDocs) to derive from ExtensibleJsonSchemaMixin so it can get arbitrary keys? I haven't really thought about the consequences of that at all, but it's what I'd look at first.

I'm supportive of core's classes (HasDocs, ColumnInfo) accepting arbitrary keys, as long as there's some way for plugin authors to enumerate those additional keys, similar to AdapterSpecificConfigs.

If having HasDocs derive from ExtensibleJsonSchemaMixin is the correct solution here, any guidance on how to setup an AdapterSpecificConfigs equivalent for this case?

Hey @jtcohen6 and @beckjake, any thoughts on the above question?

Hey @beckjake, I'm more unsure about how to setup the equivalent of a AdapterSpecificColumnInfo so that I can pass use a BigQueryColumnInfo with policy_tags property here but elsewhere it defaults to ColumnInfo.

I know inside BigQueryAdapter we set Column = BigQueryColumn, I'm assuming we'd want to do something similar for the ColumnInfo but not sure how to ensure that the passed in ColumnInfo gets used elsewhere

I think you just want to make ColumnInfo itself allow additional properties, and then the bigquery code that wants it can extract them out if it needs them. Making that aspect of parsing itself adapter-specific would be quite an overhaul.

Thanks for clarifying @beckjake. Out of curiosity, what's the thinking behind creating the new AdditionalPropertiesMixin class vs. using AdditionalPropertiesAllowed and removing the default value for _extra to get around the TypeError: non-default argument ... follows default argument error?

Well, this answer is kind of circular reasoning, but: Because I want to have a default value for _extra! It's convenient.

Gotcha, okay updating accordingly!

beckjake

I have a couple notes, really they're more about the passage of time than actual changes. But this looks awesome, thanks for contributing another great bigquery feature!

beckjake · 2020-07-30T13:43:22Z

core/dbt/contracts/graph/unparsed.py

@@ -66,13 +66,45 @@ class Docs(JsonSchemaMixin, Replaceable):
    show: bool = True


+class AdditionalPropertiesMixin:


I think this actually exists in dev/marian-anderson now (sorry, I've been busy since you opened this and should've clarified!) if you rebase/merge in, you can import it from dbt.contracts.util.

beckjake · 2020-07-30T13:48:25Z

CHANGELOG.md

+- Added support for setting policy tags for BigQuery columns ([#2586](https://github.com/fishtown-analytics/dbt/issues/2586), [#2589](https://github.com/fishtown-analytics/dbt/pull/2589))



Can you push this up into the top when you rebase/merge? If we do a release in the meantime (likely), I'll fix it myself! b1 shipped a while ago

beckjake · 2020-07-30T13:58:50Z

CHANGELOG.md

@@ -42,7 +43,7 @@ Contributors:
 - [@alf-mindshift](https://github.com/alf-mindshift) ([#2431](https://github.com/fishtown-analytics/dbt/pull/2431))
 - [@scarrucciu](https://github.com/scarrucciu) ([#2508](https://github.com/fishtown-analytics/dbt/pull/2508))
 - [@southpolemonkey](https://github.com/southpolemonkey) ([#2511](https://github.com/fishtown-analytics/dbt/issues/2511))
- - [@azhard](https://github.com/azhard) ([#2517](https://github.com/fishtown-analytics/dbt/pull/2517), ([#2521](https://github.com/fishtown-analytics/dbt/pull/2521)), [#2547](https://github.com/fishtown-analytics/dbt/pull/2547))
+ - [@azhard](https://github.com/azhard) ([#2517](https://github.com/fishtown-analytics/dbt/pull/2517), ([#2521](https://github.com/fishtown-analytics/dbt/pull/2521)), [#2547](https://github.com/fishtown-analytics/dbt/pull/2547), [#2588](https://github.com/fishtown-analytics/dbt/pull/2588))


Can you move this issue up into a new contributors block? I think it will actually exist if you pull in dev/marian-anderson at the moment.

beckjake · 2020-07-30T17:07:10Z

Thanks so much! I'm going to get this merged in for 0.18.0b2.

cla-bot bot added the cla:yes label Jun 25, 2020

drewbanin mentioned this pull request Jul 1, 2020

Add support for adding BigQuery policy tags #2586

Closed

drewbanin self-requested a review July 1, 2020 13:37

drewbanin reviewed Jul 1, 2020

View reviewed changes

beckjake approved these changes Jul 30, 2020

View reviewed changes

azhard added 8 commits July 30, 2020 10:55

Added support for setting policy tags for columns in BigQuery

8782714

Updated pull request #

e3193c6

Changed update_column to update_columns

1423318

Rebased

b964f6d

Linting fix

0a64708

bigquery integration test fix

8ec93d9

Addressed PR comments

007cb45

Addressed PR commments

d2c5783

beckjake merged commit 21a3462 into dbt-labs:dev/marian-anderson Jul 30, 2020

dbeatty10 mentioned this pull request Nov 22, 2022

[CT-1332] [Bug] Removing the policy_tags property from a column does not remove the policy tag(s) dbt-labs/dbt-bigquery#349

Closed

2 tasks

dataders mentioned this pull request Mar 11, 2024

update external table columns dbt-labs/dbt-external-tables#252

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for setting policy tags for columns in BigQuery #2589

Added support for setting policy tags for columns in BigQuery #2589

azhard commented Jun 25, 2020

drewbanin left a comment

drewbanin Jul 1, 2020

azhard Jul 6, 2020

drewbanin Jul 1, 2020

beckjake Jul 1, 2020

jtcohen6 Jul 1, 2020

azhard Jul 6, 2020

azhard Jul 21, 2020

azhard Jul 27, 2020

beckjake Jul 28, 2020

azhard Jul 29, 2020

beckjake Jul 29, 2020

azhard Jul 29, 2020

beckjake left a comment

beckjake Jul 30, 2020

beckjake Jul 30, 2020

beckjake Jul 30, 2020

beckjake commented Jul 30, 2020

		@@ -66,13 +66,45 @@ class Docs(JsonSchemaMixin, Replaceable):
		show: bool = True


		class AdditionalPropertiesMixin:

		- Added support for setting policy tags for BigQuery columns ([#2586](https://github.com/fishtown-analytics/dbt/issues/2586), [#2589](https://github.com/fishtown-analytics/dbt/pull/2589))

Added support for setting policy tags for columns in BigQuery #2589

Added support for setting policy tags for columns in BigQuery #2589

Conversation

azhard commented Jun 25, 2020

Description

Checklist

drewbanin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beckjake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beckjake commented Jul 30, 2020