materialized_view with persist_docs does not *update* existing comments

[vergenzt]

Don't have time to be super detailed right now (which is why I didn't do a bug report yet) but I wanted to start the discussion. I have models with +materialized: materialized_view and +persist_docs: {relation: true, columns: true}. When I made a change to (only) the descriptions, the SQL to alter the comment did not get issued during dbt build. It seems dbt-databricks currently only adds the comments on creation / full refresh.

I may be able to work around this by just manually calling persist_docs(this, model, true, true) in a post hook -- I'm gonna try that and see how it goes.

[vergenzt]

Update: This workaround seems to work for now:

# in dbt_project.yml
models:
  ...
  +post-hook:
  # force existing column docs to update (https://github.com/databricks/dbt-databricks/discussions/1275)
  - '{{ persist_docs(this.incorporate(type="table"), model) }}'  

Note that the .incorporate(type="table") call on this was necessary because the DatabricksRelation's type field is used in the persist_docs implementation but is only set in a local target_relation variable at the top of the materialization, not in anything which is accessible by the post-hook. This meant that a simple {{ persist_docs(this, model) }} post-hook resulted in a runtime error:

  'None' has no attribute 'render'
  
  > in macro databricks__persist_docs (macros/adapters/persist_docs.sql)
  > called by macro persist_docs (macros/adapters/persist_docs.sql)
  > called by macro run_hooks (macros/materializations/hooks.sql)
  > called by macro materialized_view_execute_build_sql (macros/materializations/materialized_view.sql)
  > called by macro materialization_materialized_view_databricks (macros/materializations/materialized_view.sql)

When I then initially (accurately) passed this.incorporate(type=this.MaterializedView) as the first argument instead of this, I then got a SQL syntax error:

  [PARSE_SYNTAX_ERROR] Syntax error at or near 'MATERIALIZED'. SQLSTATE: 42601 (line 4, pos 11)
  
  == SQL ==
  /* {"app": "dbt", "dbt_version": "1.10.15", "dbt_databricks_version": "1.11.0", "databricks_sql_connector_version": "4.1.3", "profile_name": "...", "target_name": "...", "node_id": "..."} */
  
  COMMENT ON MATERIALIZED VIEW `[redacted]`.`[redacted]`.`[redacted]` IS '[redacted]'
  -----------^^^

So then I settled on incorporating type="table" instead of this.MaterializedView and it worked.

This now leads me to realize though that the current implementation of alter_relation_comment_sql used by persist_docs has a bug:

{% macro alter_relation_comment_sql(relation, description) %}
COMMENT ON {{ relation.type.render().upper() }} {{ relation.render() }} IS '{{ description | replace("'", "\\'") }}'
{% endmacro %}

should instead be:

 {% macro alter_relation_comment_sql(relation, description) %}
-COMMENT ON {{ relation.type.render().upper() }} {{ relation.render() }} IS '{{ description | replace("'", "\\'") }}'
+COMMENT ON TABLE {{ relation.render() }} IS '{{ description | replace("'", "\\'") }}'
 {% endmacro %}

Explanation: Databricks' COMMENT ON ... syntax does not change phrasing based on relation type; to update a top-level relation comment, the correct syntax is always COMMENT ON TABLE <relation_name> IS ..., even if the relation is e.g. a materialized view.

Additionally, materialized_view_execute_build_sql should probably be updated to call {%- do persist_docs(target_relation, model) %}...

 {% macro materialized_view_execute_build_sql(build_sql, existing_relation, target_relation, post_hooks) %}
     ...
     {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %}
     {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}
+
+    {%- do persist_docs(target_relation, model) %}
 
     {{ run_hooks(post_hooks, inside_transaction=True) }}
 
 {% endmacro %}

... same as the other materializations do ...

... to avoid having to apply this workaround in the first place. 🙃