dbt-msft / dbt-sqlserver

dbt adapter for SQL Server and Azure SQL
MIT License
216 stars 101 forks source link

Implement persist_docs #134

Open telenieko opened 3 years ago

telenieko commented 3 years ago

dbt has a configuration option to persist documentation to the database: https://docs.getdbt.com/reference/resource-configs/persist_docs

Attempting to use it with the sql server adapter results in:

ERROR: alter_relation_comment macro not implemented for adapter sqlserver
ERROR: alter_column_comment macro not implemented for adapter sqlserver

Feature Request: implement the above

dataders commented 3 years ago

I agree that this feature would be cool, but how would one use it? This example using Extended Properties seems helpful, but how would a user of the database see this info in SSMS or Power BI?

I'm an Extended Properties newb, but the persist_docs feature makes a lot more sense for BigQuery and Snowflake where it's easier to see the docs.

telenieko commented 3 years ago

I agree that this feature would be cool, but how would one use it? This example using Extended Properties seems helpful, but how would a user of the database see this info in SSMS or Power BI?

I'm an Extended Properties newb, but the persist_docs feature makes a lot more sense for BigQuery and Snowflake where it's easier to see the docs.

TBH I have no idea how one would use it.

As far as I know Extended Properties is the recomended way to handle in-database documentation. So it would be safe to assume that user tooling (SSMS, PowerBI, ...) will at some point do proper use of that documentation.

Maybe the way to make those tools do proper use of Extended Properties is to first put something in there so people can start complaining they cannot use their documentation on the tools 😂.

So, focusing this issue, maybe: Step 1 is to see wether Extended Properties is the place to persist_docs, and Step 2 would be to make it happen. Step 3 would be to pester MSFT & other tools to add proper UI for using those docs?

jeroen-mostert commented 3 years ago

There is a semi-standard in the form of the MS_Description extended property. This is what SSMS surfaces for columns if you use the table designer, and it surfaces it for tables and columns if you use the Diagram Designer and show the properties. Power BI appears to have no support, at least it doesn't show anything if you're browsing tables in the import. I have no idea how widespread support for MS_Description is among third-party tools, but as a pick for something to use for persisting a description it's certainly better than nothing.

mahazza commented 3 years ago

I had a need for this feature using MS_Description. A few of our tools use it as default to pull out descriptions such as redgate tools, azure purview and I assume a few others. It may be useful to make MS_Description configurable if there is a need. I've attached what is working for us...

{% macro sqlserver__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) %}
        {%- do log('Alter extended property "MS_Description" to "' ~ column_dict[column_name]['description'] ~ '" for ' ~ relation ~ ' column "' ~ column_name ~ '"', info=false) %}
        IF NOT EXISTS (
            SELECT NULL 
            FROM SYS.EXTENDED_PROPERTIES AS ep

                INNER JOIN SYS.ALL_COLUMNS AS cols
                    ON cols.object_id = ep.major_id
                    AND cols.column_id = ep.minor_id

            WHERE   ep.major_id = OBJECT_ID('{{ relation }}') 
            AND     ep.name = N'MS_Description'
            AND     cols.name = N'{{ column_name }}'
        )
            EXECUTE sp_addextendedproperty      @name = N'MS_Description', @value = N'{{ column_dict[column_name]['description'] }}'
                                                , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                                , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}'
                                                , @level2type = N'COLUMN', @level2name = N'{{ column_name }}';
        ELSE
            EXECUTE sp_updateextendedproperty   @name = N'MS_Description', @value = N'{{ column_dict[column_name]['description'] }}'
                                                , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                                , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}'
                                                , @level2type = N'COLUMN', @level2name = N'{{ column_name }}';
    {%- endfor %}
{%- endmacro %}

{% macro sqlserver__alter_relation_comment(relation, relation_comment) -%}
   {% do log('Alter extended property "MS_Description" to "' ~ relation_comment ~ '" for ' ~ relation, info=false) %}

    IF NOT EXISTS (
        SELECT NULL 
        FROM SYS.EXTENDED_PROPERTIES AS ep
        WHERE   ep.major_id = OBJECT_ID('{{ relation }}')
        AND     ep.name = N'MS_Description'
        AND     ep.minor_id = 0
    )
        EXECUTE sp_addextendedproperty      @name = N'MS_Description', @value = N'{{ relation_comment }}'
                                            , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                            , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}';
    ELSE
        EXECUTE sp_updateextendedproperty   @name = N'MS_Description', @value = N'{{ relation_comment }}'
                                            , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                            , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}';
{% endmacro %}
dataders commented 3 years ago

I had a need for this feature using MS_Description. A few of our tools use it as default to pull out descriptions such as redgate tools, azure purview and I assume a few others. It may be useful to make MS_Description configurable if there is a need. I've attached what is working for us...

{% macro sqlserver__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) %}
        {%- do log('Alter extended property "MS_Description" to "' ~ column_dict[column_name]['description'] ~ '" for ' ~ relation ~ ' column "' ~ column_name ~ '"', info=false) %}
        IF NOT EXISTS (
            SELECT NULL 
            FROM SYS.EXTENDED_PROPERTIES AS ep

                INNER JOIN SYS.ALL_COLUMNS AS cols
                    ON cols.object_id = ep.major_id
                    AND cols.column_id = ep.minor_id

            WHERE   ep.major_id = OBJECT_ID('{{ relation }}') 
            AND     ep.name = N'MS_Description'
            AND       cols.name = N'{{ column_name }}'
        )
            EXECUTE sp_addextendedproperty      @name = N'MS_Description', @value = N'{{ column_dict[column_name]['description'] }}'
                                                , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                                , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}'
                                                , @level2type = N'COLUMN', @level2name = N'{{ column_name }}';
        ELSE
            EXECUTE sp_updateextendedproperty   @name = N'MS_Description', @value = N'{{ column_dict[column_name]['description'] }}'
                                                , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                                , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}'
                                                , @level2type = N'COLUMN', @level2name = N'{{ column_name }}';
    {%- endfor %}
{%- endmacro %}

{% macro sqlserver__alter_relation_comment(relation, relation_comment) -%}
   {% do log('Alter extended property "MS_Description" to "' ~ relation_comment ~ '" for ' ~ relation, info=false) %}

    IF NOT EXISTS (
        SELECT NULL 
        FROM SYS.EXTENDED_PROPERTIES AS ep
        WHERE   ep.major_id = OBJECT_ID('{{ relation }}')
        AND     ep.name = N'MS_Description'
        AND     ep.minor_id = 0
    )
        EXECUTE sp_addextendedproperty      @name = N'MS_Description', @value = N'{{ relation_comment }}'
                                            , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                            , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}';
    ELSE
        EXECUTE sp_updateextendedproperty   @name = N'MS_Description', @value = N'{{ relation_comment }}'
                                            , @level0type = N'SCHEMA', @level0name = N'{{ relation.schema }}'
                                            , @level1type = N'{{ relation.type }}', @level1name = N'{{ relation.identifier }}';
{% endmacro %}

this is awesome! can you tell me more about what "working for us" means? i.e. Purview/redgate can pull out column descriptions no problem? Way cool!

mahazza commented 3 years ago

With Redgate Sql Prompt, it will display MS_Description in the tooltip. Redgate Sql Doc also pulls it as the main description. I know the SSMS designer uses it too for MS_Descriptions on columns, but I don't know anybody who actually uses the designer in SSMS! I asked my colleague about azure purview and it needed some work apparently to get it to take in MS_Description, so I don't think this standard helps there.

edit I forgot to add, SSMS Database Diagram also displays these descriptions in the properties pane when you select a table/view or column

semcha commented 2 years ago

@mahazza Thank you for sharing that code!

semcha commented 1 year ago

@sdebruyn How else I can help?

sdebruyn commented 1 year ago

@semcha

I just added all the new adapter tests to #390. The docs persistence tests in there should be failing right now.

Ideally, your PR #289 would be merged into this branch and would make those tests succeed. That way we are much more certain that both the implementation works and that it works according to how dbt-core expects it to.

So if you would find some time, change the target of your PR to the branch used in #390 and make those persist_docs tests succeed :) Any help is much appreciated!