From c3f58faf5ee79901429ccc49c003893b7196cb35 Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Tue, 23 Jun 2026 09:51:59 +0530 Subject: [PATCH 1/4] chore: remove unused internal logging-event classes (#1547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description Removes internal logging-event classes that have had **no call sites** since the cursor-management (#910/#912) and pipeline (#849) refactors. They are dead code: zero consumers across source and tests, no `events/__init__.py` re-export, and no dynamic/string references. Git history confirms they were once used and their consumers were later deleted (orphaned, not never-wired). Removed: - `events/credential_events.py` — whole module (`CredentialLoadError`, `CredentialSaveError`, `CredentialShardEvent`) - `events/pipeline_events.py` — whole module (`PipelineEvent`, `PipelineRefresh`, `PipelineRefreshError`) - `events/connection_events.py` — `ConnectionReset`, `ConnectionReuse`, `ConnectionIdleClose`, `ConnectionCreated` Kept (still live or still a needed base): `ConnectionEvent`, `ConnectionCreateError`, the `ConnectionWrapperEvent` base, `ConnectionCreate`, and all of `events/base.py` / `events/other_events.py` / `logging.py`. Targeting `1.13.latest` rather than a patch: although these were never public API, they sit at importable paths, so the removal is scoped to the next minor as a safety margin. Full unit suite passes (1081 passed, 6 skipped); ruff/ruff-format/mypy clean. ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests, or tests are not required/relevant for this PR - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt-databricks next" section. --- CHANGELOG.md | 6 +++++ .../databricks/events/connection_events.py | 20 ---------------- .../databricks/events/credential_events.py | 19 --------------- .../databricks/events/pipeline_events.py | 23 ------------------- 4 files changed, 6 insertions(+), 62 deletions(-) delete mode 100644 dbt/adapters/databricks/events/credential_events.py delete mode 100644 dbt/adapters/databricks/events/pipeline_events.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3adfc661f..eeacab1e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## dbt-databricks next + +### 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.0 (May 18, 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 41255ff08..000000000 --- a/dbt/adapters/databricks/events/credential_events.py +++ /dev/null @@ -1,19 +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 __init__(self, password_len: int): - self.password_len = password_len - - def __str__(self) -> str: - return f"Password is {self.password_len} characters, sharding it" 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}") From bbcace93e8d6f9cfff09cffcb2d371efbf1d9ed5 Mon Sep 17 00:00:00 2001 From: Rashi J Date: Sat, 8 Aug 2026 19:34:38 +0530 Subject: [PATCH 2/4] fix: warn when documented columns are missing from the relation in persist_docs (#1563) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports [dbt-labs/dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) (closes [dbt-labs/dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) into dbt-databricks. dbt-databricks overrides `persist_docs` with its own native handling, so the upstream macro change does not reach this adapter — the warning is added here directly. ### Description When `persist_docs.columns` is enabled, columns documented in a model's `schema.yml` that are **not** present in the relation being commented (typo, renamed column, stale docs) are silently skipped — no comment is applied and no feedback is given. This PR adds a warning that names those columns on the two **column-comment comparison paths**, where the adapter diffs documented columns against the relation's actual columns to build the comment `ALTER`: - **V1 materialization** — `DatabricksAdapter.get_persist_doc_columns` (`impl.py`), which iterates only the columns that actually exist in the relation. - **V2 materialization** — `ColumnCommentsConfig.get_diff` (`relation_configs/column_comments.py`), which previously emitted the missing column into the diff (targeting a nonexistent column on the `ALTER`). Filtering behavior is unchanged (missing columns are still skipped so the `ALTER` never errors); the columns are now surfaced via `warn_or_error`, so the warning respects `--warn-error` like other adapter warnings. Both paths route through a shared helper (`persist_doc_column_warnings`) that dedupes, so a single model materialization warns at most once per unique missing-column set. The dedupe cache is thread-local and reset in `pre_model_hook` at the start of each model — dbt pins each model to one worker thread, so parallel model runs never suppress each other's warnings. Example warning: ``` The following columns are specified in the schema but are not present in the database and will be skipped: col2 ``` ### Scope Covered: the two column-comment comparison paths above (V1 `get_persist_doc_columns`, V2 `ColumnCommentsConfig.get_diff`). These run when documented columns are diffed against an *existing* relation to build the comment `ALTER`: - **V1** — post-write on table and incremental materializations. - **V2** — the relation-config **changeset path**, reached by incremental (and materialized-view / streaming-table) re-runs. Note a V2 **table** rebuilds via `CREATE OR REPLACE` and applies comments inline every run, so it does not go through `get_diff` — V2 table coverage is the create-time follow-up below. ### Out of scope / follow-ups Deliberately not addressed here to keep the change focused on the comparison paths. Filed as follow-ups: - **Create-time inline comments (`parse_columns_and_constraints`).** This builds inline `CREATE`/replace comments and never warns. It covers V2 table models (every run) and the first create / `--full-refresh` of any model. A warning here must be gated explicitly on `config.persist_column_docs()` — an unconditional warning in `parse_columns_and_constraints` would fire even when column persistence is disabled, and `--warn-error` would turn those false positives into failures. - **View create (V1 + V2).** `get_persist_docs_column_list` iterates query columns only, so YAML-only columns stay silent. - **Typed persist gate.** `ColumnCommentsProcessor` gates column comments on `persist_docs.relation`; it should gate on `persist_docs.columns` (`config.persist_column_docs()`). - **V2 with `incremental_apply_config_changes: false`.** Skips `get_diff` entirely, with no V1-style persist_docs fallback, so no warning is emitted. ### Tests - **Unit** — both comparison paths, warn and no-warn branches (`tests/unit/test_adapter.py`, `tests/unit/relation_configs/test_column_comments_config.py`); dedupe helper (`tests/unit/test_persist_doc_column_warnings.py`) covering warn-once-per-set, shared V1/V2 dedupe, distinct sets, and thread isolation. - **Functional** (`tests/functional/adapter/persist_docs/test_persist_docs.py`) — V1 warns and still comments present columns; V2 warns on the changeset path (second run of an **incremental** model — a table rebuild re-applies comments inline and never reaches `get_diff`); `--warn-error` escalates the warning to a run failure. Verified against a live UC SQL warehouse. ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests (unit + functional; see above) - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt-databricks next" section. --------- Co-authored-by: Shubham Dhal --- CHANGELOG.md | 4 + .../relation_configs/column_comments.py | 6 +- .../macros/adapters/persist_docs.sql | 36 +- .../incremental/incremental.sql | 3 + .../macros/materializations/table.sql | 2 + .../adapter/persist_docs/fixtures.py | 76 +++++ .../adapter/persist_docs/test_persist_docs.py | 321 ++++++++++++++++++ .../adapters/test_persist_docs_macros.py | 60 ++++ .../test_column_comments_config.py | 24 +- 9 files changed, 527 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8578f0cf..2e8f9289e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## 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)). + ### 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)) 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/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/tests/functional/adapter/persist_docs/fixtures.py b/tests/functional/adapter/persist_docs/fixtures.py index 1dd73e884..899fc8671 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,63 @@ - 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" +""" diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index 4c40be2f6..271ee2f8a 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -419,3 +419,324 @@ 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"]}' + + +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" + + +@pytest.mark.skip_profile("databricks_cluster") +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" 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) From a36f2979eb682de38bc9527ead64e3e2f69325be Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Mon, 10 Aug 2026 12:33:15 +0530 Subject: [PATCH 3/4] fix: recreate materialized views on query schema drift (#1621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Detect when a materialized view's create-time column list no longer matches the query-inferred schema (e.g. upstream `select *` gains a column) and recreate via replace instead of issuing `REFRESH`, which Databricks rejects with an incompatible user-specified schema error. - Names-only comparison by design (type-label variance across DESCRIBE paths would cause spurious recreates). Streaming tables (#1303) are intentionally out of scope — they need a different approach around locking columns on CREATE. Resolves #1359 ## Test plan - [x] Unit: `tests/unit/test_column_schemas_differ.py` - [x] Functional (SQL warehouse): `tests/functional/adapter/materialized_view_tests/test_mv_schema_evolution.py` - [x] Smoke: existing `test_mv_alter_no_rebuild` still passes - [ ] CI integration tests --- CHANGELOG.md | 1 + .../materializations/materialized_view.sql | 16 ++- .../macros/relations/schema_drift.sql | 19 ++++ .../materialized_view_tests/fixtures.py | 45 ++++++++ .../test_mv_schema_evolution.py | 103 ++++++++++++++++++ .../macros/relations/test_schema_drift.py | 58 ++++++++++ 6 files changed, 240 insertions(+), 2 deletions(-) create mode 100644 dbt/include/databricks/macros/relations/schema_drift.sql create mode 100644 tests/functional/adapter/materialized_view_tests/test_mv_schema_evolution.py create mode 100644 tests/unit/macros/relations/test_schema_drift.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e8f9289e..f97ca715c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### Fixes +- Recreate materialized views when query schema drifts, honoring `on_configuration_change` ([#1621](https://github.com/databricks/dbt-databricks/pull/1621) resolves [#1359](https://github.com/databricks/dbt-databricks/issues/1359)) - 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)). ### Under the Hood diff --git a/dbt/include/databricks/macros/materializations/materialized_view.sql b/dbt/include/databricks/macros/materializations/materialized_view.sql index 0ed13fd03..a7859052b 100644 --- a/dbt/include/databricks/macros/materializations/materialized_view.sql +++ b/dbt/include/databricks/macros/materializations/materialized_view.sql @@ -33,9 +33,16 @@ -- get config options {% set on_configuration_change = config.get('on_configuration_change') %} {% set configuration_changes = get_configuration_changes(existing_relation) %} + {#- Schema drift is a configuration change the components cannot see, so it joins them + here and is then subject to the same `on_configuration_change` handling. REFRESH + cannot reconcile a drifted schema, so it demands a full refresh. -#} + {% set schema_drifted = dlt_inferred_query_schema_changed(existing_relation, sql) %} + {%- if schema_drifted and configuration_changes is not none -%} + {%- set configuration_changes = configuration_changes.model_copy(update={'requires_full_refresh': true}) -%} + {%- endif -%} {# Skip manual REFRESH on no-op re-runs for auto-refreshed modes. #} - {% if configuration_changes is none %} + {% if configuration_changes is none and not schema_drifted %} {%- set refresh = adapter.get_config_from_model(config.model).config["refresh"] -%} {%- if refresh.auto_refreshed -%} {% set build_sql = '' %} @@ -44,7 +51,12 @@ {%- endif -%} {% elif on_configuration_change == 'apply' %} - {% set build_sql = get_alter_materialized_view_as_sql(target_relation, configuration_changes, sql, existing_relation, None, None) %} + {%- if configuration_changes is none -%} + {#- Drift alone: no component changes to alter, so replace outright. -#} + {% set build_sql = get_replace_sql(existing_relation, target_relation, sql) %} + {%- else -%} + {% set build_sql = get_alter_materialized_view_as_sql(target_relation, configuration_changes, sql, existing_relation, None, None) %} + {%- endif -%} {% elif on_configuration_change == 'continue' %} {% set build_sql = "" %} {{ exceptions.warn("Configuration changes were identified and `on_configuration_change` was set to `continue` for `" ~ target_relation ~ "`") }} diff --git a/dbt/include/databricks/macros/relations/schema_drift.sql b/dbt/include/databricks/macros/relations/schema_drift.sql new file mode 100644 index 000000000..18df7478e --- /dev/null +++ b/dbt/include/databricks/macros/relations/schema_drift.sql @@ -0,0 +1,19 @@ +{#-- CREATE embeds an explicit column list (Databricks user-specified schema). + When that drifts from the current query (e.g. upstream `select *` column add), + REFRESH fails — the caller replaces instead (#1359). The model's query text is + unchanged in that scenario, so the QueryProcessor component of + get_configuration_changes cannot detect it; callers treat the result as an + additional configuration change. Names only: type-label variance across DESCRIBE + paths would cause spurious recreates. Streaming tables (#1303) are out of scope + here: locking columns on CREATE is a separate design question. --#} +{% macro dlt_inferred_query_schema_changed(relation, sql) -%} + {%- set inferred_names = get_columns_in_query(sql) | map('lower') | list -%} + {%- set existing_names = adapter.get_columns_in_relation(relation) + | map(attribute='name') | map('lower') | list -%} + {%- set drifted = inferred_names != existing_names -%} + {%- if drifted -%} + {%- do log("Materialized view " ~ relation ~ " query schema drifted (was " ~ existing_names + ~ ", now " ~ inferred_names ~ "); recreating instead of refreshing.") -%} + {%- endif -%} + {%- do return(drifted) -%} +{%- endmacro %} diff --git a/tests/functional/adapter/materialized_view_tests/fixtures.py b/tests/functional/adapter/materialized_view_tests/fixtures.py index 00450ad06..bf34f5dc2 100644 --- a/tests/functional/adapter/materialized_view_tests/fixtures.py +++ b/tests/functional/adapter/materialized_view_tests/fixtures.py @@ -257,3 +257,48 @@ def materialized_view_with_every(every_value: str) -> str: ) }} select * from {{ ref('mv_norebuild_seed') }} """ + +# Issue #1359: upstream select * schema evolution against an existing MV. +schema_evolution_base_v1_sql = """ +{{ config(materialized='table') }} +select 1 as id, 'foo' as name +""" + +schema_evolution_base_v2_sql = """ +{{ config(materialized='table') }} +select 1 as id, 'foo' as name, 42 as new_column +""" + +schema_evolution_mv_sql = """ +{{ config(materialized='materialized_view', on_configuration_change='apply') }} +select * from {{ ref('schema_evolution_base') }} +""" + +schema_evolution_mv_yml = """ +version: 2 +models: + - name: schema_evolution_mv + columns: + - name: id + - name: name +""" + +schema_evolution_mv_yml_v2 = """ +version: 2 +models: + - name: schema_evolution_mv + columns: + - name: id + - name: name + - name: new_column +""" + +schema_evolution_mv_fail_sql = """ +{{ config(materialized='materialized_view', on_configuration_change='fail') }} +select * from {{ ref('schema_evolution_base') }} +""" + +schema_evolution_mv_continue_sql = """ +{{ config(materialized='materialized_view', on_configuration_change='continue') }} +select * from {{ ref('schema_evolution_base') }} +""" diff --git a/tests/functional/adapter/materialized_view_tests/test_mv_schema_evolution.py b/tests/functional/adapter/materialized_view_tests/test_mv_schema_evolution.py new file mode 100644 index 000000000..bf23299be --- /dev/null +++ b/tests/functional/adapter/materialized_view_tests/test_mv_schema_evolution.py @@ -0,0 +1,103 @@ +"""Schema evolution for materialized views (issue #1359).""" + +import pytest +from dbt.tests import util + +from tests.functional.adapter.fixtures import RerunSafeMixin +from tests.functional.adapter.materialized_view_tests import fixtures + + +@pytest.mark.dlt +@pytest.mark.skip_profile("databricks_cluster", "databricks_uc_cluster") +class TestMaterializedViewSchemaEvolution(RerunSafeMixin): + """Upstream ``select *`` column adds must recreate the MV, not REFRESH.""" + + @pytest.fixture(scope="class") + def models(self): + return { + "schema_evolution_base.sql": fixtures.schema_evolution_base_v1_sql, + "schema_evolution_mv.sql": fixtures.schema_evolution_mv_sql, + "schema_evolution_mv.yml": fixtures.schema_evolution_mv_yml, + } + + @pytest.fixture(scope="class") + def relations_to_reset(self): + return ("schema_evolution_mv", "schema_evolution_base") + + def test_upstream_column_add_recreates_without_full_refresh_flag(self, project): + util.run_dbt(["run"]) + util.write_file( + fixtures.schema_evolution_base_v2_sql, "models", "schema_evolution_base.sql" + ) + + util.run_dbt(["run"]) + rows = project.run_sql("select id, name, new_column from schema_evolution_mv", fetch="all") + assert rows == [(1, "foo", 42)] + + def test_column_add_to_properties_yaml_recreates(self, project): + """The issue's exact sequence: upstream gains a column, then the YAML follows.""" + util.run_dbt(["run"]) + util.write_file( + fixtures.schema_evolution_base_v2_sql, "models", "schema_evolution_base.sql" + ) + util.write_file(fixtures.schema_evolution_mv_yml_v2, "models", "schema_evolution_mv.yml") + + util.run_dbt(["run"]) + rows = project.run_sql("select id, name, new_column from schema_evolution_mv", fetch="all") + assert rows == [(1, "foo", 42)] + + +@pytest.mark.dlt +@pytest.mark.skip_profile("databricks_cluster", "databricks_uc_cluster") +class TestMaterializedViewSchemaDriftOnConfigurationChangeFail(RerunSafeMixin): + """Schema drift is a configuration change, so `fail` must stop the run.""" + + @pytest.fixture(scope="class") + def models(self): + return { + "schema_evolution_base.sql": fixtures.schema_evolution_base_v1_sql, + "schema_evolution_mv.sql": fixtures.schema_evolution_mv_fail_sql, + } + + @pytest.fixture(scope="class") + def relations_to_reset(self): + return ("schema_evolution_mv", "schema_evolution_base") + + def test_drift_fails_and_leaves_mv_untouched(self, project): + util.run_dbt(["run"]) + util.write_file( + fixtures.schema_evolution_base_v2_sql, "models", "schema_evolution_base.sql" + ) + + util.run_dbt(["run"], expect_pass=False) + + rows = project.run_sql("select * from schema_evolution_mv", fetch="all") + assert rows == [(1, "foo")] + + +@pytest.mark.dlt +@pytest.mark.skip_profile("databricks_cluster", "databricks_uc_cluster") +class TestMaterializedViewSchemaDriftOnConfigurationChangeContinue(RerunSafeMixin): + """Schema drift is a configuration change, so `continue` must skip the rebuild.""" + + @pytest.fixture(scope="class") + def models(self): + return { + "schema_evolution_base.sql": fixtures.schema_evolution_base_v1_sql, + "schema_evolution_mv.sql": fixtures.schema_evolution_mv_continue_sql, + } + + @pytest.fixture(scope="class") + def relations_to_reset(self): + return ("schema_evolution_mv", "schema_evolution_base") + + def test_drift_continues_and_leaves_mv_untouched(self, project): + util.run_dbt(["run"]) + util.write_file( + fixtures.schema_evolution_base_v2_sql, "models", "schema_evolution_base.sql" + ) + + util.run_dbt(["run"]) + + rows = project.run_sql("select * from schema_evolution_mv", fetch="all") + assert rows == [(1, "foo")] diff --git a/tests/unit/macros/relations/test_schema_drift.py b/tests/unit/macros/relations/test_schema_drift.py new file mode 100644 index 000000000..87c09c868 --- /dev/null +++ b/tests/unit/macros/relations/test_schema_drift.py @@ -0,0 +1,58 @@ +"""Macro tests for MV query schema-drift detection (issue #1359).""" + +import pytest + +from dbt.adapters.databricks.column import DatabricksColumn +from tests.unit.macros.base import MacroTestBase + + +class TestDltInferredQuerySchemaChanged(MacroTestBase): + @pytest.fixture(scope="class") + def template_name(self) -> str: + return "schema_drift.sql" + + @pytest.fixture(scope="class") + def macro_folders_to_load(self) -> list: + return ["macros", "macros/relations"] + + def detect(self, template_bundle, existing: list[str], inferred: list[str]) -> bool: + """Run the macro with the two column-name lists the adapter would return. + + The macro yields its verdict via `do return(...)`, which renders as empty text, + so capture the value the harness's `return` hook receives. + """ + template_bundle.context["adapter"].get_columns_in_relation = lambda relation: [ + DatabricksColumn.create(name, "string") for name in existing + ] + template_bundle.context["get_columns_in_query"] = lambda sql: inferred + returned: list[bool] = [] + template_bundle.context["return"] = returned.append + self.run_macro_raw( + template_bundle.template, + "dlt_inferred_query_schema_changed", + template_bundle.relation, + "select * from upstream", + ) + assert len(returned) == 1, f"macro returned {len(returned)} values, expected 1" + return returned[0] + + def test_same_names_same_order(self, template_bundle): + assert self.detect(template_bundle, ["id", "name"], ["id", "name"]) is False + + def test_case_insensitive_name_match(self, template_bundle): + assert self.detect(template_bundle, ["ID", "Name"], ["id", "name"]) is False + + def test_added_column(self, template_bundle): + assert self.detect(template_bundle, ["id", "name"], ["id", "name", "new_column"]) is True + + def test_removed_column(self, template_bundle): + assert self.detect(template_bundle, ["id", "name", "gone"], ["id", "name"]) is True + + def test_reordered_columns(self, template_bundle): + assert self.detect(template_bundle, ["id", "name"], ["name", "id"]) is True + + def test_empty_inferred_columns(self, template_bundle): + assert self.detect(template_bundle, ["id"], []) is True + + def test_both_empty(self, template_bundle): + assert self.detect(template_bundle, [], []) is False From e493fbe9d40d12134196c51c32925acbc9123be9 Mon Sep 17 00:00:00 2001 From: Rashi J Date: Thu, 20 Aug 2026 14:04:37 +0530 Subject: [PATCH 4/4] fix: warn on missing persist_docs columns for view/materialized_view/streaming_table create (#1615) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description resolves #1399 Follow-up to #1563. That PR added the "warn when a documented column is absent from the relation" check as a **post-build validation** (`validate_persist_doc_columns`, mirroring the shared `validate_doc_columns` behavior every other adapter uses) and wired it into the **table** and **incremental** materializations. This PR completes the create-time coverage by calling the same gated, post-build validation on the materializations #1563 didn't touch: - **view** create (V1 + V2) — `dbt/include/databricks/macros/materializations/view.sql` - **materialized view** create/replace/refresh — `.../materialized_view.sql` - **streaming table** create/replace/refresh — `.../streaming_table.sql` Each is a single `validate_persist_doc_columns(target_relation, model)` call after the relation is built. The macro is gated on `config.persist_column_docs()` and applies no comments, so it never fires when column persistence is off and stays safe under `--warn-error` (per the caution in #1563 about not warning unconditionally in `parse_columns_and_constraints`). Previously these paths iterated only the query's output columns (`get_persist_docs_column_list`) or built inline comments via `parse_columns_and_constraints`, so a YAML-only documented column was silently dropped. ### Stacking > **This PR is stacked on #1563** and currently shows its commits too. It targets `main`; once #1563 merges, this diff reduces to just the view/materialized-view/streaming-table changes. Review/merge #1563 first. ### Tests Functional (`tests/functional/adapter/persist_docs/test_persist_docs.py`): create-time missing-column warning for **view**, **materialized view**, and **streaming table** (each: documented column absent from the relation → warns exactly once, gated on `persist_docs.columns`). New fixtures in `tests/functional/adapter/persist_docs/fixtures.py`. The core `validate_persist_doc_columns` logic is unit-tested in #1563. ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt-databricks next" section. --- CHANGELOG.md | 1 + .../materializations/materialized_view.sql | 2 + .../materializations/streaming_table.sql | 2 + .../macros/materializations/view.sql | 3 + .../adapter/persist_docs/fixtures.py | 72 +++++ .../adapter/persist_docs/test_persist_docs.py | 270 ++++++++++++++++++ 6 files changed, 350 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 814233aa8..173691d02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Recreate materialized views when query schema drifts, honoring `on_configuration_change` ([#1621](https://github.com/databricks/dbt-databricks/pull/1621) resolves [#1359](https://github.com/databricks/dbt-databricks/issues/1359)) - 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 diff --git a/dbt/include/databricks/macros/materializations/materialized_view.sql b/dbt/include/databricks/macros/materializations/materialized_view.sql index a7859052b..6e676aea9 100644 --- a/dbt/include/databricks/macros/materializations/materialized_view.sql +++ b/dbt/include/databricks/macros/materializations/materialized_view.sql @@ -86,6 +86,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/view.sql b/dbt/include/databricks/macros/materializations/view.sql index 4c0f2fbfb..b9340dadb 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 899fc8671..38b675bb8 100644 --- a/tests/functional/adapter/persist_docs/fixtures.py +++ b/tests/functional/adapter/persist_docs/fixtures.py @@ -124,3 +124,75 @@ - name: added_col description: "added column comment" """ + +# Same query change as alter_view_updated_sql (drives the ALTER path), but the schema.yml +# also documents a column that's never added to the query - genuinely missing, not just +# not-yet-altered - to prove validate_persist_doc_columns still fires on ALTER, not just create. +alter_view_updated_with_missing_col_yml = """ +version: 2 +models: + - name: alter_view + columns: + - name: id + description: "updated id comment" + - name: added_col + description: "added column comment" + - name: ghost_col + description: "comment that cannot be created" +""" + +# 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 271ee2f8a..7e494cdc7 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -740,3 +740,273 @@ def test_new_documented_column_is_not_warned_before_alter_view(self, project, ad 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, adapter): + util.run_dbt(["seed"]) + 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_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"] == "test id column description" + + +class TestPersistDocsColumnMissingWarnsViewCreateV1: + """v1 view create: same create-time coverage as V2, exercised on the else-branch of + view.sql (view.sql's validate_persist_doc_columns call outside the v2 flag).""" + + @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": False}, + "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, + } + + def test_view_create_warns(self, project, adapter): + util.run_dbt(["seed"]) + 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_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"] == "test id column description" + + +class TestPersistDocsColumnMissingV2ViewCreateNoWarn: + """v2 view create: persist_docs.columns=False gates the missing-column warning too.""" + + @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": False}}}, + } + + def test_no_warning_when_columns_disabled(self, project): + util.run_dbt(["seed"]) + util.run_dbt(["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS]) + + +@pytest.mark.skip_profile("databricks_cluster") +class TestPersistDocsColumnMissingWarnsV2AlterView: + """v2 view: a column documented in YAML but never added to the query still warns on the + ALTER path (mirrors TestPersistDocsPlannedColumnV2AlterView's create-vs-alter setup, but + with a genuinely missing column instead of one that only doesn't exist yet), proving + validate_persist_doc_columns isn't limited to create.""" + + @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_ghost_column_warns_on_alter(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_with_missing_col_yml, + project.project_root, + "models", + "schema.yml", + ) + + 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="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" + + +@pytest.mark.dlt +@pytest.mark.skip_profile("databricks_cluster", "databricks_uc_cluster") +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"]) + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + results = project.run_sql( + f""" + SELECT comment + FROM {project.database}.information_schema.columns + WHERE table_catalog = '{project.database}' + AND table_schema = '{project.test_schema}' + AND table_name = 'missing_column_mv' + AND column_name = 'id'""", + fetch="all", + ) + assert results[0][0] == "test id column description" + + # `refresh` defaults to manual (not auto-refreshed), so a subsequent run still goes + # through the shared execute-build-sql path (REFRESH) rather than a create-only one. + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + +@pytest.mark.dlt +@pytest.mark.skip_profile("databricks_cluster", "databricks_uc_cluster") +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"]) + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + ) + + results = project.run_sql( + f""" + SELECT comment + FROM {project.database}.information_schema.columns + WHERE table_catalog = '{project.database}' + AND table_schema = '{project.test_schema}' + AND table_name = 'missing_column_st' + AND column_name = 'id'""", + fetch="all", + ) + assert results[0][0] == "test id column description" + + # `refresh` defaults to manual (not auto-refreshed), so a subsequent run still goes + # through the shared execute-build-sql path (REFRESH) rather than a create-only one. + util.run_dbt( + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False + )