When column config says quote, use quotes in SQL to add comments

[gshank]

resolves #2539

Description

When 'persist_docs' was turned on, an error occurred if a column had config 'quote: true' because the DDL to add the comment did not quote the column name. This fix passes the column quote config on to the ColumnInfo object and checks that config when constructing the SQL to add a comment.

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.

[beckjake]

@cla-bot check

[cla-bot]

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

[beckjake]

This looks mostly right to me, though I don't see any changes to support snowflake, nor any tests for it. I have some inline comments as well.

[beckjake]

Suggested change

	comment on column {{ relation }}.{{ adapter.quote(column_name) if column_dict[column_name]['quote'] == True else column_name }} is {{ escaped_comment }};
	comment on column {{ relation }}.{{ adapter.quote(column_name) if column_dict[column_name]['quote'] else column_name }} is {{ escaped_comment }};

True is the only truthy value we need to care about for quote, so no need to actually compare it to True.

[beckjake]

Unfortunately, I don't think this is a very useful test for postgres/redshift. I know columns that start with numbers aren't allowed in bigquery, but this column doesn't require quoting on postgres/redshift and would work fine regardless, so that's not much of a test. Snowflake will have case-sensitivity issues so it's probably still useful there.

I think you'll have to turn this into its own test case. I have done a bit of reading and I actually think that it's fine to exclude Bigquery from this test entirely: since the only things allowed in column names don't require quoting, and unquoted column names are case-preserving, quoting columns on Bigquery seems to be totally meaningless (identifiers are less restrictive, so quoting matters there: hyphens come to mind).

[beckjake]

Once the release version/changelog nonsense is all sorted, this looks good to me! Love to see those snowflake tests behaving like everyone else too (comments in the catalog), so thanks for getting that in there!

[jtcohen6]

LGTM, nice work on this!

Thinking beyond this PR: I wonder if there's a good way to abstract over

{{ adapter.quote(column_name) if column_dict[column_name]['quote'] else column_name }}

Since that's effectively what we'd need to fix #2468 as well.

[beckjake]

it does seem like a macro or adapter method that we could write, doesn't it?

{% macro quoted_column(column_dict, column_name) %}
  {% set result = column_name %}
  {% if column_dict[column_name]['quote'] %}
    {% set result = adapter.quote(column_name) %}
  {% endif %}
  {% do return(result) %}
{% endmacro %}