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.

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

Support persist_docs for column descriptions #170

Merged
merged 5 commits into from
Jun 6, 2021

Conversation

cristianoperez
Copy link
Contributor

resolves #84

Description

Adds support for column descriptions when its in Delta format and persist_docs = {'columns': true}

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 cla-bot bot added the cla:yes label May 27, 2021
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @cristianoperez! A few small comments from me, after which this looks good to go.

I'd love to add a test for this, but I think the right place for that test is actually in dbt-adapter-tests. We wrote those tests at a time when persist_docs was a brand-new feature; today, it's functionality that a dbt user can reasonably expect across a good number of adapters.

{% for column_name in column_dict %}
{% set comment = column_dict[column_name]['description'] %}
{% set comment_query %}
ALTER TABLE {{ relation }} ALTER COLUMN {{ adapter.quote(column_name) if column_dict[column_name]['quote'] else column_name }} COMMENT '{{ comment }}';
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Per comments in Support persist_docs for column descriptions #84, didn't you find that CHANGE COLUMN was more effective than ALTER COLUMN (even though it doesn't seem there should be a difference)?
  2. Style nit: We prefer to lowercase keywords in DDL/DML statements

@cristianoperez
Copy link
Contributor Author

Thanks for the contribution @cristianoperez! A few small comments from me, after which this looks good to go.

I'd love to add a test for this, but I think the right place for that test is actually in dbt-adapter-tests. We wrote those tests at a time when persist_docs was a brand-new feature; today, it's functionality that a dbt user can reasonably expect across a good number of adapters.

Thanks for the info, I spent a few time thinking about how to create a test for this

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This looks great @cristianoperez! Could you add a note to the changelog, under dbt next, and add yourself to the list of contributors?

@cristianoperez cristianoperez requested a review from jtcohen6 June 6, 2021 02:59
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Thanks @cristianoperez!

@jtcohen6 jtcohen6 merged commit 458cdcc into dbt-labs:master Jun 6, 2021
@jtcohen6 jtcohen6 mentioned this pull request Jun 14, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support persist_docs for column descriptions
2 participants