Snowflake column comments #2321

sfc-gh-aseitz · 2020-04-14T17:24:09Z

resolves:

Description

Adding in support for Persist Docs into the Snowflake adapter and laying out a general framework for other adapters to support this feature.

Some caveats:

It only supports table materialization currently. Incremental materializations will not be refreshed with new docs until further logic is added to them. Additionally, views in Snowflake do not support altering column comments and must be created when the view is created.
Snowflake uses $$ string literal $$ to make ingesting strings with quotes easy. Therefore in your docs, if you have a $ we will replace it with [$] such that you never have two $$ next to each other.

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 · 2020-04-14T17:24:12Z

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @snowflakeseitz

beckjake · 2020-04-14T17:44:45Z

@cla-bot check

cla-bot · 2020-04-14T17:44:48Z

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

beckjake · 2020-04-14T21:04:40Z

Additionally, views in Snowflake do not support altering column comments and must be created when the view is created.

Just out of curiosity, how does one do that? I can see how to set a comment for the whole view, but not for columns. Is the same syntax viable for tables?

sfc-gh-aseitz · 2020-04-14T21:16:15Z

@beckjake exactly, you would have to inject it into the SQL statement itself:
https://support.snowflake.net/s/article/How-To-Add-a-Comment-to-the-Column-of-a-View

sfc-gh-aseitz · 2020-04-14T23:50:30Z

@drewbanin let me know what I need to get this over the finish line :)

drewbanin

Hey @snowflakeseitz - I just dropped some comments inline here. Let me know what you think :)

It would be really good to add some tests here! Can you think of a good way to test this functionality? I can imagine an integration test which:

Creates a table/view with comments
Runs some information schema queries to ensure the comments have been set successfully

drewbanin · 2020-04-20T20:26:03Z

plugins/snowflake/dbt/include/snowflake/macros/adapters.sql


 {% endmacro %}

 {% macro snowflake__create_view_as(relation, sql) -%}
  {%- set secure = config.get('secure', default=false) -%}
  {%- set copy_grants = config.get('copy_grants', default=false) -%}
  {%- set sql_header = config.get('sql_header', none) -%}
+  {%- set raw_persist_docs = config.get('persist_docs', {}) -%}
+  {%- set relation_comment = get_relation_comment(raw_persist_docs, model) -%}
+  {%- set column_comment = get_relation_column_comments(raw_persist_docs, model) -%}


While it appears that we're not going to be able to set column comments on views, I do think we can set a view-level comment!

Can you add a call to snowflake__alter_relation_comment at the bottom of this macro to render the SQL required to add a view comment?

drewbanin · 2020-04-20T20:28:19Z

plugins/snowflake/dbt/include/snowflake/macros/adapters.sql

@@ -156,3 +172,19 @@
    alter table {{ relation }} alter {{ adapter.quote(column_name) }} set data type {{ new_column_type }};
  {% endcall %}
 {% endmacro %}
+
+{% macro snowflake__alter_relation_comment(relation, relation_comment) -%}
+  comment on table {{ relation }} IS $${{ relation_comment | replace('$', '[$]') }}$$;


Can we use relation.type instead of table here? That will let us use this macro for both tables and views :)

Same for the implementation of snowflake__alter_column_comment below!

drewbanin · 2020-04-27T19:19:38Z

hey @snowflakeseitz - do you think you'll be able to revisit this PR? I think we can indeed proceed without support for column-level comments on views for now, and there are certainly ways we can add support for them in the future too. Let me know if you're able to respond to the comments I dropped inline in this PR :)

sfc-gh-aseitz · 2020-04-27T19:22:51Z

@drewbanin I fell off the face of the earth last week with work :) I will make these changes and test them by tomorrow

drewbanin · 2020-04-27T23:54:38Z

@jtcohen6 can you give this a review tomorrow? Please loop in @beckjake if you have any questions :)

jtcohen6

This passes local testing for me. I left one comment around an argument naming inconsistency, though it doesn't actually raise an error.

Discussion: where should this happen?

Does it make sense that the comment on + alter statements should be called by snowflake__create_table_as and snowflake__create_view_as, as opposed to the Snowflake table and view materializations?

On BigQuery, we can embed the object description within the create statement's options(). If we think that BQ should set the standard, then it makes sense that we'd append comment on + alter where there are here, since it's closest to parity.

If we instead say that the standard should be Postgres/Redshift/Snowflake, where we're effectively adding descriptions by post hooks, I think it would make more sense (and be more modular) to move config.get('persist_docs'), the calls to get_relation_comment/get_relation_column_comment, and alter_relation_comment/alter_column_comment into the materialization instead, and treat them like hooks.

(I know that Snowflake clustering operates as a series of alter statements within the tight bound of snowflake__create_table_as, but I've always viewed that as a bit of a hack.)

I don't think that line of thinking should block this PR, which is just about good to go. Since an ulterior goal is scoping out future contributions that might add this functionality on Postgres and Redshift, I do think we're setting some precedent here.

Future work

@drewbanin We discussed yesterday two possible approaches for implementing column comments for the Snowflake view materialization. I imagine we'll scope that for a future PR, building off the work here.

It also looks like this branch is now 59 commits ahead of (and several thousand line diffs away from) dev/octavius-catto. I just want to confirm that that's a result of those commits having been squashed in the base branch.

jtcohen6 · 2020-04-28T15:44:09Z

core/dbt/include/global_project/macros/etc/get_relation_comment.sql

@@ -16,3 +16,19 @@
  {% endif %}

 {% endmacro %}
+
+
+{% macro get_relation_column_comments(persist_docs, model) %}


I think the exception below would return an error, because the macro argument is named persist_docs but the exception expects raw_persist_docs.

As it is, I haven't been able to actually get this exception/error. It's preempted by a parsing error, now that model + project configs have tighter type enforcement:

Invalid config field: "persist_docs" must be a dict

All the same, the argument should be named raw_persist_docs or persist_docs consistently. I'm in favor of raw_persist_docs here, since that's what you pass from create_table_as.

resolved, I stuck with persist_docs because that what was used before my PR

CHANGELOG.md

beckjake · 2020-04-28T19:14:23Z

Discussion: where should this happen?

Let me chime in with my $0.02 here, since I'm likely going to be doing the postgres/redshift one. When I first thought about this, I was firmly in favor of this happening in a special macro called by materializations, similar to how hooks work, and letting BigQuery be the weird special case.

But then I realized that create_X_as macros have kind of misleading names. Those macros are always going to act with the current model's config settings, regardless of the relation name you create. If you call create_table_as in bigquery with a Relation that isn't the model being materialized, your created relation will have docs corresponding to the model being materialized, even if it makes no sense! Same goes for labels. In the interest of consistency, maybe it makes a lot of sense to put this stuff into the create_X_as macros.

It also makes me think maybe we should rename these macros, but I would really prefer not to do that, plugins have just seen so much churn lately.

jtcohen6 · 2020-04-28T19:43:53Z

Those macros are always going to act with the current model's config settings, regardless of the relation name you create.

That's a really good point. Insofar as create_x_as macros are pulling directly from the model config, they are more than just boilerplate DDL wrappers.

I recall having discussed the desire for a future state in which only materializations pull directly from model config, and pass those values around to macros as keyword arguments. I don't know if that's still something we want, or if we want it enough to make meaningful steps in that direction—or, in this case, to not make meaningful steps away from that direction.

beckjake · 2020-04-28T20:17:59Z

I recall having discussed the desire for a future state in which only materializations pull directly from model config

You'd have to pass something suspiciously similar to the full generic model config to a lot of these things if you wanted to handle per-adapter differences nicely while allowing third parties to create their own custom adapter-generic materializations that pass along adapter-specific configs cleanly. I suppose you could have a generic adapter_macro that takes arbitrary keyword arguments and/or a model, returns some sort of object that you should then pass to create_table_as. So materializations would do:

{# get_create_table_opts calls return adapter_macro('get_create_table_opts', model=model, **kwargs) #}
{% set create_table_opts = get_create_table_opts(model=model) %} 
{% do run_query(create_table_as(tmp_relation, sql, create_table_opts) %}

Or, instead of a macro maybe you'd just pass in the global config object and give materialization authors an easy way to make their own "config"s to pass to create_table_as?

I'm not sure what that means for this PR, but I'm inclined to accept its answer of doing what bigquery already does and assuming that create_X_as is only being run for the currently-being-materialized model. I think revisiting that assumption is also totally on the table, we should just do it somewhere else (and before 1.0).

sfc-gh-aseitz · 2020-04-28T22:48:35Z

@jtcohen6 I resolved the rebase issue from above, so the PR should be much cleaner
@beckjake happy to make further modifications if you think necessary or if it is worth waiting for the future

beckjake · 2020-04-29T16:36:55Z

core/dbt/include/global_project/macros/adapters/common.sql

+  {{ return(adapter_macro('alter_column_comment', relation, column_dict)) }}
+{% endmacro %}
+
+{% macro default__alter_column_comment(relation, column_name, new_column_type) -%}


I think this should be relation, column_dict, not relation, column_name, new_column_type!

Good catch, this is fixed now

drewbanin · 2020-04-30T22:35:10Z

@snowflakeseitz FYI these tests are failing! I can take a deeper look and follow up with some more info about what's going wrong here

beckjake · 2020-05-01T14:02:31Z

@snowflakeseitz I had the same issue you did when I was working on ~~#2333~~ #2378! What I found was that the incremental model uses this, which doesn't have a type. My solution was to put {% set relation = relation.incorporate(type='view') %} in the create_view_as macro implementation before I called the comment-setting methods, and {% set relation = relation.incorporate(type='table') %} in the create_table_as macro.

Here's the relevant commit

sfc-gh-aseitz · 2020-05-04T03:13:40Z

@beckjake good catch, I just committed your suggestion!

beckjake · 2020-05-04T20:48:35Z

core/dbt/include/global_project/macros/adapters/common.sql

+
+
+{% macro alter_column_comment(relation, column_dict) -%}
+  {{ return(adapter_macro('alter_column_comment', relation, column_dict)) }}
+{% endmacro %}
+
+{% macro default__alter_column_comment(relation, column_dict) -%}
+  {{ exceptions.raise_not_implemented(
+    'alter_column_comment macro not implemented for adapter '+adapter.type()) }}
+{% endmacro %}
+
+{% macro alter_relation_comment(relation, relation_comment) -%}
+  {{ return(adapter_macro('alter_relation_comment', relation, relation_comment)) }}
+{% endmacro %}
+
+{% macro default__alter_relation_comment(relation, relation_comment) -%}
+  {{ exceptions.raise_not_implemented(
+    'alter_relation_comment macro not implemented for adapter '+adapter.type()) }}
+{% endmacro %}
+
+
+
+


@snowflakeseitz Can you remove these, or the ones from the bottom of the file and the comment on the other ones? I merged the redshift/postgres PR and I must have messed it up, git didn't detect the duplicate changes as I expected it to.

should I just rebase to your changes?

Sure! I think you'll have to remove them during the rebase anyway.

beckjake · 2020-05-04T20:49:07Z

core/dbt/include/global_project/macros/etc/get_relation_comment.sql

+    {{ return(none) }}
+  {% endif %}
+
+{% endmacro %}


The same as my above comment applies here - I think these two are the only overlap!

jtcohen6

Thanks for the discussion above @beckjake. I'm on board with shipping the implementation as-is for 0.17.0. Given your finding in #2378 on the redshift/pg version, I think we should reassess where these comment statements best belong—within create_table_as or as implicit hooks—as part of revisiting materializations more broadly for 0.18.0.

sfc-gh-aseitz · 2020-05-05T17:22:31Z

@drewbanin this has been rebased to @beckjake most recent changes! Hopefully, we can get this merged soon!

beckjake · 2020-05-05T17:38:36Z

I'll kick off the test suite now, assuming it passes I'll merge it!

cla-bot bot added the cla:yes label Apr 14, 2020

drewbanin reviewed Apr 20, 2020

View reviewed changes

jtcohen6 reviewed Apr 28, 2020

View reviewed changes

sfc-gh-aseitz force-pushed the column_comments branch from 51fa43e to caef584 Compare April 28, 2020 22:43

beckjake reviewed Apr 29, 2020

View reviewed changes

beckjake mentioned this pull request Apr 30, 2020

Column and table comments for postgres/redshift (#2333) #2378

Merged

4 tasks

beckjake reviewed May 4, 2020

View reviewed changes

jtcohen6 approved these changes May 4, 2020

View reviewed changes

sfc-gh-aseitz added 5 commits May 4, 2020 14:42

rebasing

b98a4d8

basics to get column comments working

eb3ed57

adding in Drews feedback

8de3235

fixing spacing

ea82d8f

adding in support for views

d9862af

sfc-gh-aseitz added 10 commits May 4, 2020 14:48

adding persist docs tests

74df1a1

basics to get column comments working

78110e1

adding in Drews feedback

06ffb6d

fixing spacing

f843e65

adding in support for views

1448d0b

fixing rebase

0fdff04

fixing rebase error

5cabafc

fixing default

f58bc49

fixing failing test on incremental

bd2d1f7

adding in changelog

ffaaacc

sfc-gh-aseitz force-pushed the column_comments branch from d47fb6f to ffaaacc Compare May 5, 2020 17:05

removing duplicates in common

b774702

beckjake merged commit 7c916e9 into dbt-labs:dev/octavius-catto May 5, 2020

drewbanin changed the title ~~Column comments~~ Snowflake column comments May 5, 2020

drewbanin mentioned this pull request May 5, 2020

Support persist docs on Snowflake: Relations and columns #2334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snowflake column comments #2321

Snowflake column comments #2321

sfc-gh-aseitz commented Apr 14, 2020 •

edited

Loading

cla-bot bot commented Apr 14, 2020

beckjake commented Apr 14, 2020

cla-bot bot commented Apr 14, 2020

beckjake commented Apr 14, 2020

sfc-gh-aseitz commented Apr 14, 2020

sfc-gh-aseitz commented Apr 14, 2020

drewbanin left a comment

drewbanin Apr 20, 2020

drewbanin Apr 20, 2020

drewbanin commented Apr 27, 2020

sfc-gh-aseitz commented Apr 27, 2020

drewbanin commented Apr 27, 2020

jtcohen6 left a comment

jtcohen6 Apr 28, 2020

sfc-gh-aseitz Apr 28, 2020

beckjake commented Apr 28, 2020

jtcohen6 commented Apr 28, 2020

beckjake commented Apr 28, 2020

sfc-gh-aseitz commented Apr 28, 2020

beckjake Apr 29, 2020

sfc-gh-aseitz Apr 29, 2020

drewbanin commented Apr 30, 2020

beckjake commented May 1, 2020 •

edited

Loading

sfc-gh-aseitz commented May 4, 2020 •

edited

Loading

beckjake May 4, 2020

sfc-gh-aseitz May 4, 2020

beckjake May 4, 2020

beckjake May 4, 2020

jtcohen6 left a comment

sfc-gh-aseitz commented May 5, 2020

beckjake commented May 5, 2020

Snowflake column comments #2321

Snowflake column comments #2321

Conversation

sfc-gh-aseitz commented Apr 14, 2020 • edited Loading

Description

Checklist

cla-bot bot commented Apr 14, 2020

beckjake commented Apr 14, 2020

cla-bot bot commented Apr 14, 2020

beckjake commented Apr 14, 2020

sfc-gh-aseitz commented Apr 14, 2020

sfc-gh-aseitz commented Apr 14, 2020

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

drewbanin commented Apr 27, 2020

sfc-gh-aseitz commented Apr 27, 2020

drewbanin commented Apr 27, 2020

jtcohen6 left a comment

Choose a reason for hiding this comment

Discussion: where should this happen?

Future work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beckjake commented Apr 28, 2020

jtcohen6 commented Apr 28, 2020

beckjake commented Apr 28, 2020

sfc-gh-aseitz commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewbanin commented Apr 30, 2020

beckjake commented May 1, 2020 • edited Loading

sfc-gh-aseitz commented May 4, 2020 • edited Loading

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

jtcohen6 left a comment

Choose a reason for hiding this comment

sfc-gh-aseitz commented May 5, 2020

beckjake commented May 5, 2020

sfc-gh-aseitz commented Apr 14, 2020 •

edited

Loading

beckjake commented May 1, 2020 •

edited

Loading

sfc-gh-aseitz commented May 4, 2020 •

edited

Loading