-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
Support for persist_docs #77
Conversation
38ed99d
to
8793b42
Compare
{% macro trino__alter_relation_comment(relation, relation_comment) -%} | ||
comment on {{ relation.type }} {{ relation }} IS '{{ relation_comment | replace("'", "''") }}'; | ||
{% endmacro %} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove one line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two new lines are present after each macro in this file.
|
||
{% macro trino__alter_column_comment(relation, column_dict) %} | ||
{% set existing_columns = adapter.get_columns_in_relation(relation) | map(attribute="name") | list %} | ||
{% for column_name in column_dict if (column_name in existing_columns) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we supposed to remove column comments as well when they are not set anymore?
same question for tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are deleted if there are no set anymore. It's done by setting an empty comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of comments is done by the command
COMMENT ON TABLE/COLUMN ... IS NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the code adjusted accordingly to the comment.
"name": "persist_docs_tests", | ||
"models": { | ||
"+persist_docs": { | ||
"relation": True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we basically test whether dbt is persisting docs for relations and columns, but can we check whether the comments were set as expected on the underlying database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a valid comment. This test should be handled by macro https://github.com/starburstdata/dbt-trino/blob/master/dbt/include/trino/macros/catalog.sql which creates a file target/catalog.json
generated by dbt docs generate
command.
In the get_catalog
macro table and column comments are returned as null
. Information schema tables in Trino miss comment columns. I will create a follow-up ticket in Trino for it.
Docs are generated by a file located in target/manifest.json
and all comments are present in dbt-docs and persisted in the Trino.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8793b42
to
8f30cb9
Compare
8f30cb9
to
e95cf44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Maybe add a TODO in the code in the metadata query for the comments with a link to the Trino issue.
e95cf44
to
5702413
Compare
Overview
Checklist
README.md
updated and added information about my changeCHANGELOG.md
updated and added information about my change