Add functionality to persist descriptions to nested columns in bigquery

[bodschut]

resolves #2549

Description

Changed the existing function in the bigquery adapter implementation to recursively traverse nested columns in bigquery and add descriptions if they are provided in a schema.yml file. Requires that nested fields are addressed with dotted syntax in the schema file.

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.

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bob De Schutter.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email [email protected]
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[drewbanin]

thanks for opening this PR @bodschut! This is a really good and positive change :)

Are you able to sign the CLA form? Once that's done, we'd be happy to review this PR!

[bodschut]

Ok, I signed it :-)

[drewbanin]

@cla-bot check

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bob De Schutter.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email [email protected]
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[drewbanin]

ok - thanks for signing it! I do see the signature, but the CLA bot is upset that your existing commit is not associated with your GitHub user :). This is the mechanism used to determine that commits to this repo are contributed by CLA signatories, so if you could try running

git config --global user.email [email protected]

with your github email address, i think we should be in good shape.

We'll take a look at this PR and add some comments inline! Thanks again for opening it :)

[bodschut]

Yeah, I signed the CLA with my personal email address and the commit was probably done with my professional address... I changed it locally so I think I should do a dummy commit to get it in?

[beckjake]

I think you'll want to git commit --amend --reset-author --no-edit and then force-push once you fix the git config settings.

[beckjake]

You could also add your professional address to your github, if that works for you. The cla-bot actually goes by username, and the problem is that github doesn't know that your commit is from @bodschut because it just compares emails (I think!).

[bodschut]

seems to be resolved now, thanks for the help :-)

[beckjake]

Hey @bodschut, thanks for your contribution!

It looks like flake8 is upset with some of your line lengths so the unit tests are failing.

Can you also please add an integration test for this as well? I'm happy to help with that, there are some existing tests in test/integration/060_persist_docs_tests that you could use as a starting point. You'd probably need to make a new models directory entirely and point the new test there (so you don't break snowflake tests).

Edit: Also, please add yourself to the contributors section of CHANGELOG.md and add a changelog entry for this PR/issue, so we can give you credit in our release notes!

[beckjake]

This is awesome! Thanks for sticking with it and getting that test in, this is a great addition.

I have 2 tiny comments here, no need for commenting out code if we can remove it, that's what version control is for! Once those are done I'll kick off the tests and we'll be good to merge this for 0.17.1

@drewbanin did you want to locally smoke test this? I feel pretty good about the tests!

[beckjake]

This is a weird result! The indices for the nested structs in the docs generate tests seem to have changed as a result of this PR - this is the part of the diff that actually reflects a failure:

E   -                                         'nested_field': {'comment': None,
 
E   -                                                          'index': 4,
 
E   -                                                          'name': 'nested_field',
 
E   -                                                          'type': 'STRUCT<field_4 '
 
E   -                                                                  'INT64, '
 
E   -                                                                  'field_5 '
 
E   -                                                                  'INT64>'},
 
E                                             'nested_field.field_4': {'comment': None,
 
E   -                                                                  'index': 5,
 
E   ?                                                                           ^
 
E   
 
E   +                                                                  'index': 4,
 
E   ?                                                                           ^
 
E   
 
E                                                                      'name': 'nested_field.field_4',
 
E                                                                      'type': 'INT64'},
 
E                                             'nested_field.field_5': {'comment': None,
 
E   -                                                                  'index': 6,
 
E   ?                                                                           ^
 
E   
 
E   +                                                                  'index': 5,
 
E   ?                                                                           ^
 
E   
 
E                                                                      'name': 'nested_field.field_5',
 
E                                                                      'type': 'INT64'}},
 
E                                 'metadata': {'comment': None,

I don't know why that would be, but It looks like the parent field (nested_field) now does show up, so things get re-indexed. I think that's because this drops the where clause that excluded structs from the catalog.

Should those come back?

[bodschut]

Bigquery allows you to put a description on the parent field that has type STRUCT, that's why I removed the where clause that excluded these. Maybe we should take that into account in the test?

[drewbanin]

This is awesome!! Longer term, I think I'd like the docs site to render these fields more like the BQ UI:

That said, I think this PR is great, and we'll have all the info we need in the docs site to implement those changes when we choose to. Approved on my end :)