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.

Sign up for GitHub

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

Jump to bottom

Snowflake column comments #2321

Merged
merged 16 commits into from
May 5, 2020
Merged

Snowflake column comments #2321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b98a4d8
rebasing
sfc-gh-aseitz May 4, 2020
eb3ed57
basics to get column comments working
sfc-gh-aseitz Apr 13, 2020
8de3235
adding in Drews feedback
sfc-gh-aseitz Apr 14, 2020
ea82d8f
fixing spacing
sfc-gh-aseitz Apr 14, 2020
d9862af
adding in support for views
sfc-gh-aseitz Apr 27, 2020
74df1a1
adding persist docs tests
sfc-gh-aseitz Apr 28, 2020
78110e1
basics to get column comments working
sfc-gh-aseitz Apr 13, 2020
06ffb6d
adding in Drews feedback
sfc-gh-aseitz Apr 14, 2020
f843e65
fixing spacing
sfc-gh-aseitz Apr 14, 2020
1448d0b
adding in support for views
sfc-gh-aseitz Apr 27, 2020
0fdff04
fixing rebase
sfc-gh-aseitz May 4, 2020
5cabafc
fixing rebase error
sfc-gh-aseitz Apr 28, 2020
f58bc49
fixing default
sfc-gh-aseitz Apr 29, 2020
bd2d1f7
fixing failing test on incremental
sfc-gh-aseitz May 4, 2020
ffaaacc
adding in changelog
sfc-gh-aseitz May 5, 2020
b774702
removing duplicates in common
sfc-gh-aseitz May 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
- Sources from dependencies can be overridden in schema.yml files ([#2287](https://github.com/fishtown-analytics/dbt/issues/2287), [#2357](https://github.com/fishtown-analytics/dbt/pull/2357))
- Implement persist_docs for both `relation` and `comments` on postgres and redshift, and extract them when getting the catalog. ([#2333](https://github.com/fishtown-analytics/dbt/issues/2333), [#2378](https://github.com/fishtown-analytics/dbt/pull/2378))
- Added a filter named `as_text` to the native environment rendering code that allows users to mark a value as always being a string ([#2384](https://github.com/fishtown-analytics/dbt/issues/2384), [#2395](https://github.com/fishtown-analytics/dbt/pull/2395))
- Relation comments supported for Snowflake tables and views. Column comments supported for tables. ([#1722](https://github.com/fishtown-analytics/dbt/issues/1722), [#2321](https://github.com/fishtown-analytics/dbt/pull/2321))


### Fixes
- When a jinja value is undefined, give a helpful error instead of failing with cryptic "cannot pickle ParserMacroCapture" errors ([#2110](https://github.com/fishtown-analytics/dbt/issues/2110), [#2184](https://github.com/fishtown-analytics/dbt/pull/2184))
Expand Down Expand Up @@ -63,6 +65,7 @@ Contributors:
- [@Fokko](https://github.com/Fokko) [#2361](https://github.com/fishtown-analytics/dbt/pull/2361)
- [@franloza](https://github.com/franloza) [#2349](https://github.com/fishtown-analytics/dbt/pull/2349)
- [@sethwoodworth](https://github.com/sethwoodworth) [#2389](https://github.com/fishtown-analytics/dbt/pull/2389)
- [@snowflakeseitz](https://github.com/snowflakeseitz) [#2321](https://github.com/fishtown-analytics/dbt/pull/2321)

## dbt 0.16.1 (April 14, 2020)

Expand Down Expand Up @@ -171,7 +174,7 @@ Contributors:

### Features
- Add column-level quoting control for tests ([#2106](https://github.com/fishtown-analytics/dbt/issues/2106), [#2047](https://github.com/fishtown-analytics/dbt/pull/2047))
- Add the macros every node uses to its `depends_on.macros` list ([#2082](https://github.com/fishtown-analytics/dbt/issues/2082), [#2103](https://github.com/fishtown-analytics/dbt/pull/2103))
- Add the macros every node uses to its `depends_on.macros` list ([#2082](https://github.com/fishtown-analytics/dbt/issues/2082), [#2103](https://github.com/fishtown-analytics/dbt/pull/2103))
- Add `arguments` field to macros ([#2081](https://github.com/fishtown-analytics/dbt/issues/2081), [#2083](https://github.com/fishtown-analytics/dbt/issues/2083), [#2096](https://github.com/fishtown-analytics/dbt/pull/2096))
- Batch the anonymous usage statistics requests to improve performance ([#2008](https://github.com/fishtown-analytics/dbt/issues/2008), [#2089](https://github.com/fishtown-analytics/dbt/pull/2089))
- Add documentation for macros/analyses ([#1041](https://github.com/fishtown-analytics/dbt/issues/1041), [#2068](https://github.com/fishtown-analytics/dbt/pull/2068))
Expand Down Expand Up @@ -2221,4 +2224,4 @@ profile: "side-project"

model-defaults:
....
```
```
42 changes: 23 additions & 19 deletions core/dbt/include/global_project/macros/adapters/common.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,29 @@
{{ return(adapter_macro('alter_column_type', relation, column_name, new_column_type)) }}
{% endmacro %}



{% 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 %}




Comment on lines +134 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I just rebase to your changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{% macro default__alter_column_type(relation, column_name, new_column_type) -%}
{#
1. Create a new column (w/ temp name and correct type)
Expand Down Expand Up @@ -300,22 +323,3 @@
{%- endif -%}
{%- endmacro -%}


{# copy+pasted from the snowflake PR - delete these 4 on merge #}
{% 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 %}
5 changes: 3 additions & 2 deletions core/dbt/include/global_project/macros/etc/get_relation_comment.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{% macro get_relation_comment(persist_docs, model) %}

{%- if persist_docs is not mapping -%}
{{ exceptions.raise_compiler_error("Invalid value provided for 'persist_docs'. Expected dict but got value: " ~ raw_persist_docs) }}
{{ exceptions.raise_compiler_error("Invalid value provided for 'persist_docs'. Expected dict but got value: " ~ persist_docs) }}
{% endif %}

{% if persist_docs.get('relation', false) %}
Expand All @@ -13,8 +13,8 @@
{% endmacro %}


{# copy+pasted from the snowflake PR - delete this on merge #}
{% macro get_relation_column_comments(persist_docs, model) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


{%- if persist_docs is not mapping -%}
{{ exceptions.raise_compiler_error("Invalid value provided for 'persist_docs'. Expected dict but got value: " ~ persist_docs) }}
{% endif %}
Expand All @@ -26,3 +26,4 @@
{% endif %}

{% endmacro %}
Copy link
Contributor

Choose a reason for hiding this comment

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

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


38 changes: 38 additions & 0 deletions plugins/snowflake/dbt/include/snowflake/macros/adapters.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
{%- set cluster_by_keys = config.get('cluster_by', default=none) -%}
{%- set enable_automatic_clustering = config.get('automatic_clustering', default=false) -%}
{%- set copy_grants = config.get('copy_grants', default=false) -%}
{%- 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) -%}

{%- if cluster_by_keys is not none and cluster_by_keys is string -%}
{%- set cluster_by_keys = [cluster_by_keys] -%}
{%- endif -%}
Expand Down Expand Up @@ -35,20 +39,38 @@
{% if enable_automatic_clustering and cluster_by_string is not none and not temporary -%}
alter table {{relation}} resume recluster;
{%- endif -%}
-- add in comments

{% set relation = relation.incorporate(type='table') %}
{% if relation_comment is not none -%}
{{ alter_relation_comment(relation, relation_comment) }}
{%- endif -%}

{% if column_comment is not none -%}
{{ alter_column_comment(relation, column_comment) }}
{%- endif -%}

{% 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) -%}

{{ sql_header if sql_header is not none }}
create or replace {% if secure -%}
secure
{%- endif %} view {{ relation }} {% if copy_grants -%} copy grants {%- endif %} as (
{{ sql }}
);

{%- set relation = relation.incorporate(type='view') -%}
{% if relation_comment is not none -%}
{{ alter_relation_comment(relation, relation_comment) }}
{%- endif -%}

{% endmacro %}

{% macro snowflake__get_columns_in_relation(relation) -%}
Expand Down Expand Up @@ -150,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 {{ relation.type }} {{ relation }} IS $${{ relation_comment | replace('$', '[$]') }}$$;
{% endmacro %}


{% macro snowflake__alter_column_comment(relation, column_dict) -%}
alter {{ relation.type }} {{ relation }} alter
{% for column_name in column_dict %}
{{ column_name }} COMMENT $${{ column_dict[column_name]['description'] | replace('$', '[$]') }}$$ {{ ',' if not loop.last else ';' }}
{% endfor %}
{% endmacro %}




8 changes: 8 additions & 0 deletions test/integration/059_persist_docs_test/sf_models/incremental_test.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{{
config ({
"materialized" : 'incremental',
"persist_docs" : { "relation": true, "columns": true, "schema": true }
})
}}

select 1 as column1
21 changes: 21 additions & 0 deletions test/integration/059_persist_docs_test/sf_models/schema.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
models:
- name: view_test
description: >
View - "money" aka $$$$$
columns:
- name: column1
description: test 1 - abcd "" $$$ ''

- name: incremental_test
description: >
Incremental - "money" aka $$$$$
columns:
- name: column1
description: test 1 - abcd "" $$$ ''

- name: table_test
description: >
Table - "money" aka $$$$$
columns:
- name: column1
description: test 1 - abcd "" $$$ ''
8 changes: 8 additions & 0 deletions test/integration/059_persist_docs_test/sf_models/table_test.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{{
config ({
"materialized" : 'table',
"persist_docs" : { "relation": true, "columns": true, "schema": true }
})
}}

select 1 as column1
7 changes: 7 additions & 0 deletions test/integration/059_persist_docs_test/sf_models/view_test.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{{
config ({
"persist_docs" : { "relation": true, "columns": true, "schema": true }
})
}}

select 1 as column1