Merge pull request #2378 from fishtown-analytics/feature/pg-rs-persis…

…t-docs

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

CHANGELOG.md

@@ -22,6 +22,7 @@
 - Sources (and therefore freshness tests) can be enabled and disabled via dbt_project.yml ([#2283](https:/fishtown-analytics/dbt/issues/2283), [#2312](https:/fishtown-analytics/dbt/pull/2312), [#2357](https:/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:/fishtown-analytics/dbt/issues/2269), [#2357](https:/fishtown-analytics/dbt/pull/2357))
 - Sources from dependencies can be overridden in schema.yml files ([#2287](https:/fishtown-analytics/dbt/issues/2287), [#2357](https:/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:/fishtown-analytics/dbt/issues/2333), [#2378](https:/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:/fishtown-analytics/dbt/issues/2110), [#2184](https:/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 %}
+