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

CHANGELOG.md

@@ -21,6 +21,7 @@
 - Sources (and therefore freshness tests) can be enabled and disabled via dbt_project.yml ([#2283](https://github.com/fishtown-analytics/dbt/issues/2283), [#2312](https://github.com/fishtown-analytics/dbt/pull/2312), [#2357](https://github.com/fishtown-analytics/dbt/pull/2357))
 - schema.yml files are now fully rendered in a context that is aware of vars declared in from dbt_project.yml files ([#2269](https://github.com/fishtown-analytics/dbt/issues/2269), [#2357](https://github.com/fishtown-analytics/dbt/pull/2357))
 - 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))
 
 ### 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))

core/dbt/clients/_jinja_blocks.py

@@ -92,6 +92,18 @@ def __init__(self, data):
         self._parenthesis_stack = []
         self.pos = 0
 
+    def linepos(self, end=None) -> str:
+        """Given an absolute position in the input data, return a pair of
+        line number + relative position to the start of the line.
+        """
+        end_val: int = self.pos if end is None else end
+        data = self.data[:end_val]
+        # if not found, rfind returns -1, and -1+1=0, which is perfect!
+        last_line_start = data.rfind('\n') + 1
+        # it's easy to forget this, but line numbers are 1-indexed
+        line_number = data.count('\n') + 1
+        return f'{line_number}:{end_val - last_line_start}'
+
     def advance(self, new_position):
         self.pos = new_position
 
@@ -320,20 +332,28 @@ def find_blocks(self, allowed_blocks=None, collect_raw_data=True):
                     dbt.exceptions.raise_compiler_error((
                         'Got an unexpected control flow end tag, got {} but '
                         'never saw a preceeding {} (@ {})'
-                    ).format(tag.block_type_name, expected, tag.start))
+                    ).format(
+                        tag.block_type_name,
+                        expected,
+                        self.tag_parser.linepos(tag.start)
+                    ))
                 expected = _CONTROL_FLOW_TAGS[found]
                 if expected != tag.block_type_name:
                     dbt.exceptions.raise_compiler_error((
                         'Got an unexpected control flow end tag, got {} but '
                         'expected {} next (@ {})'
-                    ).format(tag.block_type_name, expected, tag.start))
+                    ).format(
+                        tag.block_type_name,
+                        expected,
+                        self.tag_parser.linepos(tag.start)
+                    ))
 
             if tag.block_type_name in allowed_blocks:
                 if self.stack:
                     dbt.exceptions.raise_compiler_error((
                         'Got a block definition inside control flow at {}. '
                         'All dbt block definitions must be at the top level'
-                    ).format(tag.start))
+                    ).format(self.tag_parser.linepos(tag.start)))
                 if self.current is not None:
                     dbt.exceptions.raise_compiler_error(
                         duplicate_tags.format(outer=self.current, inner=tag)

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

@@ -78,6 +78,7 @@
   as (
     {{ sql }}
   );
+
 {% endmacro %}
 
 {% macro create_view_as(relation, sql) -%}
@@ -280,3 +281,41 @@
 {% macro set_sql_header(config) -%}
   {{ config.set('sql_header', caller()) }}
 {%- endmacro %}
+
+
+{%- macro set_relation_comment(relation) -%}
+  {%- set raw_persist_docs = config.get('persist_docs', {}) -%}
+  {%- set comment = get_relation_comment(raw_persist_docs, model) -%}
+  {%- if comment is not none -%}
+    {{ alter_relation_comment(relation, comment) }}
+  {%- endif -%}
+{%- endmacro -%}
+
+
+{%- macro set_column_comments(relation) -%}
+  {%- set raw_persist_docs = config.get('persist_docs', {}) -%}
+  {%- set column_dict = get_relation_column_comments(raw_persist_docs, model) -%}
+  {%- if column_dict is not none -%}
+    {{ alter_column_comment(relation, column_dict) }}
+  {%- 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 %}

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

@@ -1,16 +1,26 @@
-{% macro table_options() %}
-  {%- set raw_persist_docs = config.get('persist_docs', {}) -%}
-
-{%- endmacro -%}
-
 {% 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) }}
   {% endif %}
 
   {% if persist_docs.get('relation', false) %}
-    {{ return((model.description | tojson)[1:-1]) }}
+    {{ return(model.description) }}
+  {%- else -%}
+    {{ return(none) }}
+  {% endif %}
+
+{% endmacro %}
+
+
+{# copy+pasted from the snowflake PR - delete this on merge #}
+{% macro get_relation_column_comments(persist_docs, model) %}
+  {%- if persist_docs is not mapping -%}
+    {{ exceptions.raise_compiler_error("Invalid value provided for 'persist_docs'. Expected dict but got value: " ~ persist_docs) }}
+  {% endif %}
+
+  {% if persist_docs.get('columns', false) and model.columns|length != 0 %}
+    {{ return(model.columns) }}
   {%- else -%}
     {{ return(none) }}
   {% endif %}

core/dbt/include/global_project/macros/materializations/incremental/incremental.sql

@@ -4,7 +4,7 @@
   {% set unique_key = config.get('unique_key') %}
   {% set full_refresh_mode = flags.FULL_REFRESH %}
 
-  {% set target_relation = this %}
+  {% set target_relation = this.incorporate(type='table') %}
   {% set existing_relation = load_relation(this) %}
   {% set tmp_relation = make_temp_relation(this) %}
 

core/dbt/include/global_project/macros/materializations/snapshot/snapshot.sql

@@ -210,9 +210,7 @@
   {% if not target_relation_exists %}
 
       {% set build_sql = build_snapshot_table(strategy, model['injected_sql']) %}
-      {% call statement('main') -%}
-          {{ create_table_as(False, target_relation, build_sql) }}
-      {% endcall %}
+      {% set final_sql = create_table_as(False, target_relation, build_sql) %}
 
   {% else %}
 
@@ -245,17 +243,19 @@
         {% do quoted_source_columns.append(adapter.quote(column.name)) %}
       {% endfor %}
 
-      {% call statement('main') %}
-          {{ snapshot_merge_sql(
-                target = target_relation,
-                source = staging_table,
-                insert_cols = quoted_source_columns
-             )
-          }}
-      {% endcall %}
+      {% set final_sql = snapshot_merge_sql(
+            target = target_relation,
+            source = staging_table,
+            insert_cols = quoted_source_columns
+         )
+      %}
 
   {% endif %}
 
+  {% call statement('main') %}
+      {{ final_sql }}
+  {% endcall %}
+
   {{ run_hooks(post_hooks, inside_transaction=True) }}
 
   {{ adapter.commit() }}

core/dbt/parser/macros.py

@@ -48,22 +48,26 @@ def parse_macro(
     def parse_unparsed_macros(
         self, base_node: UnparsedMacro
     ) -> Iterable[ParsedMacro]:
-        blocks: List[jinja.BlockTag] = [
-            t for t in
-            jinja.extract_toplevel_blocks(
-                base_node.raw_sql,
-                allowed_blocks={'macro', 'materialization'},
-                collect_raw_data=False,
-            )
-            if isinstance(t, jinja.BlockTag)
-        ]
+        try:
+            blocks: List[jinja.BlockTag] = [
+                t for t in
+                jinja.extract_toplevel_blocks(
+                    base_node.raw_sql,
+                    allowed_blocks={'macro', 'materialization'},
+                    collect_raw_data=False,
+                )
+                if isinstance(t, jinja.BlockTag)
+            ]
+        except CompilationException as exc:
+            exc.add_node(base_node)
+            raise
 
         for block in blocks:
             try:
                 ast = jinja.parse(block.full_block)
             except CompilationException as e:
                 e.add_node(base_node)
-                raise e
+                raise
 
             macro_nodes = list(ast.find_all(jinja2.nodes.Macro))
 

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

@@ -27,12 +27,21 @@
 
 {%- endmacro -%}
 
+
+{%- macro bigquery_escape_comment(comment) -%}
+  {%- if comment is not string -%}
+    {%- do exceptions.raise_compiler_exception('cannot escape a non-string: ' ~ comment) -%}
+  {%- endif -%}
+  {%- do return((comment | tojson)[1:-1]) -%}
+{%- endmacro -%}
+
+
 {% macro bigquery_table_options(persist_docs, temporary, kms_key_name, labels) %}
   {% set opts = {} -%}
 
   {%- set description = get_relation_comment(persist_docs, model) -%}
   {%- if description is not none -%}
-    {%- do opts.update({'description': "'" ~ description ~ "'"}) -%}
+    {%- do opts.update({'description': "'" ~ bigquery_escape_comment(description) ~ "'"}) -%}
   {%- endif -%}
   {%- if temporary -%}
     {% do opts.update({'expiration_timestamp': 'TIMESTAMP_ADD(CURRENT_TIMESTAMP(), INTERVAL 12 hour)'}) %}

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

@@ -12,8 +12,20 @@
   as (
     {{ sql }}
   );
+
+  {% set relation = relation.incorporate(type='table') %}
+  {{ set_relation_comment(relation) }}
+  {{ set_column_comments(relation) }}
 {%- endmacro %}
 
+
+{% macro postgres__create_view_as(relation, sql) %}
+  {{ default__create_view_as(relation, sql) }}
+  {%- set relation = relation.incorporate(type='view') -%}
+  {{ set_relation_comment(relation) }}
+  {{ set_column_comments(relation) }}
+{% endmacro %}
+
 {% macro postgres__create_schema(database_name, schema_name) -%}
   {% if database_name -%}
     {{ adapter.verify_database(database_name) }}
@@ -127,3 +139,34 @@
                                   })) -%}
 {% endmacro %}
 
+
+{#
+  By using dollar-quoting like this, users can embed anything they want into their comments
+  (including nested dollar-quoting), as long as they don't use this exact dollar-quoting
+  label. It would be nice to just pick a new one but eventually you do have to give up.
+#}
+{% macro postgres_escape_comment(comment) -%}
+  {% if comment is not string %}
+    {% do exceptions.raise_compiler_exception('cannot escape a non-string: ' ~ comment) %}
+  {% endif %}
+  {%- set magic = '$dbt_comment_literal_block$' -%}
+  {%- if magic in comment -%}
+    {%- do exceptions.raise_compiler_exception('The string ' ~ magic ~ ' is not allowed in comments.') -%}
+  {%- endif -%}
+  {{ magic }}{{ comment }}{{ magic }}
+{%- endmacro %}
+
+
+{% macro postgres__alter_relation_comment(relation, comment) %}
+  {% set escaped_comment = postgres_escape_comment(comment) %}
+  comment on {{ relation.type }} {{ relation }} is {{ escaped_comment }};
+{% endmacro %}
+
+
+{% macro postgres__alter_column_comment(relation, column_dict) %}
+  {% for column_name in column_dict %}
+    {% set comment = column_dict[column_name]['description'] %}
+    {% set escaped_comment = postgres_escape_comment(comment) %}
+    comment on column {{ relation }}.{{ column_name }} is {{ escaped_comment }};
+  {% endfor %}
+{% endmacro %}

plugins/postgres/dbt/include/postgres/macros/catalog.sql

@@ -17,16 +17,18 @@
             when 'v' then 'VIEW'
             else 'BASE TABLE'
         end as table_type,
-        null::text as table_comment,
+        tbl_desc.description as table_comment,
         col.attname as column_name,
         col.attnum as column_index,
         pg_catalog.format_type(col.atttypid, col.atttypmod) as column_type,
-        null::text as column_comment,
+        col_desc.description as column_comment,
         pg_get_userbyid(tbl.relowner) as table_owner
 
     from pg_catalog.pg_namespace sch
     join pg_catalog.pg_class tbl on tbl.relnamespace = sch.oid
     join pg_catalog.pg_attribute col on col.attrelid = tbl.oid
+    left outer join pg_catalog.pg_description tbl_desc on (tbl_desc.objoid = tbl.oid and tbl_desc.objsubid = 0)
+    left outer join pg_catalog.pg_description col_desc on (col_desc.objoid = tbl.oid and col_desc.objsubid = col.attnum)
 
     where (
         {%- for schema in schemas -%}

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

@@ -49,19 +49,34 @@
   as (
     {{ sql }}
   );
+
+  {% set relation = relation.incorporate(type='table') %}
+  {{ set_relation_comment(relation) }}
+  {{ set_column_comments(relation) }}
 {%- endmacro %}
 
 
 {% macro redshift__create_view_as(relation, sql) -%}
+  {%- set binding = config.get('bind', default=True) -%}
 
-  {% set bind_qualifier = '' if config.get('bind', default=True) else 'with no schema binding' %}
+  {% set bind_qualifier = '' if binding else 'with no schema binding' %}
   {%- set sql_header = config.get('sql_header', none) -%}
 
   {{ sql_header if sql_header is not none }}
 
   create view {{ relation }} as (
     {{ sql }}
   ) {{ bind_qualifier }};
+
+  {#
+    For late-binding views, it's possible to set comments on the view (though they don't seem to end up anywhere).
+    Unfortunately, setting comments on columns just results in an error.
+  #}
+  {% set relation = relation.incorporate(type='view') %}
+  {{ set_relation_comment(relation) }}
+  {% if binding %}
+    {{ set_column_comments(relation) }}
+  {% endif %}
 {% endmacro %}
 
 
@@ -188,3 +203,14 @@
 {% macro redshift__make_temp_relation(base_relation, suffix) %}
     {% do return(postgres__make_temp_relation(base_relation, suffix)) %}
 {% endmacro %}
+
+
+{% macro redshift__alter_relation_comment(relation, comment) %}
+  {% do return(postgres__alter_relation_comment(relation, comment)) %}
+{% endmacro %}
+
+
+{% macro redshift__alter_column_comment(relation, column_dict) %}
+  {% do return(postgres__alter_column_comment(relation, column_dict)) %}
+{% endmacro %}
+