diff --git a/CHANGELOG.md b/CHANGELOG.md index 9048b637d..98782f8df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +## dbt-databricks next + +### Fixes + +- Warn when documented columns are missing from V1 and V2 table and incremental models, and honor `persist_docs.columns` for V2 column comments ([#1563](https://github.com/databricks/dbt-databricks/pull/1563) ports [dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684), resolving [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)). +- Extend the missing-column `persist_docs` warning to the view, materialized-view, and streaming-table create paths (post-build validation against the actual relation, gated on `persist_docs.columns`), completing the create-time coverage left as a follow-up to [#1563](https://github.com/databricks/dbt-databricks/pull/1563) ([#1615](https://github.com/databricks/dbt-databricks/pull/1615)). + +### Under the Hood + +- Remove unused internal logging-event classes (`CredentialLoadError`/`CredentialSaveError`/`CredentialShardEvent`, `PipelineEvent`/`PipelineRefresh`/`PipelineRefreshError`, and the `ConnectionReset`/`ConnectionReuse`/`ConnectionIdleClose`/`ConnectionCreated` connection events) that have had no call sites since the cursor-management and pipeline refactors ([#1547](https://github.com/databricks/dbt-databricks/pull/1547)) + ## dbt-databricks 1.12.2 (Jul 9, 2026) ### Features diff --git a/dbt/adapters/databricks/events/connection_events.py b/dbt/adapters/databricks/events/connection_events.py index 347e36b91..96c9c5b10 100644 --- a/dbt/adapters/databricks/events/connection_events.py +++ b/dbt/adapters/databricks/events/connection_events.py @@ -33,26 +33,6 @@ def __str__(self) -> str: return f"{self.description} - {self.message}" -class ConnectionReset(ConnectionWrapperEvent): - def __init__(self, description: str): - super().__init__(description, "Reset connection handle") - - -class ConnectionReuse(ConnectionWrapperEvent): - def __init__(self, description: str, prior_name: str): - super().__init__(description, f"Reusing connection previously named {prior_name}") - - class ConnectionCreate(ConnectionWrapperEvent): def __init__(self, description: str): super().__init__(description, "Creating connection") - - -class ConnectionIdleClose(ConnectionWrapperEvent): - def __init__(self, description: str): - super().__init__(description, "Recreating due to idleness") - - -class ConnectionCreated(ConnectionWrapperEvent): - def __init__(self, description: str): - super().__init__(description, "Connection created") diff --git a/dbt/adapters/databricks/events/credential_events.py b/dbt/adapters/databricks/events/credential_events.py deleted file mode 100644 index be58d69aa..000000000 --- a/dbt/adapters/databricks/events/credential_events.py +++ /dev/null @@ -1,16 +0,0 @@ -from dbt.adapters.databricks.events.base import ErrorEvent - - -class CredentialLoadError(ErrorEvent): - def __init__(self, exception: Exception): - super().__init__(exception, "Exception while trying to load credentials") - - -class CredentialSaveError(ErrorEvent): - def __init__(self, exception: Exception): - super().__init__(exception, "Exception while trying to save credentials") - - -class CredentialShardEvent: - def __str__(self) -> str: - return "Sharding credentials" diff --git a/dbt/adapters/databricks/events/pipeline_events.py b/dbt/adapters/databricks/events/pipeline_events.py deleted file mode 100644 index 3c526af6a..000000000 --- a/dbt/adapters/databricks/events/pipeline_events.py +++ /dev/null @@ -1,23 +0,0 @@ -from abc import ABC - - -class PipelineEvent(ABC): - def __init__(self, pipeline_id: str, update_id: str, message: str): - self.pipeline_id = pipeline_id - self.update_id = update_id - self.message = message - - def __str__(self) -> str: - return ( - f"Pipeline(pipeline-id={self.pipeline_id}, update-id={self.update_id}) - {self.message}" - ) - - -class PipelineRefresh(PipelineEvent): - def __init__(self, pipeline_id: str, update_id: str, state: str): - super().__init__(pipeline_id, update_id, f"Refreshing - got state {state}") - - -class PipelineRefreshError(PipelineEvent): - def __init__(self, pipeline_id: str, update_id: str, message: str): - super().__init__(pipeline_id, update_id, f"Error refreshing pipeline: {message}") diff --git a/dbt/adapters/databricks/relation_configs/column_comments.py b/dbt/adapters/databricks/relation_configs/column_comments.py index 2f3487151..6f502dee5 100644 --- a/dbt/adapters/databricks/relation_configs/column_comments.py +++ b/dbt/adapters/databricks/relation_configs/column_comments.py @@ -24,7 +24,9 @@ def get_diff(self, other: "ColumnCommentsConfig") -> Optional["ColumnCommentsCon other_comments_lower = {k.lower(): v for k, v in other.comments.items()} for column_name, comment in self.comments.items(): - # Use case-insensitive comparison for column names + # Missing columns are reported post-build. + if column_name.lower() not in other_comments_lower: + continue other_comment = other_comments_lower.get(column_name.lower()) if comment != other_comment: column_name = f"`{column_name}`" @@ -53,7 +55,7 @@ def from_relation_config(cls, relation_config: RelationConfig) -> ColumnComments columns = getattr(relation_config, "columns", {}) persist = False if relation_config.config: - persist = relation_config.config.persist_docs.get("relation") or False + persist = relation_config.config.persist_docs.get("columns") or False comments = {} for column_name, column in columns.items(): if hasattr(column, "description"): diff --git a/dbt/include/databricks/macros/adapters/persist_docs.sql b/dbt/include/databricks/macros/adapters/persist_docs.sql index da3734ea1..8ff88609e 100644 --- a/dbt/include/databricks/macros/adapters/persist_docs.sql +++ b/dbt/include/databricks/macros/adapters/persist_docs.sql @@ -37,11 +37,43 @@ {% endif %} {% if for_columns and config.persist_column_docs() and model.columns %} {%- set existing_columns = adapter.get_columns_in_relation(relation) -%} - {%- set columns_to_persist_docs = adapter.get_persist_doc_columns(existing_columns, model.columns) -%} + {%- set existing_column_names = existing_columns | map(attribute='name') | list -%} + {%- set valid_columns = dbt_databricks_validate_doc_columns(relation, model.columns, existing_column_names) -%} + {%- set columns_to_persist_docs = adapter.get_persist_doc_columns(existing_columns, valid_columns) -%} {{ alter_column_comment(relation, columns_to_persist_docs) }} {% endif %} {% endmacro %} +{#-- Match column names case-insensitively. --#} +{% macro dbt_databricks_validate_doc_columns(relation, column_dict, existing_column_names) -%} + {%- set existing_lower = existing_column_names | map('lower') | list -%} + {%- set missing = [] -%} + {%- set valid = {} -%} + {%- for column_name in column_dict -%} + {%- if (column_name | lower) in existing_lower -%} + {%- do valid.update({column_name: column_dict[column_name]}) -%} + {%- else -%} + {%- do missing.append(column_name) -%} + {%- endif -%} + {%- endfor -%} + {%- if missing -%} + {%- do exceptions.warn( + "In relation " ~ relation.render() ~ ": The following columns are specified in the schema " + ~ "but are not present in the database: " ~ missing | join(", ") + ) -%} + {%- endif -%} + {{- return(valid) -}} +{%- endmacro %} + +{#-- Validate V2 column docs post-build. --#} +{% macro validate_persist_doc_columns(relation, model) -%} + {% if config.persist_column_docs() and model.columns %} + {%- set existing_columns = adapter.get_columns_in_relation(relation) -%} + {%- set existing_column_names = existing_columns | map(attribute='name') | list -%} + {%- do dbt_databricks_validate_doc_columns(relation, model.columns, existing_column_names) -%} + {% endif %} +{%- endmacro %} + {% macro alter_relation_comment_sql(relation, description) %} COMMENT ON {{ relation.type.render().upper() }} {{ relation.render() }} IS '{{ description | replace("'", "\\'") }}' {% endmacro %} @@ -53,4 +85,4 @@ COMMENT ON {{ relation.type.render().upper() }} {{ relation.render() }} IS '{{ d {% set column_path = relation.render() ~ '.' ~ adapter.quote(column) %} {{ run_query_as(comment_on_column_sql(column_path, escaped_comment), 'main', fetch_result=False) }} {% endfor %} -{% endmacro %} \ No newline at end of file +{% endmacro %} diff --git a/dbt/include/databricks/macros/materializations/incremental/incremental.sql b/dbt/include/databricks/macros/materializations/incremental/incremental.sql index f226344b9..e949ca4c3 100644 --- a/dbt/include/databricks/macros/materializations/incremental/incremental.sql +++ b/dbt/include/databricks/macros/materializations/incremental/incremental.sql @@ -81,6 +81,9 @@ {%- endif -%} {%- endif -%} + {#-- Validate every create, replace, and merge path. --#} + {% do validate_persist_doc_columns(target_relation, model) %} + {% set should_revoke = should_revoke(existing_relation, full_refresh_mode) %} {% do apply_grants(target_relation, grant_config, should_revoke) %} {% do optimize(target_relation) %} diff --git a/dbt/include/databricks/macros/materializations/materialized_view.sql b/dbt/include/databricks/macros/materializations/materialized_view.sql index 0ed13fd03..9b978a0c5 100644 --- a/dbt/include/databricks/macros/materializations/materialized_view.sql +++ b/dbt/include/databricks/macros/materializations/materialized_view.sql @@ -74,6 +74,8 @@ {{ execute_multiple_statements(build_sql) }} + {% do validate_persist_doc_columns(target_relation, model) %} + {%- do apply_tags(target_relation, tags) -%} {% set column_tags = adapter.get_column_tags_from_model(config.model) %} diff --git a/dbt/include/databricks/macros/materializations/streaming_table.sql b/dbt/include/databricks/macros/materializations/streaming_table.sql index 7639077bc..7e5f712d6 100644 --- a/dbt/include/databricks/macros/materializations/streaming_table.sql +++ b/dbt/include/databricks/macros/materializations/streaming_table.sql @@ -74,6 +74,8 @@ {{ execute_multiple_statements(build_sql) }} + {% do validate_persist_doc_columns(target_relation, model) %} + {%- do apply_tags(target_relation, tags) -%} {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %} diff --git a/dbt/include/databricks/macros/materializations/table.sql b/dbt/include/databricks/macros/materializations/table.sql index 157c86993..f0f165255 100644 --- a/dbt/include/databricks/macros/materializations/table.sql +++ b/dbt/include/databricks/macros/materializations/table.sql @@ -32,6 +32,8 @@ {% endif %} {% endif %} + {% do validate_persist_doc_columns(target_relation, model) %} + {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %} {{ apply_grants(target_relation, grant_config, should_revoke) }} {% do optimize(target_relation) %} diff --git a/dbt/include/databricks/macros/materializations/view.sql b/dbt/include/databricks/macros/materializations/view.sql index 433afde1e..4b9a76ad2 100644 --- a/dbt/include/databricks/macros/materializations/view.sql +++ b/dbt/include/databricks/macros/materializations/view.sql @@ -36,6 +36,7 @@ {{ apply_column_tags(target_relation, column_tags) }} {% endif %} {% endif %} + {% do validate_persist_doc_columns(target_relation, model) %} {% set should_revoke = should_revoke(exists_as_view, full_refresh_mode=True) %} {% do apply_grants(target_relation, grant_config, should_revoke=True) %} @@ -66,6 +67,8 @@ {{ apply_column_tags(target_relation, column_tags) }} {% endif %} + {% do validate_persist_doc_columns(target_relation, model) %} + {{ run_hooks(post_hooks) }} {% endif %} diff --git a/tests/functional/adapter/persist_docs/fixtures.py b/tests/functional/adapter/persist_docs/fixtures.py index 1dd73e884..a06e65644 100644 --- a/tests/functional/adapter/persist_docs/fixtures.py +++ b/tests/functional/adapter/persist_docs/fixtures.py @@ -39,6 +39,22 @@ select 1 as id, 'alice' as name """ +missing_column_incremental_sql = """ +{{ config(materialized='incremental') }} +select 1 as id, 'Ed' as name +""" + +missing_column_incremental_schema = """ +version: 2 +models: + - name: missing_column_incremental + columns: + - name: id + description: "test id column description" + - name: column_that_does_not_exist + description: "comment that cannot be created" +""" + gate_model_schema = """ version: 2 models: @@ -48,3 +64,119 @@ - name: id description: The id column description """ + +schema_change_incremental_initial_sql = """ +{{ config(materialized='incremental', on_schema_change='append_new_columns') }} +select 1 as id +""" + +schema_change_incremental_updated_sql = """ +{{ config(materialized='incremental', on_schema_change='append_new_columns') }} +select 1 as id, 'new value' as new_col +""" + +schema_change_incremental_initial_yml = """ +version: 2 +models: + - name: schema_change_incremental + columns: + - name: id + description: "id comment" +""" + +schema_change_incremental_updated_yml = """ +version: 2 +models: + - name: schema_change_incremental + columns: + - name: id + description: "id comment" + - name: new_col + description: "new column comment" +""" + +alter_view_initial_sql = """ +{{ config(materialized='view', view_update_via_alter=true) }} +select 1 as id +""" + +alter_view_updated_sql = """ +{{ config(materialized='view', view_update_via_alter=true) }} +select 1 as id, 2 as added_col +""" + +alter_view_initial_yml = """ +version: 2 +models: + - name: alter_view + columns: + - name: id + description: "id comment" +""" + +alter_view_updated_yml = """ +version: 2 +models: + - name: alter_view + columns: + - name: id + description: "updated id comment" + - name: added_col + description: "added column comment" +""" + +# Create-time coverage for the materializations #1563 did not touch: view, materialized_view, +# streaming_table. Each documents a column absent from the relation; the post-build +# validate_persist_doc_columns check must surface it on create. +missing_column_create_seed = """id,value +1,10 +2,20 +""" + +missing_column_view_sql = """ +{{ config(materialized='view') }} +select * from {{ ref('mc_seed') }} +""" + +missing_column_view_schema = """ +version: 2 +models: + - name: missing_column_view + columns: + - name: id + description: "test id column description" + - name: column_that_does_not_exist + description: "comment that cannot be created" +""" + +missing_column_mv_sql = """ +{{ config(materialized='materialized_view') }} +select * from {{ ref('mc_seed') }} +""" + +missing_column_mv_schema = """ +version: 2 +models: + - name: missing_column_mv + columns: + - name: id + description: "test id column description" + - name: column_that_does_not_exist + description: "comment that cannot be created" +""" + +missing_column_st_sql = """ +{{ config(materialized='streaming_table') }} +select * from stream {{ ref('mc_seed') }} +""" + +missing_column_st_schema = """ +version: 2 +models: + - name: missing_column_st + columns: + - name: id + description: "test id column description" + - name: column_that_does_not_exist + description: "comment that cannot be created" +""" diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index 4c40be2f6..d2184c7f1 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -419,3 +419,415 @@ def test_column_comment_suppressed_when_columns_false(self, adapter, table_relat f"v1 must suppress column comment when persist_docs.columns is false, " f"got {id_columns[0].comment!r}" ) + + +_JINJA_WARNING_ERROR_OPTIONS = '{"error": ["JinjaLogWarning"]}' + +# Substring of the warning dbt_databricks_validate_doc_columns emits for a documented column +# absent from the relation ("In relation : The following columns are specified in the +# schema but are not present in the database: "). +_MISSING_COLUMN_WARNING = "are not present in the database" + + +class TestPersistDocsColumnMissingWarnsV1: + @pytest.fixture(scope="class") + def models(self): + return {"missing_column.sql": fixtures._MODELS__MISSING_COLUMN} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": fixtures._PROPERTIES__SCHEMA_MISSING_COL} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": False}, + "models": {"test": {"+persist_docs": {"relation": True, "columns": True}}}, + } + + @pytest.fixture(scope="class") + def table_relation(self, project): + return DatabricksRelation.create( + database=project.database, + schema=project.test_schema, + identifier="missing_column", + type="table", + ) + + def test_warns_and_still_comments_present_columns(self, adapter, table_relation): + util.run_dbt(["run"]) + + results = util.run_sql_with_adapter( + adapter, f"describe extended {table_relation}", fetch="all" + ) + _, columns = adapter.parse_describe_extended( + table_relation, Table(results, ["col_name", "data_type", "comment"]) + ) + id_columns = [c for c in columns if c.column == "id"] + assert id_columns and id_columns[0].comment + assert id_columns[0].comment.startswith("test id column description") + + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + +class TestPersistDocsColumnMissingWarnsV2: + @pytest.fixture(scope="class") + def models(self): + return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.missing_column_incremental_schema} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": True}, + "models": {"test": {"+persist_docs": {"relation": True, "columns": True}}}, + } + + def test_warning_escalates_on_create_and_subsequent_runs(self, project, adapter): + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + relation = DatabricksRelation.create( + database=project.database, + schema=project.test_schema, + identifier="missing_column_incremental", + type="table", + ) + rows = util.run_sql_with_adapter(adapter, f"describe extended {relation}", fetch="all") + _, columns = adapter.parse_describe_extended( + relation, Table(rows, ["col_name", "data_type", "comment"]) + ) + comments = {column.column: column.comment for column in columns} + assert comments["id"] == "test id column description" + + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + +class TestPersistDocsColumnMissingWarnsV2ColumnsOnly: + @pytest.fixture(scope="class") + def models(self): + return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.missing_column_incremental_schema} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": True}, + "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, + } + + def test_warning_escalates_with_columns_only(self, project, adapter): + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + relation = DatabricksRelation.create( + database=project.database, + schema=project.test_schema, + identifier="missing_column_incremental", + type="table", + ) + rows = util.run_sql_with_adapter(adapter, f"describe extended {relation}", fetch="all") + _, columns = adapter.parse_describe_extended( + relation, Table(rows, ["col_name", "data_type", "comment"]) + ) + comments = {column.column: column.comment for column in columns} + assert comments["id"] == "test id column description" + + +class TestPersistDocsColumnMissingV2RelationOnlyNoWarn: + @pytest.fixture(scope="class") + def models(self): + return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.missing_column_incremental_schema} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": True}, + "models": {"test": {"+persist_docs": {"relation": True, "columns": False}}}, + } + + def test_no_warning_when_columns_disabled(self, project): + util.run_dbt(["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS]) + + +class TestPersistDocsColumnMissingWarnsV1ColumnsOnly: + @pytest.fixture(scope="class") + def models(self): + return {"missing_column.sql": fixtures._MODELS__MISSING_COLUMN} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": fixtures._PROPERTIES__SCHEMA_MISSING_COL} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": False}, + "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, + } + + @pytest.fixture(scope="class") + def table_relation(self, project): + return DatabricksRelation.create( + database=project.database, + schema=project.test_schema, + identifier="missing_column", + type="table", + ) + + def test_warning_escalates_with_columns_only(self, adapter, table_relation): + util.run_dbt(["run"]) + + results = util.run_sql_with_adapter( + adapter, f"describe extended {table_relation}", fetch="all" + ) + _, columns = adapter.parse_describe_extended( + table_relation, Table(results, ["col_name", "data_type", "comment"]) + ) + id_columns = [c for c in columns if c.column == "id"] + assert id_columns and id_columns[0].comment + assert id_columns[0].comment.startswith("test id column description") + + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + +class TestPersistDocsColumnMissingWarnsV1IncrementalSubsequent: + @pytest.fixture(scope="class") + def models(self): + return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.missing_column_incremental_schema} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": False}, + "models": { + "test": { + "+persist_docs": {"relation": True, "columns": True}, + "+incremental_apply_config_changes": True, + } + }, + } + + def test_warning_escalates_on_subsequent_run(self, project): + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + +class TestPersistDocsPlannedColumnV1Incremental: + @pytest.fixture(scope="class") + def models(self): + return { + "schema_change_incremental.sql": ( + override_fixtures.schema_change_incremental_initial_sql + ) + } + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.schema_change_incremental_initial_yml} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": False}, + "models": { + "test": { + "+persist_docs": {"relation": False, "columns": True}, + } + }, + } + + def test_new_documented_column_is_not_warned_before_schema_sync(self, project, adapter): + util.run_dbt(["run"]) + util.write_file( + override_fixtures.schema_change_incremental_updated_sql, + project.project_root, + "models", + "schema_change_incremental.sql", + ) + util.write_file( + override_fixtures.schema_change_incremental_updated_yml, + project.project_root, + "models", + "schema.yml", + ) + + util.run_dbt(["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS]) + + relation = DatabricksRelation.create( + database=project.database, + schema=project.test_schema, + identifier="schema_change_incremental", + type="table", + ) + rows = util.run_sql_with_adapter(adapter, f"describe extended {relation}", fetch="all") + _, columns = adapter.parse_describe_extended( + relation, Table(rows, ["col_name", "data_type", "comment"]) + ) + comments = {column.column: column.comment for column in columns} + assert comments["new_col"] == "new column comment" + + +class TestPersistDocsPlannedColumnV2AlterView: + @pytest.fixture(scope="class") + def models(self): + return {"alter_view.sql": override_fixtures.alter_view_initial_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.alter_view_initial_yml} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": True}, + "models": { + "test": { + "+persist_docs": {"relation": False, "columns": True}, + } + }, + } + + def test_new_documented_column_is_not_warned_before_alter_view(self, project, adapter): + util.run_dbt(["run"]) + util.write_file( + override_fixtures.alter_view_updated_sql, + project.project_root, + "models", + "alter_view.sql", + ) + util.write_file( + override_fixtures.alter_view_updated_yml, + project.project_root, + "models", + "schema.yml", + ) + + util.run_dbt(["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS]) + + relation = DatabricksRelation.create( + database=project.database, + schema=project.test_schema, + identifier="alter_view", + type="view", + ) + rows = util.run_sql_with_adapter(adapter, f"describe extended {relation}", fetch="all") + _, columns = adapter.parse_describe_extended( + relation, Table(rows, ["col_name", "data_type", "comment"]) + ) + comments = {column.column: column.comment for column in columns} + assert comments["id"] == "updated id comment" + assert comments["added_col"] == "added column comment" + + +class TestPersistDocsColumnMissingWarnsViewCreate: + """v2 view create: a documented column absent from the view is warned about post-build.""" + + @pytest.fixture(scope="class") + def seeds(self): + return {"mc_seed.csv": override_fixtures.missing_column_create_seed} + + @pytest.fixture(scope="class") + def models(self): + return {"missing_column_view.sql": override_fixtures.missing_column_view_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.missing_column_view_schema} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": True}, + "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, + } + + def test_view_create_warns(self, project): + util.run_dbt(["seed"]) + _, logs = util.run_dbt_and_capture(["run"]) + assert "column_that_does_not_exist" in logs + assert logs.count(_MISSING_COLUMN_WARNING) == 1 + + +class TestPersistDocsColumnMissingWarnsMaterializedViewCreate: + """materialized view create: a documented column absent from the MV warns post-build.""" + + @pytest.fixture(scope="class") + def seeds(self): + return {"mc_seed.csv": override_fixtures.missing_column_create_seed} + + @pytest.fixture(scope="class") + def models(self): + return {"missing_column_mv.sql": override_fixtures.missing_column_mv_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.missing_column_mv_schema} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": True}, + "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, + } + + def test_materialized_view_create_warns(self, project): + util.run_dbt(["seed"]) + _, logs = util.run_dbt_and_capture(["run"]) + assert "column_that_does_not_exist" in logs + assert logs.count(_MISSING_COLUMN_WARNING) == 1 + + +class TestPersistDocsColumnMissingWarnsStreamingTableCreate: + """streaming table create: a documented column absent from the ST is warned about post-build.""" + + @pytest.fixture(scope="class") + def seeds(self): + return {"mc_seed.csv": override_fixtures.missing_column_create_seed} + + @pytest.fixture(scope="class") + def models(self): + return {"missing_column_st.sql": override_fixtures.missing_column_st_sql} + + @pytest.fixture(scope="class") + def properties(self): + return {"schema.yml": override_fixtures.missing_column_st_schema} + + @pytest.fixture(scope="class") + def project_config_update(self): + return { + "flags": {"use_materialization_v2": True}, + "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, + } + + def test_streaming_table_create_warns(self, project): + util.run_dbt(["seed"]) + _, logs = util.run_dbt_and_capture(["run"]) + assert "column_that_does_not_exist" in logs + assert logs.count(_MISSING_COLUMN_WARNING) == 1 diff --git a/tests/unit/macros/adapters/test_persist_docs_macros.py b/tests/unit/macros/adapters/test_persist_docs_macros.py index 0ebf7f7c5..12c1ff8be 100644 --- a/tests/unit/macros/adapters/test_persist_docs_macros.py +++ b/tests/unit/macros/adapters/test_persist_docs_macros.py @@ -1,3 +1,4 @@ +from ast import literal_eval from unittest.mock import MagicMock, Mock import pytest @@ -26,6 +27,65 @@ def mock_model_with_columns(self): return model + def test_validate_doc_columns_filters_missing_and_warns( + self, template_bundle, context, relation + ): + columns = { + "id": {"name": "id", "description": "Primary key"}, + "missing": {"name": "missing", "description": "Not materialized"}, + } + + result = self.run_macro_raw( + template_bundle.template, + "dbt_databricks_validate_doc_columns", + relation, + columns, + ["id"], + ) + + assert literal_eval(result) == {"id": columns["id"]} + context["exceptions"].warn.assert_called_once_with( + "In relation `some_database`.`some_schema`.`some_table`: The following columns are " + "specified in the schema but are not present in the database: missing" + ) + + def test_validate_doc_columns_is_silent_when_all_columns_exist( + self, template_bundle, context, relation + ): + columns = { + "id": {"name": "id", "description": "Primary key"}, + "value": {"name": "value", "description": "Value"}, + } + + result = self.run_macro_raw( + template_bundle.template, + "dbt_databricks_validate_doc_columns", + relation, + columns, + ["id", "value"], + ) + + assert literal_eval(result) == columns + context["exceptions"].warn.assert_not_called() + + def test_validate_doc_columns_matches_names_case_insensitively( + self, template_bundle, context, relation + ): + columns = { + "account_id": {"name": "account_id", "description": "Account ID"}, + } + + result = self.run_macro_raw( + template_bundle.template, + "dbt_databricks_validate_doc_columns", + relation, + columns, + ["Account_ID"], + ) + + assert literal_eval(result) == columns + context["exceptions"].warn.assert_not_called() + def test_comment_on_column_sql_dbr_16_1_or_newer(self, template_bundle, context): """Test COMMENT ON COLUMN syntax for DBR 16.1+""" column_path = "`test_db`.`test_schema`.`test_table`.id" diff --git a/tests/unit/relation_configs/test_column_comments_config.py b/tests/unit/relation_configs/test_column_comments_config.py index d88440f3d..0a132e0cd 100644 --- a/tests/unit/relation_configs/test_column_comments_config.py +++ b/tests/unit/relation_configs/test_column_comments_config.py @@ -45,10 +45,24 @@ def test_from_relation_config__no_persist(self): def test_from_relation_config__with_persist(self): model = Mock() model.columns = {"col1": {"description": "test comment"}} - model.config.persist_docs = {"relation": True} + model.config.persist_docs = {"columns": True} config = ColumnCommentsProcessor.from_relation_config(model) assert config == ColumnCommentsConfig(comments={"col1": "test comment"}, persist=True) + def test_from_relation_config__columns_true_relation_false(self): + model = Mock() + model.columns = {"col1": {"description": "test comment"}} + model.config.persist_docs = {"columns": True, "relation": False} + config = ColumnCommentsProcessor.from_relation_config(model) + assert config == ColumnCommentsConfig(comments={"col1": "test comment"}, persist=True) + + def test_from_relation_config__relation_true_columns_false(self): + model = Mock() + model.columns = {"col1": {"description": "test comment"}} + model.config.persist_docs = {"relation": True, "columns": False} + config = ColumnCommentsProcessor.from_relation_config(model) + assert config == ColumnCommentsConfig(comments={"col1": "test comment"}, persist=False) + class TestColumnCommentsConfig: def test_get_diff__no_changes(self): @@ -102,3 +116,11 @@ def test_get_diff__case_mismatch_with_actual_changes(self): assert diff == ColumnCommentsConfig( comments={"`account_id`": "New Account ID"}, persist=True ) + + def test_get_diff__skips_missing_column(self): + config = ColumnCommentsConfig( + comments={"col1": "new comment", "col2": "comment for missing column"}, persist=True + ) + other = ColumnCommentsConfig(comments={"col1": "old comment"}) + diff = config.get_diff(other) + assert diff == ColumnCommentsConfig(comments={"`col1`": "new comment"}, persist=True)