-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
BigQuery Labels #1964
BigQuery Labels #1964
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of quick comments here!
Are you also able to update a BQ integration test to set these labels? It might be tricky to confirm that the labels are being set correctly in the resulting table/view; I'm mostly just interested in adding a smoke test that makes sure dbt doesn't fail really hard if labels are present. Maybe you could add the config somewhere around here? https://github.com/fishtown-analytics/dbt/blob/e51c942e91a94936f68f2965963d3b46f1257658/test/integration/022_bigquery_test/models/table_model.sql#L4
Added an integration test and incorporated your suggestions @drewbanin |
thanks @kconvey! I kicked off the tests here. Will follow up when they finish running :) |
@drewbanin May need you to kick the integration tests off again. Made some tweaks to (hopefully) fix the integration test I added and add label support for views. |
ok - tests should be running again now :) Unsure if you saw this comment but it looks like labels can also be supplied without values as "tags". Sounds like that pattern will work just fine with this implementation. I need to do some smoke testing here, but largely, this is looking very good to me :D |
@drewbanin Did see that comment and I think the way that works in this implementation is that, since some labels have values, and yml dicts require that all keys have values, you have to set the empty string as the value for a tag when configuring in dbt. ex.
This is consistent with the bq ddl syntax for tags:
(Taken from https://cloud.google.com/bigquery/docs/adding-labels#adding_a_tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kicked off the tests here - this PR will be good to merge when they're all passing. Thanks for your contribution @kconvey!!
merging this - thanks again :) |
Implement BigQuery labels as a configurable option for the BQ adapter.
Also cleaned up Jinja whitespace a bit around 'create or replace' and 'OPTIONS', as there was a pretty huge block of empty lines in the query ultimately submitted to BQ.
#1942