From c3f58faf5ee79901429ccc49c003893b7196cb35 Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Tue, 23 Jun 2026 09:51:59 +0530 Subject: [PATCH 01/16] 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 bdd2bfb69113ceb65510bb0a1bce54a17d0b9809 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 30 Jun 2026 14:33:45 +0530 Subject: [PATCH 02/16] Warn when documented columns are missing from the relation in persist_docs When persist_docs.columns is enabled, columns documented in a model's schema.yml but absent from the materialized relation were silently skipped by get_persist_doc_columns. Emit a warning naming those columns so users can catch typos and stale documentation. The columns are still filtered out (no behavior change to the comments that get applied). Ports the behavior added upstream in dbt-adapters#1684 (issue #1690). --- CHANGELOG.md | 4 ++++ dbt/adapters/databricks/impl.py | 21 +++++++++++++++++++++ tests/unit/test_adapter.py | 25 +++++++++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8578f0cf..64d146e0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## dbt-databricks next +### Fixes + +- Warn when a column documented in a model's `schema.yml` is absent from the relation while applying column comments, instead of silently skipping it — surfaces typos and stale column documentation. Covers the column-comment comparison paths (V1 `get_persist_doc_columns` and V2 `ColumnCommentsConfig.get_diff`); create-time inline comments are not yet covered. Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). + ### 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/impl.py b/dbt/adapters/databricks/impl.py index 3e1069417..fc3745761 100644 --- a/dbt/adapters/databricks/impl.py +++ b/dbt/adapters/databricks/impl.py @@ -20,6 +20,7 @@ from dbt.adapters.catalogs import CatalogRelation from dbt.adapters.contracts.connection import AdapterResponse, Connection from dbt.adapters.contracts.relation import RelationConfig, RelationType +from dbt.adapters.events.types import AdapterEventWarning from dbt.adapters.relation_configs import RelationResults from dbt.adapters.spark.impl import ( DESCRIBE_TABLE_EXTENDED_MACRO_NAME, @@ -31,6 +32,7 @@ ) from dbt_common.behavior_flags import BehaviorFlag from dbt_common.contracts.config.base import BaseConfig, MergeBehavior +from dbt_common.events.functions import warn_or_error from dbt_common.exceptions import DbtConfigError, DbtInternalError, DbtRuntimeError from dbt_common.record import auto_record_function, record_function from dbt_common.utils import executor @@ -1008,6 +1010,25 @@ def get_persist_doc_columns( # Create a case-insensitive lookup for column names columns_lower = {k.lower(): k for k in columns.keys()} + # Warn about columns that are documented in the model's schema but are not present in the + # relation. These are silently skipped below (rather than erroring on the alter), so surface + # them to the user to catch typos and stale documentation. + existing_lower = {column.column.lower() for column in existing_columns} + missing = [ + original_name + for name_lower, original_name in columns_lower.items() + if name_lower not in existing_lower + ] + if missing: + warn_or_error( + AdapterEventWarning( + base_msg=( + "The following columns are specified in the schema but are not present " + "in the database and will be skipped: " + ", ".join(missing) + ) + ) + ) + for column in existing_columns: name = column.column # Use case-insensitive comparison for column names diff --git a/tests/unit/test_adapter.py b/tests/unit/test_adapter.py index 33c841983..2a4e948ae 100644 --- a/tests/unit/test_adapter.py +++ b/tests/unit/test_adapter.py @@ -1182,6 +1182,31 @@ def test_get_persist_doc_columns_case_mismatch_no_update_needed(self, adapter): # No update needed since comments match assert result == {} + @patch("dbt.adapters.databricks.impl.warn_or_error") + def test_get_persist_doc_columns_warns_on_missing_column(self, mock_warn, adapter): + """Documented columns absent from the relation are warned about and skipped.""" + existing = [self.create_column("col1", "comment1")] + column_dict = { + "col1": {"name": "col1", "description": "new comment"}, + "col2": {"name": "col2", "description": "comment for missing column"}, + } + result = adapter.get_persist_doc_columns(existing, column_dict) + # The missing column is filtered out; only the existing column is returned. + assert result == {"col1": {"name": "col1", "description": "new comment"}} + # A warning is emitted naming the missing column. + mock_warn.assert_called_once() + warned_event = mock_warn.call_args.args[0] + assert "col2" in warned_event.base_msg + assert "col1" not in warned_event.base_msg + + @patch("dbt.adapters.databricks.impl.warn_or_error") + def test_get_persist_doc_columns_no_warning_when_all_present(self, mock_warn, adapter): + """No warning is emitted when every documented column exists (case-insensitively).""" + existing = [self.create_column("Account_ID", "")] + column_dict = {"account_id": {"name": "account_id", "description": "Account ID column"}} + adapter.get_persist_doc_columns(existing, column_dict) + mock_warn.assert_not_called() + class TestGetColumnsByDbrVersion(DatabricksAdapterBase): @pytest.fixture From af361c0631a1e2e1db35d4080b004caa2261b103 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Mon, 20 Jul 2026 16:22:47 +0530 Subject: [PATCH 03/16] Warn when documented columns are missing on the V2 materialization path The persist_docs missing-column warning only covered the V1 path (get_persist_doc_columns). The V2 (relation-config) path diffs column comments through ColumnCommentsConfig.get_diff, where a column documented in schema.yml but absent from the relation was still emitted into the diff (targeting a nonexistent column on the ALTER) with no feedback. Warn about those columns and skip them, matching the V1 behavior and the same warning message. Addresses review feedback on #1563. --- .../relation_configs/column_comments.py | 22 ++++++++++++ .../test_column_comments_config.py | 35 ++++++++++++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/dbt/adapters/databricks/relation_configs/column_comments.py b/dbt/adapters/databricks/relation_configs/column_comments.py index 2f3487151..493304a59 100644 --- a/dbt/adapters/databricks/relation_configs/column_comments.py +++ b/dbt/adapters/databricks/relation_configs/column_comments.py @@ -1,7 +1,9 @@ from typing import ClassVar, Optional from dbt.adapters.contracts.relation import RelationConfig +from dbt.adapters.events.types import AdapterEventWarning from dbt.adapters.relation_configs.config_base import RelationResults +from dbt_common.events.functions import warn_or_error from dbt.adapters.databricks.logging import logger from dbt.adapters.databricks.relation_configs.base import ( @@ -23,8 +25,28 @@ def get_diff(self, other: "ColumnCommentsConfig") -> Optional["ColumnCommentsCon # Create a case-insensitive lookup for other's column comments other_comments_lower = {k.lower(): v for k, v in other.comments.items()} + # Warn about columns that are documented in the model's schema but are not present in + # the relation. These are skipped below (rather than erroring on the alter), so surface + # them to the user to catch typos and stale documentation. + missing = [ + column_name + for column_name in self.comments + if column_name.lower() not in other_comments_lower + ] + if missing: + warn_or_error( + AdapterEventWarning( + base_msg=( + "The following columns are specified in the schema but are not present " + "in the database and will be skipped: " + ", ".join(missing) + ) + ) + ) + for column_name, comment in self.comments.items(): # Use case-insensitive comparison for column names + 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}`" diff --git a/tests/unit/relation_configs/test_column_comments_config.py b/tests/unit/relation_configs/test_column_comments_config.py index d88440f3d..0f9948014 100644 --- a/tests/unit/relation_configs/test_column_comments_config.py +++ b/tests/unit/relation_configs/test_column_comments_config.py @@ -1,4 +1,4 @@ -from unittest.mock import Mock +from unittest.mock import Mock, patch from agate import Table @@ -102,3 +102,36 @@ def test_get_diff__case_mismatch_with_actual_changes(self): assert diff == ColumnCommentsConfig( comments={"`account_id`": "New Account ID"}, persist=True ) + + @patch("dbt.adapters.databricks.relation_configs.column_comments.warn_or_error") + def test_get_diff__warns_and_skips_missing_column(self, mock_warn): + """Documented columns absent from the relation are warned about and skipped.""" + # col2 is documented but not present in the relation + config = ColumnCommentsConfig( + comments={"col1": "new comment", "col2": "comment for missing column"}, persist=True + ) + other = ColumnCommentsConfig(comments={"col1": "old comment"}) + diff = config.get_diff(other) + # Only the existing column is included in the diff; the missing one is skipped. + assert diff == ColumnCommentsConfig(comments={"`col1`": "new comment"}, persist=True) + # A warning is emitted naming the missing column. + mock_warn.assert_called_once() + warned_event = mock_warn.call_args.args[0] + assert "col2" in warned_event.base_msg + assert "col1" not in warned_event.base_msg + + @patch("dbt.adapters.databricks.relation_configs.column_comments.warn_or_error") + def test_get_diff__no_warning_when_all_present(self, mock_warn): + """No warning is emitted when every documented column exists (case-insensitively).""" + config = ColumnCommentsConfig(comments={"account_id": "Account ID"}, persist=True) + other = ColumnCommentsConfig(comments={"Account_ID": ""}) + config.get_diff(other) + mock_warn.assert_not_called() + + @patch("dbt.adapters.databricks.relation_configs.column_comments.warn_or_error") + def test_get_diff__no_warning_when_not_persisting(self, mock_warn): + """Missing columns are not evaluated (or warned about) when persist is False.""" + config = ColumnCommentsConfig(comments={"col1": "comment", "col2": "comment"}) + other = ColumnCommentsConfig(comments={"col1": "comment"}) + assert config.get_diff(other) is None + mock_warn.assert_not_called() From 8d412b81b38a6686945bd3cafd71b4195d577628 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 21 Jul 2026 16:14:38 +0530 Subject: [PATCH 04/16] test: functional coverage for persist_docs missing-column warning Add three functional tests exercising the missing-column warning end to end: - V1 comment path warns and still comments present columns - V2 alter path (get_diff) warns on a subsequent run - --warn-error escalates the warning to a run failure --- .../adapter/persist_docs/test_persist_docs.py | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index 4c40be2f6..bf5d34ffe 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -419,3 +419,115 @@ 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}" ) + + +# Tail of the warning emitted when a documented column is absent from the relation. The offending +# column name (column_that_does_not_exist, from fixtures._PROPERTIES__SCHEMA_MISSING_COL) is +# asserted separately. +_MISSING_COLUMN_WARNING = "not present in the database and will be skipped" + + +class TestPersistDocsColumnMissingWarnsV1: + """v1: a documented column absent from the relation is warned about, not silently skipped. + + Complements TestPersistDocsColumnMissing (which only checks the run survives) by asserting the + warning names the offending column on the v1 comment path + (DatabricksAdapter.get_persist_doc_columns), and that present columns are still commented. + """ + + @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): + _, logs = util.run_dbt_and_capture(["run"]) + assert _MISSING_COLUMN_WARNING in logs + assert "column_that_does_not_exist" in logs + + 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") + + +class TestPersistDocsColumnMissingWarnsV2: + """v2: the warning surfaces on the alter path (ColumnCommentsConfig.get_diff). + + On first create, comments are applied inline (parse_columns_and_constraints) and a + documented-but-absent column is silently dropped — get_diff is not consulted. The warning + therefore appears on a subsequent run, when documented columns are diffed against the existing + relation. (Create-time warning is tracked as a follow-up.) + """ + + @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": True}, + "models": {"test": {"+persist_docs": {"relation": True, "columns": True}}}, + } + + def test_warns_on_second_run(self, project): + # First run creates the relation; the inline-comment create path does not warn. + first_logs = util.run_dbt_and_capture(["run"])[1] + assert _MISSING_COLUMN_WARNING not in first_logs + + # Second run diffs documented columns against the existing relation → warns. + second_logs = util.run_dbt_and_capture(["run"])[1] + assert _MISSING_COLUMN_WARNING in second_logs + assert "column_that_does_not_exist" in second_logs + + +class TestPersistDocsColumnMissingWarnError: + """--warn-error escalates the missing-column warning to a run failure (v1 path).""" + + @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}}}, + } + + def test_warn_error_fails_run(self, project): + _, logs = util.run_dbt_and_capture(["run", "--warn-error"], expect_pass=False) + assert "column_that_does_not_exist" in logs From bd412b96cb4cd23e9d4a14b552474cf2337f8995 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 21 Jul 2026 16:22:58 +0530 Subject: [PATCH 05/16] test: use incremental model for v2 missing-column warning A table rebuild re-applies comments inline and never hits ColumnCommentsConfig.get_diff; the v2 changeset/alter path is only reached on a subsequent incremental run. Verified all three functional tests pass against a live UC SQL warehouse. --- .../adapter/persist_docs/fixtures.py | 19 +++++++++++++++++++ .../adapter/persist_docs/test_persist_docs.py | 13 +++++++------ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/tests/functional/adapter/persist_docs/fixtures.py b/tests/functional/adapter/persist_docs/fixtures.py index 1dd73e884..af2aa3141 100644 --- a/tests/functional/adapter/persist_docs/fixtures.py +++ b/tests/functional/adapter/persist_docs/fixtures.py @@ -39,6 +39,25 @@ select 1 as id, 'alice' as name """ +# Incremental model whose schema documents a column absent from the relation. Used to exercise the +# V2 alter/changeset path (ColumnCommentsConfig.get_diff), which — unlike a table rebuild — is only +# reached on a subsequent run against an existing relation. +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: diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index bf5d34ffe..db7515178 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -478,19 +478,20 @@ def test_warns_and_still_comments_present_columns(self, adapter, table_relation) class TestPersistDocsColumnMissingWarnsV2: """v2: the warning surfaces on the alter path (ColumnCommentsConfig.get_diff). - On first create, comments are applied inline (parse_columns_and_constraints) and a - documented-but-absent column is silently dropped — get_diff is not consulted. The warning - therefore appears on a subsequent run, when documented columns are diffed against the existing - relation. (Create-time warning is tracked as a follow-up.) + Uses an incremental model: a table rebuild re-applies comments inline and never consults + get_diff, so the changeset path is only reached on a subsequent incremental run, when + documented columns are diffed against the existing relation. On first create the + documented-but-absent column is silently dropped inline. (Create-time warning is tracked as a + follow-up.) """ @pytest.fixture(scope="class") def models(self): - return {"missing_column.sql": fixtures._MODELS__MISSING_COLUMN} + return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} @pytest.fixture(scope="class") def properties(self): - return {"schema.yml": fixtures._PROPERTIES__SCHEMA_MISSING_COL} + return {"schema.yml": override_fixtures.missing_column_incremental_schema} @pytest.fixture(scope="class") def project_config_update(self): From 2dc3a53bcfb1d3628f4fa1d69cab1d2713f6a366 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 21 Jul 2026 16:28:48 +0530 Subject: [PATCH 06/16] test: assert missing-column warning is emitted exactly once The v1 and v2 warning sites are mutually exclusive per model run, so a single run warns exactly once. Lock that in as a regression guard against future double-warning if the warning is added to additional helpers. --- .../functional/adapter/persist_docs/test_persist_docs.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index db7515178..94c87fcb1 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -461,8 +461,9 @@ def table_relation(self, project): def test_warns_and_still_comments_present_columns(self, adapter, table_relation): _, logs = util.run_dbt_and_capture(["run"]) - assert _MISSING_COLUMN_WARNING in logs assert "column_that_does_not_exist" in logs + # Emitted exactly once — the v1 and v2 warning sites are mutually exclusive per run. + assert logs.count(_MISSING_COLUMN_WARNING) == 1 results = util.run_sql_with_adapter( adapter, f"describe extended {table_relation}", fetch="all" @@ -505,10 +506,11 @@ def test_warns_on_second_run(self, project): first_logs = util.run_dbt_and_capture(["run"])[1] assert _MISSING_COLUMN_WARNING not in first_logs - # Second run diffs documented columns against the existing relation → warns. + # Second run diffs documented columns against the existing relation → warns exactly once + # (get_diff runs once per component in a single get_changeset). second_logs = util.run_dbt_and_capture(["run"])[1] - assert _MISSING_COLUMN_WARNING in second_logs assert "column_that_does_not_exist" in second_logs + assert second_logs.count(_MISSING_COLUMN_WARNING) == 1 class TestPersistDocsColumnMissingWarnError: From 11c4921a80b66b8b4aa88827fa68784d6ccba705 Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Fri, 24 Jul 2026 14:08:12 +0530 Subject: [PATCH 07/16] fix(persist_docs): emit missing-column warning only once per model V1 incremental subsequent runs hit both get_diff and get_persist_doc_columns; dedupe via a shared thread-local helper, and cover columns-only + the double-warn path. --- CHANGELOG.md | 2 +- dbt/adapters/databricks/impl.py | 23 +++-- .../databricks/persist_doc_column_warnings.py | 54 ++++++++++++ .../relation_configs/column_comments.py | 15 +--- .../adapter/persist_docs/test_persist_docs.py | 87 ++++++++++++++++++- .../test_column_comments_config.py | 12 ++- tests/unit/test_adapter.py | 9 +- .../unit/test_persist_doc_column_warnings.py | 56 ++++++++++++ 8 files changed, 228 insertions(+), 30 deletions(-) create mode 100644 dbt/adapters/databricks/persist_doc_column_warnings.py create mode 100644 tests/unit/test_persist_doc_column_warnings.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 64d146e0a..98f156d27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Fixes -- Warn when a column documented in a model's `schema.yml` is absent from the relation while applying column comments, instead of silently skipping it — surfaces typos and stale column documentation. Covers the column-comment comparison paths (V1 `get_persist_doc_columns` and V2 `ColumnCommentsConfig.get_diff`); create-time inline comments are not yet covered. Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). +- Warn when a column documented in a model's `schema.yml` is absent from the relation while applying column comments, instead of silently skipping it — surfaces typos and stale column documentation. Covers the column-comment comparison paths (V1 `get_persist_doc_columns` and V2 `ColumnCommentsConfig.get_diff`); create-time inline comments are not yet covered. The warning is emitted at most once per unique missing-column set when both paths run in the same model (e.g. V1 incremental subsequent). Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). ### Under the Hood diff --git a/dbt/adapters/databricks/impl.py b/dbt/adapters/databricks/impl.py index fc3745761..ac84fb14f 100644 --- a/dbt/adapters/databricks/impl.py +++ b/dbt/adapters/databricks/impl.py @@ -3,7 +3,7 @@ import re from abc import ABC, abstractmethod from collections import defaultdict -from collections.abc import Iterable, Iterator +from collections.abc import Iterable, Iterator, Mapping from concurrent.futures import Future from contextlib import contextmanager from dataclasses import dataclass, field @@ -20,7 +20,6 @@ from dbt.adapters.catalogs import CatalogRelation from dbt.adapters.contracts.connection import AdapterResponse, Connection from dbt.adapters.contracts.relation import RelationConfig, RelationType -from dbt.adapters.events.types import AdapterEventWarning from dbt.adapters.relation_configs import RelationResults from dbt.adapters.spark.impl import ( DESCRIBE_TABLE_EXTENDED_MACRO_NAME, @@ -32,7 +31,6 @@ ) from dbt_common.behavior_flags import BehaviorFlag from dbt_common.contracts.config.base import BaseConfig, MergeBehavior -from dbt_common.events.functions import warn_or_error from dbt_common.exceptions import DbtConfigError, DbtInternalError, DbtRuntimeError from dbt_common.record import auto_record_function, record_function from dbt_common.utils import executor @@ -59,6 +57,10 @@ from dbt.adapters.databricks.global_state import GlobalState from dbt.adapters.databricks.handle import SqlUtils from dbt.adapters.databricks.logging import logger +from dbt.adapters.databricks.persist_doc_column_warnings import ( + reset_missing_persist_doc_column_warnings, + warn_missing_persist_doc_columns, +) from dbt.adapters.databricks.python_models.python_submissions import ( AllPurposeClusterPythonJobHelper, JobClusterPythonJobHelper, @@ -904,6 +906,11 @@ def get_behavior_flag_no_warn(self, behavior_flag_name: str) -> bool: behavior_flag = getattr(self.behavior, behavior_flag_name) return behavior_flag.no_warn + def pre_model_hook(self, config: Mapping[str, Any]) -> Any: + """Reset missing-column warn dedupe so each model materialization can warn once.""" + reset_missing_persist_doc_column_warnings() + return super().pre_model_hook(config) + @available.parse(lambda *a, **k: (None, None)) @record_function( DatabricksAdapterAddQueryRecord, @@ -1019,15 +1026,7 @@ def get_persist_doc_columns( for name_lower, original_name in columns_lower.items() if name_lower not in existing_lower ] - if missing: - warn_or_error( - AdapterEventWarning( - base_msg=( - "The following columns are specified in the schema but are not present " - "in the database and will be skipped: " + ", ".join(missing) - ) - ) - ) + warn_missing_persist_doc_columns(missing) for column in existing_columns: name = column.column diff --git a/dbt/adapters/databricks/persist_doc_column_warnings.py b/dbt/adapters/databricks/persist_doc_column_warnings.py new file mode 100644 index 000000000..118aaca2e --- /dev/null +++ b/dbt/adapters/databricks/persist_doc_column_warnings.py @@ -0,0 +1,54 @@ +"""Shared warning for documented columns absent from the relation. + +Both the V1 persist_docs helper (``get_persist_doc_columns``) and the V2 changeset +helper (``ColumnCommentsConfig.get_diff``) need to surface the same user-facing +warning. On V1 incremental subsequent runs those two paths can both execute in a +single model materialization; dedupe so the message appears exactly once per +unique missing set **within that materialization**. + +State is thread-local: dbt assigns each model to one worker thread, so parallel +runs do not clear or suppress each other's warnings. ``pre_model_hook`` resets +the cache at the start of each model on that thread. +""" + +from __future__ import annotations + +import threading +from collections.abc import Sequence + +from dbt.adapters.events.types import AdapterEventWarning +from dbt_common.events.functions import warn_or_error + +_thread_state = threading.local() + + +def _emitted_keys() -> set[str]: + keys = getattr(_thread_state, "emitted_missing_keys", None) + if keys is None: + keys = set() + _thread_state.emitted_missing_keys = keys + return keys + + +def reset_missing_persist_doc_column_warnings() -> None: + """Clear the per-materialization dedupe cache for the current thread.""" + _thread_state.emitted_missing_keys = set() + + +def warn_missing_persist_doc_columns(missing: Sequence[str]) -> None: + """Warn once per unique set of documented-but-absent column names (per thread).""" + if not missing: + return + key = ", ".join(sorted(missing, key=str.lower)) + emitted = _emitted_keys() + if key in emitted: + return + emitted.add(key) + warn_or_error( + AdapterEventWarning( + base_msg=( + "The following columns are specified in the schema but are not present " + "in the database and will be skipped: " + ", ".join(missing) + ) + ) + ) diff --git a/dbt/adapters/databricks/relation_configs/column_comments.py b/dbt/adapters/databricks/relation_configs/column_comments.py index 493304a59..26f5d95f4 100644 --- a/dbt/adapters/databricks/relation_configs/column_comments.py +++ b/dbt/adapters/databricks/relation_configs/column_comments.py @@ -1,11 +1,12 @@ from typing import ClassVar, Optional from dbt.adapters.contracts.relation import RelationConfig -from dbt.adapters.events.types import AdapterEventWarning from dbt.adapters.relation_configs.config_base import RelationResults -from dbt_common.events.functions import warn_or_error from dbt.adapters.databricks.logging import logger +from dbt.adapters.databricks.persist_doc_column_warnings import ( + warn_missing_persist_doc_columns, +) from dbt.adapters.databricks.relation_configs.base import ( DatabricksComponentConfig, DatabricksComponentProcessor, @@ -33,15 +34,7 @@ def get_diff(self, other: "ColumnCommentsConfig") -> Optional["ColumnCommentsCon for column_name in self.comments if column_name.lower() not in other_comments_lower ] - if missing: - warn_or_error( - AdapterEventWarning( - base_msg=( - "The following columns are specified in the schema but are not present " - "in the database and will be skipped: " + ", ".join(missing) - ) - ) - ) + warn_missing_persist_doc_columns(missing) for column_name, comment in self.comments.items(): # Use case-insensitive comparison for column names diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index 94c87fcb1..555993d4a 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -462,7 +462,8 @@ def table_relation(self, project): def test_warns_and_still_comments_present_columns(self, adapter, table_relation): _, logs = util.run_dbt_and_capture(["run"]) assert "column_that_does_not_exist" in logs - # Emitted exactly once — the v1 and v2 warning sites are mutually exclusive per run. + # Emitted exactly once for this materialization + # (V1 table only hits get_persist_doc_columns). assert logs.count(_MISSING_COLUMN_WARNING) == 1 results = util.run_sql_with_adapter( @@ -534,3 +535,87 @@ def project_config_update(self): def test_warn_error_fails_run(self, project): _, logs = util.run_dbt_and_capture(["run", "--warn-error"], expect_pass=False) assert "column_that_does_not_exist" in logs + + +class TestPersistDocsColumnMissingWarnsV1ColumnsOnly: + """v1: columns-only persist_docs still warns (does not require relation: true).""" + + @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_warns_once_with_columns_only(self, adapter, table_relation): + _, logs = util.run_dbt_and_capture(["run"]) + assert "column_that_does_not_exist" in logs + assert logs.count(_MISSING_COLUMN_WARNING) == 1 + + 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") + + +class TestPersistDocsColumnMissingWarnsOnceV1IncrementalSubsequent: + """v1 incremental subsequent with both flags must warn exactly once. + + On V1 subsequent runs, get_changeset may call ColumnCommentsConfig.get_diff (warn) and + persist_docs later calls get_persist_doc_columns (warn). Those must not double-fire. + """ + + @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_subsequent_run_warns_exactly_once(self, project): + # First run creates via persist_docs → get_persist_doc_columns (warns once). + first_logs = util.run_dbt_and_capture(["run"])[1] + assert first_logs.count(_MISSING_COLUMN_WARNING) == 1 + + # Second run hits get_diff (changeset) AND persist_docs; must still be exactly once. + second_logs = util.run_dbt_and_capture(["run"])[1] + assert "column_that_does_not_exist" in second_logs + assert second_logs.count(_MISSING_COLUMN_WARNING) == 1, ( + f"Expected exactly 1 missing-column warning on V1 incremental subsequent, " + f"found {second_logs.count(_MISSING_COLUMN_WARNING)}. Logs:\n{second_logs}" + ) diff --git a/tests/unit/relation_configs/test_column_comments_config.py b/tests/unit/relation_configs/test_column_comments_config.py index 0f9948014..c7b528e30 100644 --- a/tests/unit/relation_configs/test_column_comments_config.py +++ b/tests/unit/relation_configs/test_column_comments_config.py @@ -2,6 +2,9 @@ from agate import Table +from dbt.adapters.databricks.persist_doc_column_warnings import ( + reset_missing_persist_doc_column_warnings, +) from dbt.adapters.databricks.relation_configs.column_comments import ( ColumnCommentsConfig, ColumnCommentsProcessor, @@ -103,9 +106,10 @@ def test_get_diff__case_mismatch_with_actual_changes(self): comments={"`account_id`": "New Account ID"}, persist=True ) - @patch("dbt.adapters.databricks.relation_configs.column_comments.warn_or_error") + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") def test_get_diff__warns_and_skips_missing_column(self, mock_warn): """Documented columns absent from the relation are warned about and skipped.""" + reset_missing_persist_doc_column_warnings() # col2 is documented but not present in the relation config = ColumnCommentsConfig( comments={"col1": "new comment", "col2": "comment for missing column"}, persist=True @@ -120,17 +124,19 @@ def test_get_diff__warns_and_skips_missing_column(self, mock_warn): assert "col2" in warned_event.base_msg assert "col1" not in warned_event.base_msg - @patch("dbt.adapters.databricks.relation_configs.column_comments.warn_or_error") + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") def test_get_diff__no_warning_when_all_present(self, mock_warn): """No warning is emitted when every documented column exists (case-insensitively).""" + reset_missing_persist_doc_column_warnings() config = ColumnCommentsConfig(comments={"account_id": "Account ID"}, persist=True) other = ColumnCommentsConfig(comments={"Account_ID": ""}) config.get_diff(other) mock_warn.assert_not_called() - @patch("dbt.adapters.databricks.relation_configs.column_comments.warn_or_error") + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") def test_get_diff__no_warning_when_not_persisting(self, mock_warn): """Missing columns are not evaluated (or warned about) when persist is False.""" + reset_missing_persist_doc_column_warnings() config = ColumnCommentsConfig(comments={"col1": "comment", "col2": "comment"}) other = ColumnCommentsConfig(comments={"col1": "comment"}) assert config.get_diff(other) is None diff --git a/tests/unit/test_adapter.py b/tests/unit/test_adapter.py index 2a4e948ae..44b5af4fd 100644 --- a/tests/unit/test_adapter.py +++ b/tests/unit/test_adapter.py @@ -34,6 +34,9 @@ ViewAPI, get_identifier_list_string, ) +from dbt.adapters.databricks.persist_doc_column_warnings import ( + reset_missing_persist_doc_column_warnings, +) from dbt.adapters.databricks.relation import ( DatabricksRelation, DatabricksRelationType, @@ -1182,9 +1185,10 @@ def test_get_persist_doc_columns_case_mismatch_no_update_needed(self, adapter): # No update needed since comments match assert result == {} - @patch("dbt.adapters.databricks.impl.warn_or_error") + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") def test_get_persist_doc_columns_warns_on_missing_column(self, mock_warn, adapter): """Documented columns absent from the relation are warned about and skipped.""" + reset_missing_persist_doc_column_warnings() existing = [self.create_column("col1", "comment1")] column_dict = { "col1": {"name": "col1", "description": "new comment"}, @@ -1199,9 +1203,10 @@ def test_get_persist_doc_columns_warns_on_missing_column(self, mock_warn, adapte assert "col2" in warned_event.base_msg assert "col1" not in warned_event.base_msg - @patch("dbt.adapters.databricks.impl.warn_or_error") + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") def test_get_persist_doc_columns_no_warning_when_all_present(self, mock_warn, adapter): """No warning is emitted when every documented column exists (case-insensitively).""" + reset_missing_persist_doc_column_warnings() existing = [self.create_column("Account_ID", "")] column_dict = {"account_id": {"name": "account_id", "description": "Account ID column"}} adapter.get_persist_doc_columns(existing, column_dict) diff --git a/tests/unit/test_persist_doc_column_warnings.py b/tests/unit/test_persist_doc_column_warnings.py new file mode 100644 index 000000000..523a3cdfa --- /dev/null +++ b/tests/unit/test_persist_doc_column_warnings.py @@ -0,0 +1,56 @@ +"""Unit tests for missing documented-column warning dedupe.""" + +import threading +from unittest.mock import patch + +from dbt.adapters.databricks.persist_doc_column_warnings import ( + reset_missing_persist_doc_column_warnings, + warn_missing_persist_doc_columns, +) +from dbt.adapters.databricks.relation_configs.column_comments import ColumnCommentsConfig + + +class TestWarnMissingPersistDocColumnsDedupe: + def setup_method(self) -> None: + reset_missing_persist_doc_column_warnings() + + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") + def test_warns_once_for_same_missing_set(self, mock_warn): + warn_missing_persist_doc_columns(["col2"]) + warn_missing_persist_doc_columns(["col2"]) + mock_warn.assert_called_once() + + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") + def test_get_diff_and_helper_share_dedupe(self, mock_warn): + """get_diff then the shared helper must not double-fire for the same cols.""" + config = ColumnCommentsConfig(comments={"col1": "new", "col2": "missing"}, persist=True) + other = ColumnCommentsConfig(comments={"col1": "old"}) + config.get_diff(other) + warn_missing_persist_doc_columns(["col2"]) + mock_warn.assert_called_once() + assert "col2" in mock_warn.call_args.args[0].base_msg + + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") + def test_different_missing_sets_warn_separately(self, mock_warn): + warn_missing_persist_doc_columns(["col2"]) + warn_missing_persist_doc_columns(["col3"]) + assert mock_warn.call_count == 2 + + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") + def test_thread_local_isolation(self, mock_warn): + """A reset on another thread must not suppress warnings on this thread.""" + warn_missing_persist_doc_columns(["col2"]) + assert mock_warn.call_count == 1 + + def other_thread() -> None: + reset_missing_persist_doc_column_warnings() + warn_missing_persist_doc_columns(["col2"]) + + t = threading.Thread(target=other_thread) + t.start() + t.join() + # Main thread already warned once; other thread warns independently → 2 total. + assert mock_warn.call_count == 2 + # Main thread still dedupes its own second call. + warn_missing_persist_doc_columns(["col2"]) + assert mock_warn.call_count == 2 From 653eb5cf5e6debd207bb256917369b7d16f0c932 Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Fri, 24 Jul 2026 14:31:22 +0530 Subject: [PATCH 08/16] chore: drop CHANGELOG tweak from single-warn follow-up --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98f156d27..64d146e0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Fixes -- Warn when a column documented in a model's `schema.yml` is absent from the relation while applying column comments, instead of silently skipping it — surfaces typos and stale column documentation. Covers the column-comment comparison paths (V1 `get_persist_doc_columns` and V2 `ColumnCommentsConfig.get_diff`); create-time inline comments are not yet covered. The warning is emitted at most once per unique missing-column set when both paths run in the same model (e.g. V1 incremental subsequent). Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). +- Warn when a column documented in a model's `schema.yml` is absent from the relation while applying column comments, instead of silently skipping it — surfaces typos and stale column documentation. Covers the column-comment comparison paths (V1 `get_persist_doc_columns` and V2 `ColumnCommentsConfig.get_diff`); create-time inline comments are not yet covered. Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). ### Under the Hood From 79176037a6c730720751512ecef060d28ae0f64a Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 28 Jul 2026 11:44:18 +0530 Subject: [PATCH 09/16] Validate persist_docs columns post-build; gate v2 column comments on persist_docs.columns --- CHANGELOG.md | 2 +- dbt/adapters/databricks/impl.py | 35 +++++-- .../relation_configs/column_comments.py | 24 ++--- .../macros/adapters/persist_docs.sql | 17 ++++ .../incremental/incremental.sql | 4 + .../macros/materializations/table.sql | 3 + .../adapter/persist_docs/test_persist_docs.py | 94 ++++++++++++++++--- .../test_column_comments_config.py | 42 +++++---- tests/unit/test_adapter.py | 39 ++++++++ .../unit/test_persist_doc_column_warnings.py | 8 +- 10 files changed, 213 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64d146e0a..1a1d8baea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Fixes -- Warn when a column documented in a model's `schema.yml` is absent from the relation while applying column comments, instead of silently skipping it — surfaces typos and stale column documentation. Covers the column-comment comparison paths (V1 `get_persist_doc_columns` and V2 `ColumnCommentsConfig.get_diff`); create-time inline comments are not yet covered. Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). +- Warn when a column documented in a model's `schema.yml` is absent from the relation, instead of silently skipping it — surfaces typos and stale column documentation. The check runs post-build against the actual relation (matching the shared `validate_doc_columns` behavior the other adapters use), so it covers V1 and V2 table/incremental on both the initial create and subsequent runs, does not false-warn on a legitimately new column, and is gated on `persist_docs.columns`. Also fixes the V2 column-comment gate to key off `persist_docs.columns` rather than `persist_docs.relation`. Materialized-view/streaming-table and view create are tracked as follow-ups. Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). ### Under the Hood diff --git a/dbt/adapters/databricks/impl.py b/dbt/adapters/databricks/impl.py index ac84fb14f..d8823a6db 100644 --- a/dbt/adapters/databricks/impl.py +++ b/dbt/adapters/databricks/impl.py @@ -1003,6 +1003,26 @@ def _catalog(self, catalog: Optional[str]) -> Iterator[None]: if current_catalog is not None: self.execute_macro(USE_CATALOG_MACRO_NAME, kwargs=dict(catalog=current_catalog)) + @staticmethod + def _find_missing_doc_columns( + existing_columns: list[DatabricksColumn], columns: dict[str, Any] + ) -> list[str]: + """Documented column names (from the model) that are absent from the relation.""" + existing_lower = {column.column.lower() for column in existing_columns} + return [name for name in columns if name.lower() not in existing_lower] + + @available.parse(lambda *a, **k: None) + def validate_persist_doc_columns( + self, existing_columns: list[DatabricksColumn], columns: dict[str, Any] + ) -> None: + """Warn about documented columns that are absent from the relation. + + Mirrors the shared ``validate_doc_columns`` behavior the other adapters use: the check runs + post-build against the actual relation, so a legitimately new column (present in the model + and the freshly-built relation) is not flagged. Applies no comments. + """ + warn_missing_persist_doc_columns(self._find_missing_doc_columns(existing_columns, columns)) + @available.parse(lambda *a, **k: {}) def get_persist_doc_columns( self, existing_columns: list[DatabricksColumn], columns: dict[str, Any] @@ -1017,16 +1037,11 @@ def get_persist_doc_columns( # Create a case-insensitive lookup for column names columns_lower = {k.lower(): k for k in columns.keys()} - # Warn about columns that are documented in the model's schema but are not present in the - # relation. These are silently skipped below (rather than erroring on the alter), so surface - # them to the user to catch typos and stale documentation. - existing_lower = {column.column.lower() for column in existing_columns} - missing = [ - original_name - for name_lower, original_name in columns_lower.items() - if name_lower not in existing_lower - ] - warn_missing_persist_doc_columns(missing) + # Documented-but-absent columns are skipped below (rather than erroring on the alter); warn + # so the user can catch typos and stale documentation. This runs post-build (V1 persist_docs + # gathers existing_columns from the written relation), so a legitimately new column is not + # flagged. + warn_missing_persist_doc_columns(self._find_missing_doc_columns(existing_columns, columns)) for column in existing_columns: name = column.column diff --git a/dbt/adapters/databricks/relation_configs/column_comments.py b/dbt/adapters/databricks/relation_configs/column_comments.py index 26f5d95f4..8c848a911 100644 --- a/dbt/adapters/databricks/relation_configs/column_comments.py +++ b/dbt/adapters/databricks/relation_configs/column_comments.py @@ -4,9 +4,6 @@ from dbt.adapters.relation_configs.config_base import RelationResults from dbt.adapters.databricks.logging import logger -from dbt.adapters.databricks.persist_doc_column_warnings import ( - warn_missing_persist_doc_columns, -) from dbt.adapters.databricks.relation_configs.base import ( DatabricksComponentConfig, DatabricksComponentProcessor, @@ -26,18 +23,12 @@ def get_diff(self, other: "ColumnCommentsConfig") -> Optional["ColumnCommentsCon # Create a case-insensitive lookup for other's column comments other_comments_lower = {k.lower(): v for k, v in other.comments.items()} - # Warn about columns that are documented in the model's schema but are not present in - # the relation. These are skipped below (rather than erroring on the alter), so surface - # them to the user to catch typos and stale documentation. - missing = [ - column_name - for column_name in self.comments - if column_name.lower() not in other_comments_lower - ] - warn_missing_persist_doc_columns(missing) - for column_name, comment in self.comments.items(): - # Use case-insensitive comparison for column names + # Use case-insensitive comparison for column names. Documented columns that are + # absent from the relation are skipped here so the alter never targets a nonexistent + # column; the user-facing "missing column" warning is emitted post-build by + # validate_persist_doc_columns (against the actual relation), so a legitimately new + # column is not flagged before it has been materialized. if column_name.lower() not in other_comments_lower: continue other_comment = other_comments_lower.get(column_name.lower()) @@ -68,7 +59,10 @@ 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 + # Column comments are gated on persist_docs.columns (the column-level knob), matching + # config.persist_column_docs() used by the V1 persist_docs / view-create / seed paths. + # persist_docs.relation is the table-comment knob and is the wrong gate here. + 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..029c2d5f9 100644 --- a/dbt/include/databricks/macros/adapters/persist_docs.sql +++ b/dbt/include/databricks/macros/adapters/persist_docs.sql @@ -42,6 +42,23 @@ {% endif %} {% endmacro %} +{#-- + Post-build validation of documented column comments against the actual relation. + + The V2 materialization path applies column comments inline at create-time and via the + relation-config diff (neither of which sees the model's documented columns as a set), so this + runs after the relation is built to surface columns that are documented in the schema but absent + from the relation (typos / stale docs). It mirrors the shared validate_doc_columns behavior the + other adapters use, and applies no comments itself. Gated on persist_docs.columns so it never + fires when column persistence is disabled (avoids --warn-error false failures). +--#} +{% macro validate_persist_doc_columns(relation, model) -%} + {% if config.persist_column_docs() and model.columns %} + {%- set existing_columns = adapter.get_columns_in_relation(relation) -%} + {%- do adapter.validate_persist_doc_columns(existing_columns, model.columns) -%} + {% endif %} +{%- endmacro %} + {% macro alter_relation_comment_sql(relation, description) %} COMMENT ON {{ relation.type.render().upper() }} {{ relation.render() }} IS '{{ description | replace("'", "\\'") }}' {% endmacro %} diff --git a/dbt/include/databricks/macros/materializations/incremental/incremental.sql b/dbt/include/databricks/macros/materializations/incremental/incremental.sql index f226344b9..fe72f7a38 100644 --- a/dbt/include/databricks/macros/materializations/incremental/incremental.sql +++ b/dbt/include/databricks/macros/materializations/incremental/incremental.sql @@ -81,6 +81,10 @@ {%- endif -%} {%- endif -%} + {#-- Warn (post-build) about documented columns absent from the final relation. Runs on every + sub-branch above and regardless of incremental_apply_config_changes. --#} + {% 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..d1d9fdc27 100644 --- a/dbt/include/databricks/macros/materializations/table.sql +++ b/dbt/include/databricks/macros/materializations/table.sql @@ -32,6 +32,9 @@ {% endif %} {% endif %} + {#-- Warn (post-build) about documented columns absent from the final relation. --#} + {% 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/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index 555993d4a..601f4d1df 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -478,13 +478,12 @@ def test_warns_and_still_comments_present_columns(self, adapter, table_relation) class TestPersistDocsColumnMissingWarnsV2: - """v2: the warning surfaces on the alter path (ColumnCommentsConfig.get_diff). + """v2: a documented column absent from the relation is warned about post-build. - Uses an incremental model: a table rebuild re-applies comments inline and never consults - get_diff, so the changeset path is only reached on a subsequent incremental run, when - documented columns are diffed against the existing relation. On first create the - documented-but-absent column is silently dropped inline. (Create-time warning is tracked as a - follow-up.) + The warning is emitted by validate_persist_doc_columns after the relation is built (mirroring + the shared validate_doc_columns behavior the other adapters use), not from the + pre-materialization changeset diff. So it fires on the initial create and on every subsequent + incremental run — and a legitimately new column (present post-build) would not warn. """ @pytest.fixture(scope="class") @@ -502,18 +501,67 @@ def project_config_update(self): "models": {"test": {"+persist_docs": {"relation": True, "columns": True}}}, } - def test_warns_on_second_run(self, project): - # First run creates the relation; the inline-comment create path does not warn. + def test_warns_on_create_and_subsequent_runs(self, project): + # Post-build validation warns on the initial create... first_logs = util.run_dbt_and_capture(["run"])[1] - assert _MISSING_COLUMN_WARNING not in first_logs + assert "column_that_does_not_exist" in first_logs + assert first_logs.count(_MISSING_COLUMN_WARNING) == 1 - # Second run diffs documented columns against the existing relation → warns exactly once - # (get_diff runs once per component in a single get_changeset). + # ...and again on a subsequent incremental run, still exactly once (no double-warn). second_logs = util.run_dbt_and_capture(["run"])[1] assert "column_that_does_not_exist" in second_logs assert second_logs.count(_MISSING_COLUMN_WARNING) == 1 +class TestPersistDocsColumnMissingWarnsV2ColumnsOnly: + """v2 columns-only persist_docs warns (E2: column comments gate on persist_docs.columns, + not .relation — previously this combination was silent on the v2 path).""" + + @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_warns_with_columns_only(self, project): + _, logs = util.run_dbt_and_capture(["run"]) + assert "column_that_does_not_exist" in logs + assert logs.count(_MISSING_COLUMN_WARNING) == 1 + + +class TestPersistDocsColumnMissingV2RelationOnlyNoWarn: + """v2 with columns:false does no column-doc work, so the missing-column check stays silent + (E2: column comments are gated on persist_docs.columns).""" + + @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): + _, logs = util.run_dbt_and_capture(["run"]) + assert _MISSING_COLUMN_WARNING not in logs + + class TestPersistDocsColumnMissingWarnError: """--warn-error escalates the missing-column warning to a run failure (v1 path).""" @@ -537,6 +585,30 @@ def test_warn_error_fails_run(self, project): assert "column_that_does_not_exist" in logs +class TestPersistDocsColumnMissingWarnErrorV2: + """--warn-error escalates the post-build missing-column warning to a run failure (v2 path).""" + + @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_warn_error_fails_run(self, project): + # V2 create goes through the post-build validation; --warn-error must fail the run. + _, logs = util.run_dbt_and_capture(["run", "--warn-error"], expect_pass=False) + assert "column_that_does_not_exist" in logs + + class TestPersistDocsColumnMissingWarnsV1ColumnsOnly: """v1: columns-only persist_docs still warns (does not require relation: true).""" diff --git a/tests/unit/relation_configs/test_column_comments_config.py b/tests/unit/relation_configs/test_column_comments_config.py index c7b528e30..c56aaf514 100644 --- a/tests/unit/relation_configs/test_column_comments_config.py +++ b/tests/unit/relation_configs/test_column_comments_config.py @@ -48,10 +48,27 @@ 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} + # Column comments are gated on persist_docs.columns, not .relation. + 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): + """persist_docs.columns drives column comments even when .relation is false.""" + 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): + """Column comments are not applied when .columns is off, even if .relation is on.""" + 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): @@ -107,8 +124,13 @@ def test_get_diff__case_mismatch_with_actual_changes(self): ) @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_diff__warns_and_skips_missing_column(self, mock_warn): - """Documented columns absent from the relation are warned about and skipped.""" + def test_get_diff__skips_missing_column_without_warning(self, mock_warn): + """Documented columns absent from the relation are skipped; get_diff does not warn. + + The missing-column warning now runs post-build (validate_persist_doc_columns), so the + pre-materialization diff must stay silent to avoid false-warning on a legitimately new + column. + """ reset_missing_persist_doc_column_warnings() # col2 is documented but not present in the relation config = ColumnCommentsConfig( @@ -118,19 +140,7 @@ def test_get_diff__warns_and_skips_missing_column(self, mock_warn): diff = config.get_diff(other) # Only the existing column is included in the diff; the missing one is skipped. assert diff == ColumnCommentsConfig(comments={"`col1`": "new comment"}, persist=True) - # A warning is emitted naming the missing column. - mock_warn.assert_called_once() - warned_event = mock_warn.call_args.args[0] - assert "col2" in warned_event.base_msg - assert "col1" not in warned_event.base_msg - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_diff__no_warning_when_all_present(self, mock_warn): - """No warning is emitted when every documented column exists (case-insensitively).""" - reset_missing_persist_doc_column_warnings() - config = ColumnCommentsConfig(comments={"account_id": "Account ID"}, persist=True) - other = ColumnCommentsConfig(comments={"Account_ID": ""}) - config.get_diff(other) + # No warning is emitted from the diff path. mock_warn.assert_not_called() @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") diff --git a/tests/unit/test_adapter.py b/tests/unit/test_adapter.py index 44b5af4fd..f8c8d193e 100644 --- a/tests/unit/test_adapter.py +++ b/tests/unit/test_adapter.py @@ -1212,6 +1212,45 @@ def test_get_persist_doc_columns_no_warning_when_all_present(self, mock_warn, ad adapter.get_persist_doc_columns(existing, column_dict) mock_warn.assert_not_called() + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") + def test_validate_persist_doc_columns_warns_on_missing_column(self, mock_warn, adapter): + """A documented column absent from the (post-build) relation is warned about.""" + reset_missing_persist_doc_column_warnings() + existing = [self.create_column("col1", "comment1")] + column_dict = { + "col1": {"name": "col1", "description": "comment1"}, + "col2": {"name": "col2", "description": "typo / stale doc"}, + } + assert adapter.validate_persist_doc_columns(existing, column_dict) is None + mock_warn.assert_called_once() + warned_event = mock_warn.call_args.args[0] + assert "col2" in warned_event.base_msg + assert "col1" not in warned_event.base_msg + + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") + def test_validate_persist_doc_columns_no_warning_for_newly_added_column( + self, mock_warn, adapter + ): + """A legitimately new column is present in the freshly built relation post-build, so the + post-build check does not false-warn about it (the E1 regression).""" + reset_missing_persist_doc_column_warnings() + # col2 was just added to the model; post-build it exists in the relation. + existing = [self.create_column("col1", "c1"), self.create_column("col2", "c2")] + column_dict = { + "col1": {"name": "col1", "description": "c1"}, + "col2": {"name": "col2", "description": "c2"}, + } + adapter.validate_persist_doc_columns(existing, column_dict) + mock_warn.assert_not_called() + + @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") + def test_validate_persist_doc_columns_case_insensitive(self, mock_warn, adapter): + reset_missing_persist_doc_column_warnings() + existing = [self.create_column("Account_ID", "")] + column_dict = {"account_id": {"name": "account_id", "description": "Account ID"}} + adapter.validate_persist_doc_columns(existing, column_dict) + mock_warn.assert_not_called() + class TestGetColumnsByDbrVersion(DatabricksAdapterBase): @pytest.fixture diff --git a/tests/unit/test_persist_doc_column_warnings.py b/tests/unit/test_persist_doc_column_warnings.py index 523a3cdfa..eeee4aa23 100644 --- a/tests/unit/test_persist_doc_column_warnings.py +++ b/tests/unit/test_persist_doc_column_warnings.py @@ -21,11 +21,15 @@ def test_warns_once_for_same_missing_set(self, mock_warn): mock_warn.assert_called_once() @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_diff_and_helper_share_dedupe(self, mock_warn): - """get_diff then the shared helper must not double-fire for the same cols.""" + def test_get_diff_does_not_warn_helper_still_dedupes(self, mock_warn): + """get_diff is silent (warning moved post-build); the shared helper still dedupes.""" config = ColumnCommentsConfig(comments={"col1": "new", "col2": "missing"}, persist=True) other = ColumnCommentsConfig(comments={"col1": "old"}) config.get_diff(other) + # The pre-materialization diff no longer contributes a warning. + mock_warn.assert_not_called() + # The post-build path warns once per unique set. + warn_missing_persist_doc_columns(["col2"]) warn_missing_persist_doc_columns(["col2"]) mock_warn.assert_called_once() assert "col2" in mock_warn.call_args.args[0].base_msg From e25456f11944a7e4df4af59fc5b89e959b1eae72 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 28 Jul 2026 12:33:46 +0530 Subject: [PATCH 10/16] Trim what-comments at persist_docs validation call sites --- .../macros/materializations/incremental/incremental.sql | 4 ++-- dbt/include/databricks/macros/materializations/table.sql | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/dbt/include/databricks/macros/materializations/incremental/incremental.sql b/dbt/include/databricks/macros/materializations/incremental/incremental.sql index fe72f7a38..7adbd20da 100644 --- a/dbt/include/databricks/macros/materializations/incremental/incremental.sql +++ b/dbt/include/databricks/macros/materializations/incremental/incremental.sql @@ -81,8 +81,8 @@ {%- endif -%} {%- endif -%} - {#-- Warn (post-build) about documented columns absent from the final relation. Runs on every - sub-branch above and regardless of incremental_apply_config_changes. --#} + {#-- Placed here so it runs on every sub-branch above (create/replace/merge) and regardless of + incremental_apply_config_changes. --#} {% do validate_persist_doc_columns(target_relation, model) %} {% set should_revoke = should_revoke(existing_relation, full_refresh_mode) %} diff --git a/dbt/include/databricks/macros/materializations/table.sql b/dbt/include/databricks/macros/materializations/table.sql index d1d9fdc27..f0f165255 100644 --- a/dbt/include/databricks/macros/materializations/table.sql +++ b/dbt/include/databricks/macros/materializations/table.sql @@ -32,7 +32,6 @@ {% endif %} {% endif %} - {#-- Warn (post-build) about documented columns absent from the final relation. --#} {% do validate_persist_doc_columns(target_relation, model) %} {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %} From 9750b13fb850785801bde48511d5737d6a310f8a Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Wed, 5 Aug 2026 17:46:08 +0530 Subject: [PATCH 11/16] test: harden persist-doc column coverage --- .../adapter/persist_docs/fixtures.py | 60 +++++ .../adapter/persist_docs/test_persist_docs.py | 246 +++++++++++------- 2 files changed, 217 insertions(+), 89 deletions(-) diff --git a/tests/functional/adapter/persist_docs/fixtures.py b/tests/functional/adapter/persist_docs/fixtures.py index af2aa3141..0d7b3a1b5 100644 --- a/tests/functional/adapter/persist_docs/fixtures.py +++ b/tests/functional/adapter/persist_docs/fixtures.py @@ -67,3 +67,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 601f4d1df..e65264190 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -421,19 +421,11 @@ def test_column_comment_suppressed_when_columns_false(self, adapter, table_relat ) -# Tail of the warning emitted when a documented column is absent from the relation. The offending -# column name (column_that_does_not_exist, from fixtures._PROPERTIES__SCHEMA_MISSING_COL) is -# asserted separately. -_MISSING_COLUMN_WARNING = "not present in the database and will be skipped" +_ADAPTER_WARNING_ERROR_OPTIONS = '{"error": ["AdapterEventWarning"]}' class TestPersistDocsColumnMissingWarnsV1: - """v1: a documented column absent from the relation is warned about, not silently skipped. - - Complements TestPersistDocsColumnMissing (which only checks the run survives) by asserting the - warning names the offending column on the v1 comment path - (DatabricksAdapter.get_persist_doc_columns), and that present columns are still commented. - """ + """V1 persists comments for present columns and emits an adapter warning for missing ones.""" @pytest.fixture(scope="class") def models(self): @@ -460,11 +452,7 @@ def table_relation(self, project): ) def test_warns_and_still_comments_present_columns(self, adapter, table_relation): - _, logs = util.run_dbt_and_capture(["run"]) - assert "column_that_does_not_exist" in logs - # Emitted exactly once for this materialization - # (V1 table only hits get_persist_doc_columns). - assert logs.count(_MISSING_COLUMN_WARNING) == 1 + util.run_dbt(["run"]) results = util.run_sql_with_adapter( adapter, f"describe extended {table_relation}", fetch="all" @@ -476,6 +464,10 @@ def test_warns_and_still_comments_present_columns(self, adapter, table_relation) 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", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ) + class TestPersistDocsColumnMissingWarnsV2: """v2: a documented column absent from the relation is warned about post-build. @@ -501,16 +493,27 @@ def project_config_update(self): "models": {"test": {"+persist_docs": {"relation": True, "columns": True}}}, } - def test_warns_on_create_and_subsequent_runs(self, project): - # Post-build validation warns on the initial create... - first_logs = util.run_dbt_and_capture(["run"])[1] - assert "column_that_does_not_exist" in first_logs - assert first_logs.count(_MISSING_COLUMN_WARNING) == 1 + def test_warning_escalates_on_create_and_subsequent_runs(self, project, adapter): + util.run_dbt( + ["run", "--warn-error-options", _ADAPTER_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" - # ...and again on a subsequent incremental run, still exactly once (no double-warn). - second_logs = util.run_dbt_and_capture(["run"])[1] - assert "column_that_does_not_exist" in second_logs - assert second_logs.count(_MISSING_COLUMN_WARNING) == 1 + util.run_dbt( + ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ) class TestPersistDocsColumnMissingWarnsV2ColumnsOnly: @@ -532,10 +535,23 @@ def project_config_update(self): "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, } - def test_warns_with_columns_only(self, project): - _, logs = util.run_dbt_and_capture(["run"]) - assert "column_that_does_not_exist" in logs - assert logs.count(_MISSING_COLUMN_WARNING) == 1 + def test_warning_escalates_with_columns_only(self, project, adapter): + util.run_dbt( + ["run", "--warn-error-options", _ADAPTER_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: @@ -558,12 +574,11 @@ def project_config_update(self): } def test_no_warning_when_columns_disabled(self, project): - _, logs = util.run_dbt_and_capture(["run"]) - assert _MISSING_COLUMN_WARNING not in logs + util.run_dbt(["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS]) -class TestPersistDocsColumnMissingWarnError: - """--warn-error escalates the missing-column warning to a run failure (v1 path).""" +class TestPersistDocsColumnMissingWarnsV1ColumnsOnly: + """v1: columns-only persist_docs still warns (does not require relation: true).""" @pytest.fixture(scope="class") def models(self): @@ -577,16 +592,38 @@ def properties(self): def project_config_update(self): return { "flags": {"use_materialization_v2": False}, - "models": {"test": {"+persist_docs": {"relation": True, "columns": True}}}, + "models": {"test": {"+persist_docs": {"relation": False, "columns": True}}}, } - def test_warn_error_fails_run(self, project): - _, logs = util.run_dbt_and_capture(["run", "--warn-error"], expect_pass=False) - assert "column_that_does_not_exist" in logs + @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", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ) -class TestPersistDocsColumnMissingWarnErrorV2: - """--warn-error escalates the post-build missing-column warning to a run failure (v2 path).""" +class TestPersistDocsColumnMissingWarnsV1IncrementalSubsequent: + """V1 incremental runs continue to surface missing documented columns.""" @pytest.fixture(scope="class") def models(self): @@ -599,95 +636,126 @@ def properties(self): @pytest.fixture(scope="class") def project_config_update(self): return { - "flags": {"use_materialization_v2": True}, - "models": {"test": {"+persist_docs": {"relation": True, "columns": True}}}, + "flags": {"use_materialization_v2": False}, + "models": { + "test": { + "+persist_docs": {"relation": True, "columns": True}, + "+incremental_apply_config_changes": True, + } + }, } - def test_warn_error_fails_run(self, project): - # V2 create goes through the post-build validation; --warn-error must fail the run. - _, logs = util.run_dbt_and_capture(["run", "--warn-error"], expect_pass=False) - assert "column_that_does_not_exist" in logs - + def test_warning_escalates_on_subsequent_run(self, project): + util.run_dbt( + ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ) + util.run_dbt( + ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ) -class TestPersistDocsColumnMissingWarnsV1ColumnsOnly: - """v1: columns-only persist_docs still warns (does not require relation: true).""" +class TestPersistDocsPlannedColumnV1Incremental: @pytest.fixture(scope="class") def models(self): - return {"missing_column.sql": fixtures._MODELS__MISSING_COLUMN} + return { + "schema_change_incremental.sql": ( + override_fixtures.schema_change_incremental_initial_sql + ) + } @pytest.fixture(scope="class") def properties(self): - return {"schema.yml": fixtures._PROPERTIES__SCHEMA_MISSING_COL} + 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}}}, + "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_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", ) - def test_warns_once_with_columns_only(self, adapter, table_relation): - _, logs = util.run_dbt_and_capture(["run"]) - assert "column_that_does_not_exist" in logs - assert logs.count(_MISSING_COLUMN_WARNING) == 1 + util.run_dbt(["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS]) - results = util.run_sql_with_adapter( - adapter, f"describe extended {table_relation}", fetch="all" + 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( - table_relation, Table(results, ["col_name", "data_type", "comment"]) + relation, Table(rows, ["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") - - -class TestPersistDocsColumnMissingWarnsOnceV1IncrementalSubsequent: - """v1 incremental subsequent with both flags must warn exactly once. + comments = {column.column: column.comment for column in columns} + assert comments["new_col"] == "new column comment" - On V1 subsequent runs, get_changeset may call ColumnCommentsConfig.get_diff (warn) and - persist_docs later calls get_persist_doc_columns (warn). Those must not double-fire. - """ +class TestPersistDocsPlannedColumnV2AlterView: @pytest.fixture(scope="class") def models(self): - return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} + return {"alter_view.sql": override_fixtures.alter_view_initial_sql} @pytest.fixture(scope="class") def properties(self): - return {"schema.yml": override_fixtures.missing_column_incremental_schema} + return {"schema.yml": override_fixtures.alter_view_initial_yml} @pytest.fixture(scope="class") def project_config_update(self): return { - "flags": {"use_materialization_v2": False}, + "flags": {"use_materialization_v2": True}, "models": { "test": { - "+persist_docs": {"relation": True, "columns": True}, - "+incremental_apply_config_changes": True, + "+persist_docs": {"relation": False, "columns": True}, } }, } - def test_subsequent_run_warns_exactly_once(self, project): - # First run creates via persist_docs → get_persist_doc_columns (warns once). - first_logs = util.run_dbt_and_capture(["run"])[1] - assert first_logs.count(_MISSING_COLUMN_WARNING) == 1 + 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", + ) - # Second run hits get_diff (changeset) AND persist_docs; must still be exactly once. - second_logs = util.run_dbt_and_capture(["run"])[1] - assert "column_that_does_not_exist" in second_logs - assert second_logs.count(_MISSING_COLUMN_WARNING) == 1, ( - f"Expected exactly 1 missing-column warning on V1 incremental subsequent, " - f"found {second_logs.count(_MISSING_COLUMN_WARNING)}. Logs:\n{second_logs}" + util.run_dbt(["run", "--warn-error-options", _ADAPTER_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" From 6b79044a5dfff257dee404fe26aea06364fd8dc9 Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Wed, 5 Aug 2026 19:11:42 +0530 Subject: [PATCH 12/16] refactor: simplify persist docs column validation --- dbt/adapters/databricks/impl.py | 37 +--------- .../databricks/persist_doc_column_warnings.py | 54 --------------- .../relation_configs/column_comments.py | 3 +- .../macros/adapters/persist_docs.sql | 33 ++++++++- .../adapter/persist_docs/fixtures.py | 4 +- .../adapter/persist_docs/test_persist_docs.py | 31 ++++----- .../adapters/test_persist_docs_macros.py | 60 ++++++++++++++++ .../test_column_comments_config.py | 28 +------- tests/unit/test_adapter.py | 69 ------------------- .../unit/test_persist_doc_column_warnings.py | 60 ---------------- 10 files changed, 110 insertions(+), 269 deletions(-) delete mode 100644 dbt/adapters/databricks/persist_doc_column_warnings.py delete mode 100644 tests/unit/test_persist_doc_column_warnings.py diff --git a/dbt/adapters/databricks/impl.py b/dbt/adapters/databricks/impl.py index d8823a6db..3e1069417 100644 --- a/dbt/adapters/databricks/impl.py +++ b/dbt/adapters/databricks/impl.py @@ -3,7 +3,7 @@ import re from abc import ABC, abstractmethod from collections import defaultdict -from collections.abc import Iterable, Iterator, Mapping +from collections.abc import Iterable, Iterator from concurrent.futures import Future from contextlib import contextmanager from dataclasses import dataclass, field @@ -57,10 +57,6 @@ from dbt.adapters.databricks.global_state import GlobalState from dbt.adapters.databricks.handle import SqlUtils from dbt.adapters.databricks.logging import logger -from dbt.adapters.databricks.persist_doc_column_warnings import ( - reset_missing_persist_doc_column_warnings, - warn_missing_persist_doc_columns, -) from dbt.adapters.databricks.python_models.python_submissions import ( AllPurposeClusterPythonJobHelper, JobClusterPythonJobHelper, @@ -906,11 +902,6 @@ def get_behavior_flag_no_warn(self, behavior_flag_name: str) -> bool: behavior_flag = getattr(self.behavior, behavior_flag_name) return behavior_flag.no_warn - def pre_model_hook(self, config: Mapping[str, Any]) -> Any: - """Reset missing-column warn dedupe so each model materialization can warn once.""" - reset_missing_persist_doc_column_warnings() - return super().pre_model_hook(config) - @available.parse(lambda *a, **k: (None, None)) @record_function( DatabricksAdapterAddQueryRecord, @@ -1003,26 +994,6 @@ def _catalog(self, catalog: Optional[str]) -> Iterator[None]: if current_catalog is not None: self.execute_macro(USE_CATALOG_MACRO_NAME, kwargs=dict(catalog=current_catalog)) - @staticmethod - def _find_missing_doc_columns( - existing_columns: list[DatabricksColumn], columns: dict[str, Any] - ) -> list[str]: - """Documented column names (from the model) that are absent from the relation.""" - existing_lower = {column.column.lower() for column in existing_columns} - return [name for name in columns if name.lower() not in existing_lower] - - @available.parse(lambda *a, **k: None) - def validate_persist_doc_columns( - self, existing_columns: list[DatabricksColumn], columns: dict[str, Any] - ) -> None: - """Warn about documented columns that are absent from the relation. - - Mirrors the shared ``validate_doc_columns`` behavior the other adapters use: the check runs - post-build against the actual relation, so a legitimately new column (present in the model - and the freshly-built relation) is not flagged. Applies no comments. - """ - warn_missing_persist_doc_columns(self._find_missing_doc_columns(existing_columns, columns)) - @available.parse(lambda *a, **k: {}) def get_persist_doc_columns( self, existing_columns: list[DatabricksColumn], columns: dict[str, Any] @@ -1037,12 +1008,6 @@ def get_persist_doc_columns( # Create a case-insensitive lookup for column names columns_lower = {k.lower(): k for k in columns.keys()} - # Documented-but-absent columns are skipped below (rather than erroring on the alter); warn - # so the user can catch typos and stale documentation. This runs post-build (V1 persist_docs - # gathers existing_columns from the written relation), so a legitimately new column is not - # flagged. - warn_missing_persist_doc_columns(self._find_missing_doc_columns(existing_columns, columns)) - for column in existing_columns: name = column.column # Use case-insensitive comparison for column names diff --git a/dbt/adapters/databricks/persist_doc_column_warnings.py b/dbt/adapters/databricks/persist_doc_column_warnings.py deleted file mode 100644 index 118aaca2e..000000000 --- a/dbt/adapters/databricks/persist_doc_column_warnings.py +++ /dev/null @@ -1,54 +0,0 @@ -"""Shared warning for documented columns absent from the relation. - -Both the V1 persist_docs helper (``get_persist_doc_columns``) and the V2 changeset -helper (``ColumnCommentsConfig.get_diff``) need to surface the same user-facing -warning. On V1 incremental subsequent runs those two paths can both execute in a -single model materialization; dedupe so the message appears exactly once per -unique missing set **within that materialization**. - -State is thread-local: dbt assigns each model to one worker thread, so parallel -runs do not clear or suppress each other's warnings. ``pre_model_hook`` resets -the cache at the start of each model on that thread. -""" - -from __future__ import annotations - -import threading -from collections.abc import Sequence - -from dbt.adapters.events.types import AdapterEventWarning -from dbt_common.events.functions import warn_or_error - -_thread_state = threading.local() - - -def _emitted_keys() -> set[str]: - keys = getattr(_thread_state, "emitted_missing_keys", None) - if keys is None: - keys = set() - _thread_state.emitted_missing_keys = keys - return keys - - -def reset_missing_persist_doc_column_warnings() -> None: - """Clear the per-materialization dedupe cache for the current thread.""" - _thread_state.emitted_missing_keys = set() - - -def warn_missing_persist_doc_columns(missing: Sequence[str]) -> None: - """Warn once per unique set of documented-but-absent column names (per thread).""" - if not missing: - return - key = ", ".join(sorted(missing, key=str.lower)) - emitted = _emitted_keys() - if key in emitted: - return - emitted.add(key) - warn_or_error( - AdapterEventWarning( - base_msg=( - "The following columns are specified in the schema but are not present " - "in the database and will be skipped: " + ", ".join(missing) - ) - ) - ) diff --git a/dbt/adapters/databricks/relation_configs/column_comments.py b/dbt/adapters/databricks/relation_configs/column_comments.py index 8c848a911..9e84c4565 100644 --- a/dbt/adapters/databricks/relation_configs/column_comments.py +++ b/dbt/adapters/databricks/relation_configs/column_comments.py @@ -26,8 +26,7 @@ def get_diff(self, other: "ColumnCommentsConfig") -> Optional["ColumnCommentsCon for column_name, comment in self.comments.items(): # Use case-insensitive comparison for column names. Documented columns that are # absent from the relation are skipped here so the alter never targets a nonexistent - # column; the user-facing "missing column" warning is emitted post-build by - # validate_persist_doc_columns (against the actual relation), so a legitimately new + # column; Jinja validation warns after the relation is built, so a legitimately new # column is not flagged before it has been materialized. if column_name.lower() not in other_comments_lower: continue diff --git a/dbt/include/databricks/macros/adapters/persist_docs.sql b/dbt/include/databricks/macros/adapters/persist_docs.sql index 029c2d5f9..50157fe66 100644 --- a/dbt/include/databricks/macros/adapters/persist_docs.sql +++ b/dbt/include/databricks/macros/adapters/persist_docs.sql @@ -37,11 +37,37 @@ {% 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 %} +{#-- + Warn about documented columns absent from a materialized relation and return only the columns + that are present. Column names are matched case-insensitively, consistent with Databricks. +--#} +{% 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 %} + {#-- Post-build validation of documented column comments against the actual relation. @@ -55,7 +81,8 @@ {% macro validate_persist_doc_columns(relation, model) -%} {% if config.persist_column_docs() and model.columns %} {%- set existing_columns = adapter.get_columns_in_relation(relation) -%} - {%- do adapter.validate_persist_doc_columns(existing_columns, model.columns) -%} + {%- set existing_column_names = existing_columns | map(attribute='name') | list -%} + {%- do dbt_databricks_validate_doc_columns(relation, model.columns, existing_column_names) -%} {% endif %} {%- endmacro %} @@ -70,4 +97,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/tests/functional/adapter/persist_docs/fixtures.py b/tests/functional/adapter/persist_docs/fixtures.py index 0d7b3a1b5..0ca59e573 100644 --- a/tests/functional/adapter/persist_docs/fixtures.py +++ b/tests/functional/adapter/persist_docs/fixtures.py @@ -39,9 +39,7 @@ select 1 as id, 'alice' as name """ -# Incremental model whose schema documents a column absent from the relation. Used to exercise the -# V2 alter/changeset path (ColumnCommentsConfig.get_diff), which — unlike a table rebuild — is only -# reached on a subsequent run against an existing relation. +# Incremental model whose schema documents a column absent from the materialized relation. missing_column_incremental_sql = """ {{ config(materialized='incremental') }} select 1 as id, 'Ed' as name diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index e65264190..066e202df 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -421,11 +421,11 @@ def test_column_comment_suppressed_when_columns_false(self, adapter, table_relat ) -_ADAPTER_WARNING_ERROR_OPTIONS = '{"error": ["AdapterEventWarning"]}' +_JINJA_WARNING_ERROR_OPTIONS = '{"error": ["JinjaLogWarning"]}' class TestPersistDocsColumnMissingWarnsV1: - """V1 persists comments for present columns and emits an adapter warning for missing ones.""" + """V1 persists comments for present columns and warns for missing ones.""" @pytest.fixture(scope="class") def models(self): @@ -465,17 +465,16 @@ def test_warns_and_still_comments_present_columns(self, adapter, table_relation) assert id_columns[0].comment.startswith("test id column description") util.run_dbt( - ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False ) class TestPersistDocsColumnMissingWarnsV2: """v2: a documented column absent from the relation is warned about post-build. - The warning is emitted by validate_persist_doc_columns after the relation is built (mirroring - the shared validate_doc_columns behavior the other adapters use), not from the - pre-materialization changeset diff. So it fires on the initial create and on every subsequent - incremental run — and a legitimately new column (present post-build) would not warn. + The warning is emitted by validate_persist_doc_columns after the relation is built, not from + the pre-materialization changeset diff. So it fires on the initial create and on every + subsequent incremental run — and a legitimately new column (present post-build) does not warn. """ @pytest.fixture(scope="class") @@ -495,7 +494,7 @@ def project_config_update(self): def test_warning_escalates_on_create_and_subsequent_runs(self, project, adapter): util.run_dbt( - ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False ) relation = DatabricksRelation.create( @@ -512,7 +511,7 @@ def test_warning_escalates_on_create_and_subsequent_runs(self, project, adapter) assert comments["id"] == "test id column description" util.run_dbt( - ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False ) @@ -537,7 +536,7 @@ def project_config_update(self): def test_warning_escalates_with_columns_only(self, project, adapter): util.run_dbt( - ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False ) relation = DatabricksRelation.create( @@ -574,7 +573,7 @@ def project_config_update(self): } def test_no_warning_when_columns_disabled(self, project): - util.run_dbt(["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS]) + util.run_dbt(["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS]) class TestPersistDocsColumnMissingWarnsV1ColumnsOnly: @@ -618,7 +617,7 @@ def test_warning_escalates_with_columns_only(self, adapter, table_relation): assert id_columns[0].comment.startswith("test id column description") util.run_dbt( - ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False ) @@ -647,10 +646,10 @@ def project_config_update(self): def test_warning_escalates_on_subsequent_run(self, project): util.run_dbt( - ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False ) util.run_dbt( - ["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS], expect_pass=False + ["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS], expect_pass=False ) @@ -693,7 +692,7 @@ def test_new_documented_column_is_not_warned_before_schema_sync(self, project, a "schema.yml", ) - util.run_dbt(["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS]) + util.run_dbt(["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS]) relation = DatabricksRelation.create( database=project.database, @@ -744,7 +743,7 @@ def test_new_documented_column_is_not_warned_before_alter_view(self, project, ad "schema.yml", ) - util.run_dbt(["run", "--warn-error-options", _ADAPTER_WARNING_ERROR_OPTIONS]) + util.run_dbt(["run", "--warn-error-options", _JINJA_WARNING_ERROR_OPTIONS]) relation = DatabricksRelation.create( database=project.database, 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 c56aaf514..1272bd8ad 100644 --- a/tests/unit/relation_configs/test_column_comments_config.py +++ b/tests/unit/relation_configs/test_column_comments_config.py @@ -1,10 +1,7 @@ -from unittest.mock import Mock, patch +from unittest.mock import Mock from agate import Table -from dbt.adapters.databricks.persist_doc_column_warnings import ( - reset_missing_persist_doc_column_warnings, -) from dbt.adapters.databricks.relation_configs.column_comments import ( ColumnCommentsConfig, ColumnCommentsProcessor, @@ -123,31 +120,10 @@ def test_get_diff__case_mismatch_with_actual_changes(self): comments={"`account_id`": "New Account ID"}, persist=True ) - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_diff__skips_missing_column_without_warning(self, mock_warn): - """Documented columns absent from the relation are skipped; get_diff does not warn. - - The missing-column warning now runs post-build (validate_persist_doc_columns), so the - pre-materialization diff must stay silent to avoid false-warning on a legitimately new - column. - """ - reset_missing_persist_doc_column_warnings() - # col2 is documented but not present in the relation + 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) - # Only the existing column is included in the diff; the missing one is skipped. assert diff == ColumnCommentsConfig(comments={"`col1`": "new comment"}, persist=True) - # No warning is emitted from the diff path. - mock_warn.assert_not_called() - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_diff__no_warning_when_not_persisting(self, mock_warn): - """Missing columns are not evaluated (or warned about) when persist is False.""" - reset_missing_persist_doc_column_warnings() - config = ColumnCommentsConfig(comments={"col1": "comment", "col2": "comment"}) - other = ColumnCommentsConfig(comments={"col1": "comment"}) - assert config.get_diff(other) is None - mock_warn.assert_not_called() diff --git a/tests/unit/test_adapter.py b/tests/unit/test_adapter.py index f8c8d193e..33c841983 100644 --- a/tests/unit/test_adapter.py +++ b/tests/unit/test_adapter.py @@ -34,9 +34,6 @@ ViewAPI, get_identifier_list_string, ) -from dbt.adapters.databricks.persist_doc_column_warnings import ( - reset_missing_persist_doc_column_warnings, -) from dbt.adapters.databricks.relation import ( DatabricksRelation, DatabricksRelationType, @@ -1185,72 +1182,6 @@ def test_get_persist_doc_columns_case_mismatch_no_update_needed(self, adapter): # No update needed since comments match assert result == {} - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_persist_doc_columns_warns_on_missing_column(self, mock_warn, adapter): - """Documented columns absent from the relation are warned about and skipped.""" - reset_missing_persist_doc_column_warnings() - existing = [self.create_column("col1", "comment1")] - column_dict = { - "col1": {"name": "col1", "description": "new comment"}, - "col2": {"name": "col2", "description": "comment for missing column"}, - } - result = adapter.get_persist_doc_columns(existing, column_dict) - # The missing column is filtered out; only the existing column is returned. - assert result == {"col1": {"name": "col1", "description": "new comment"}} - # A warning is emitted naming the missing column. - mock_warn.assert_called_once() - warned_event = mock_warn.call_args.args[0] - assert "col2" in warned_event.base_msg - assert "col1" not in warned_event.base_msg - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_persist_doc_columns_no_warning_when_all_present(self, mock_warn, adapter): - """No warning is emitted when every documented column exists (case-insensitively).""" - reset_missing_persist_doc_column_warnings() - existing = [self.create_column("Account_ID", "")] - column_dict = {"account_id": {"name": "account_id", "description": "Account ID column"}} - adapter.get_persist_doc_columns(existing, column_dict) - mock_warn.assert_not_called() - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_validate_persist_doc_columns_warns_on_missing_column(self, mock_warn, adapter): - """A documented column absent from the (post-build) relation is warned about.""" - reset_missing_persist_doc_column_warnings() - existing = [self.create_column("col1", "comment1")] - column_dict = { - "col1": {"name": "col1", "description": "comment1"}, - "col2": {"name": "col2", "description": "typo / stale doc"}, - } - assert adapter.validate_persist_doc_columns(existing, column_dict) is None - mock_warn.assert_called_once() - warned_event = mock_warn.call_args.args[0] - assert "col2" in warned_event.base_msg - assert "col1" not in warned_event.base_msg - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_validate_persist_doc_columns_no_warning_for_newly_added_column( - self, mock_warn, adapter - ): - """A legitimately new column is present in the freshly built relation post-build, so the - post-build check does not false-warn about it (the E1 regression).""" - reset_missing_persist_doc_column_warnings() - # col2 was just added to the model; post-build it exists in the relation. - existing = [self.create_column("col1", "c1"), self.create_column("col2", "c2")] - column_dict = { - "col1": {"name": "col1", "description": "c1"}, - "col2": {"name": "col2", "description": "c2"}, - } - adapter.validate_persist_doc_columns(existing, column_dict) - mock_warn.assert_not_called() - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_validate_persist_doc_columns_case_insensitive(self, mock_warn, adapter): - reset_missing_persist_doc_column_warnings() - existing = [self.create_column("Account_ID", "")] - column_dict = {"account_id": {"name": "account_id", "description": "Account ID"}} - adapter.validate_persist_doc_columns(existing, column_dict) - mock_warn.assert_not_called() - class TestGetColumnsByDbrVersion(DatabricksAdapterBase): @pytest.fixture diff --git a/tests/unit/test_persist_doc_column_warnings.py b/tests/unit/test_persist_doc_column_warnings.py deleted file mode 100644 index eeee4aa23..000000000 --- a/tests/unit/test_persist_doc_column_warnings.py +++ /dev/null @@ -1,60 +0,0 @@ -"""Unit tests for missing documented-column warning dedupe.""" - -import threading -from unittest.mock import patch - -from dbt.adapters.databricks.persist_doc_column_warnings import ( - reset_missing_persist_doc_column_warnings, - warn_missing_persist_doc_columns, -) -from dbt.adapters.databricks.relation_configs.column_comments import ColumnCommentsConfig - - -class TestWarnMissingPersistDocColumnsDedupe: - def setup_method(self) -> None: - reset_missing_persist_doc_column_warnings() - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_warns_once_for_same_missing_set(self, mock_warn): - warn_missing_persist_doc_columns(["col2"]) - warn_missing_persist_doc_columns(["col2"]) - mock_warn.assert_called_once() - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_get_diff_does_not_warn_helper_still_dedupes(self, mock_warn): - """get_diff is silent (warning moved post-build); the shared helper still dedupes.""" - config = ColumnCommentsConfig(comments={"col1": "new", "col2": "missing"}, persist=True) - other = ColumnCommentsConfig(comments={"col1": "old"}) - config.get_diff(other) - # The pre-materialization diff no longer contributes a warning. - mock_warn.assert_not_called() - # The post-build path warns once per unique set. - warn_missing_persist_doc_columns(["col2"]) - warn_missing_persist_doc_columns(["col2"]) - mock_warn.assert_called_once() - assert "col2" in mock_warn.call_args.args[0].base_msg - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_different_missing_sets_warn_separately(self, mock_warn): - warn_missing_persist_doc_columns(["col2"]) - warn_missing_persist_doc_columns(["col3"]) - assert mock_warn.call_count == 2 - - @patch("dbt.adapters.databricks.persist_doc_column_warnings.warn_or_error") - def test_thread_local_isolation(self, mock_warn): - """A reset on another thread must not suppress warnings on this thread.""" - warn_missing_persist_doc_columns(["col2"]) - assert mock_warn.call_count == 1 - - def other_thread() -> None: - reset_missing_persist_doc_column_warnings() - warn_missing_persist_doc_columns(["col2"]) - - t = threading.Thread(target=other_thread) - t.start() - t.join() - # Main thread already warned once; other thread warns independently → 2 total. - assert mock_warn.call_count == 2 - # Main thread still dedupes its own second call. - warn_missing_persist_doc_columns(["col2"]) - assert mock_warn.call_count == 2 From a1bd0b303c1c425d123e85cde4ba847c65cc43b0 Mon Sep 17 00:00:00 2001 From: Shubham Dhal Date: Wed, 5 Aug 2026 19:24:07 +0530 Subject: [PATCH 13/16] chore: tighten persist docs comments --- CHANGELOG.md | 2 +- .../relation_configs/column_comments.py | 8 +------- .../macros/adapters/persist_docs.sql | 16 ++-------------- .../incremental/incremental.sql | 3 +-- .../adapter/persist_docs/fixtures.py | 1 - .../adapter/persist_docs/test_persist_docs.py | 19 ------------------- .../test_column_comments_config.py | 3 --- 7 files changed, 5 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a1d8baea..2e8f9289e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Fixes -- Warn when a column documented in a model's `schema.yml` is absent from the relation, instead of silently skipping it — surfaces typos and stale column documentation. The check runs post-build against the actual relation (matching the shared `validate_doc_columns` behavior the other adapters use), so it covers V1 and V2 table/incremental on both the initial create and subsequent runs, does not false-warn on a legitimately new column, and is gated on `persist_docs.columns`. Also fixes the V2 column-comment gate to key off `persist_docs.columns` rather than `persist_docs.relation`. Materialized-view/streaming-table and view create are tracked as follow-ups. Ports the behavior added in dbt-adapters ([dbt-adapters#1684](https://github.com/dbt-labs/dbt-adapters/pull/1684) closes [dbt-adapters#1690](https://github.com/dbt-labs/dbt-adapters/issues/1690)) ([#1563](https://github.com/databricks/dbt-databricks/pull/1563)). +- 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/adapters/databricks/relation_configs/column_comments.py b/dbt/adapters/databricks/relation_configs/column_comments.py index 9e84c4565..6f502dee5 100644 --- a/dbt/adapters/databricks/relation_configs/column_comments.py +++ b/dbt/adapters/databricks/relation_configs/column_comments.py @@ -24,10 +24,7 @@ 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. Documented columns that are - # absent from the relation are skipped here so the alter never targets a nonexistent - # column; Jinja validation warns after the relation is built, so a legitimately new - # column is not flagged before it has been materialized. + # 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()) @@ -58,9 +55,6 @@ def from_relation_config(cls, relation_config: RelationConfig) -> ColumnComments columns = getattr(relation_config, "columns", {}) persist = False if relation_config.config: - # Column comments are gated on persist_docs.columns (the column-level knob), matching - # config.persist_column_docs() used by the V1 persist_docs / view-create / seed paths. - # persist_docs.relation is the table-comment knob and is the wrong gate here. persist = relation_config.config.persist_docs.get("columns") or False comments = {} for column_name, column in columns.items(): diff --git a/dbt/include/databricks/macros/adapters/persist_docs.sql b/dbt/include/databricks/macros/adapters/persist_docs.sql index 50157fe66..8ff88609e 100644 --- a/dbt/include/databricks/macros/adapters/persist_docs.sql +++ b/dbt/include/databricks/macros/adapters/persist_docs.sql @@ -44,10 +44,7 @@ {% endif %} {% endmacro %} -{#-- - Warn about documented columns absent from a materialized relation and return only the columns - that are present. Column names are matched case-insensitively, consistent with Databricks. ---#} +{#-- 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 = [] -%} @@ -68,16 +65,7 @@ {{- return(valid) -}} {%- endmacro %} -{#-- - Post-build validation of documented column comments against the actual relation. - - The V2 materialization path applies column comments inline at create-time and via the - relation-config diff (neither of which sees the model's documented columns as a set), so this - runs after the relation is built to surface columns that are documented in the schema but absent - from the relation (typos / stale docs). It mirrors the shared validate_doc_columns behavior the - other adapters use, and applies no comments itself. Gated on persist_docs.columns so it never - fires when column persistence is disabled (avoids --warn-error false failures). ---#} +{#-- 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) -%} diff --git a/dbt/include/databricks/macros/materializations/incremental/incremental.sql b/dbt/include/databricks/macros/materializations/incremental/incremental.sql index 7adbd20da..e949ca4c3 100644 --- a/dbt/include/databricks/macros/materializations/incremental/incremental.sql +++ b/dbt/include/databricks/macros/materializations/incremental/incremental.sql @@ -81,8 +81,7 @@ {%- endif -%} {%- endif -%} - {#-- Placed here so it runs on every sub-branch above (create/replace/merge) and regardless of - incremental_apply_config_changes. --#} + {#-- 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) %} diff --git a/tests/functional/adapter/persist_docs/fixtures.py b/tests/functional/adapter/persist_docs/fixtures.py index 0ca59e573..899fc8671 100644 --- a/tests/functional/adapter/persist_docs/fixtures.py +++ b/tests/functional/adapter/persist_docs/fixtures.py @@ -39,7 +39,6 @@ select 1 as id, 'alice' as name """ -# Incremental model whose schema documents a column absent from the materialized relation. missing_column_incremental_sql = """ {{ config(materialized='incremental') }} select 1 as id, 'Ed' as name diff --git a/tests/functional/adapter/persist_docs/test_persist_docs.py b/tests/functional/adapter/persist_docs/test_persist_docs.py index 066e202df..0b5a3a713 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -425,8 +425,6 @@ def test_column_comment_suppressed_when_columns_false(self, adapter, table_relat class TestPersistDocsColumnMissingWarnsV1: - """V1 persists comments for present columns and warns for missing ones.""" - @pytest.fixture(scope="class") def models(self): return {"missing_column.sql": fixtures._MODELS__MISSING_COLUMN} @@ -470,13 +468,6 @@ def test_warns_and_still_comments_present_columns(self, adapter, table_relation) class TestPersistDocsColumnMissingWarnsV2: - """v2: a documented column absent from the relation is warned about post-build. - - The warning is emitted by validate_persist_doc_columns after the relation is built, not from - the pre-materialization changeset diff. So it fires on the initial create and on every - subsequent incremental run — and a legitimately new column (present post-build) does not warn. - """ - @pytest.fixture(scope="class") def models(self): return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} @@ -516,9 +507,6 @@ def test_warning_escalates_on_create_and_subsequent_runs(self, project, adapter) class TestPersistDocsColumnMissingWarnsV2ColumnsOnly: - """v2 columns-only persist_docs warns (E2: column comments gate on persist_docs.columns, - not .relation — previously this combination was silent on the v2 path).""" - @pytest.fixture(scope="class") def models(self): return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} @@ -554,9 +542,6 @@ def test_warning_escalates_with_columns_only(self, project, adapter): class TestPersistDocsColumnMissingV2RelationOnlyNoWarn: - """v2 with columns:false does no column-doc work, so the missing-column check stays silent - (E2: column comments are gated on persist_docs.columns).""" - @pytest.fixture(scope="class") def models(self): return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} @@ -577,8 +562,6 @@ def test_no_warning_when_columns_disabled(self, project): class TestPersistDocsColumnMissingWarnsV1ColumnsOnly: - """v1: columns-only persist_docs still warns (does not require relation: true).""" - @pytest.fixture(scope="class") def models(self): return {"missing_column.sql": fixtures._MODELS__MISSING_COLUMN} @@ -622,8 +605,6 @@ def test_warning_escalates_with_columns_only(self, adapter, table_relation): class TestPersistDocsColumnMissingWarnsV1IncrementalSubsequent: - """V1 incremental runs continue to surface missing documented columns.""" - @pytest.fixture(scope="class") def models(self): return {"missing_column_incremental.sql": override_fixtures.missing_column_incremental_sql} diff --git a/tests/unit/relation_configs/test_column_comments_config.py b/tests/unit/relation_configs/test_column_comments_config.py index 1272bd8ad..0a132e0cd 100644 --- a/tests/unit/relation_configs/test_column_comments_config.py +++ b/tests/unit/relation_configs/test_column_comments_config.py @@ -45,13 +45,11 @@ def test_from_relation_config__no_persist(self): def test_from_relation_config__with_persist(self): model = Mock() model.columns = {"col1": {"description": "test comment"}} - # Column comments are gated on persist_docs.columns, not .relation. 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): - """persist_docs.columns drives column comments even when .relation is false.""" model = Mock() model.columns = {"col1": {"description": "test comment"}} model.config.persist_docs = {"columns": True, "relation": False} @@ -59,7 +57,6 @@ def test_from_relation_config__columns_true_relation_false(self): assert config == ColumnCommentsConfig(comments={"col1": "test comment"}, persist=True) def test_from_relation_config__relation_true_columns_false(self): - """Column comments are not applied when .columns is off, even if .relation is on.""" model = Mock() model.columns = {"col1": {"description": "test comment"}} model.config.persist_docs = {"relation": True, "columns": False} From 40d4dc71c6640c6940c629d16847d2ae69cffb0d Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 28 Jul 2026 12:18:17 +0530 Subject: [PATCH 14/16] Warn on missing persist_docs columns for view/materialized_view/streaming_table create --- CHANGELOG.md | 1 + .../materializations/materialized_view.sql | 3 + .../materializations/streaming_table.sql | 3 + .../macros/materializations/view.sql | 5 + .../adapter/persist_docs/fixtures.py | 56 +++++++++++ .../adapter/persist_docs/test_persist_docs.py | 92 +++++++++++++++++++ 6 files changed, 160 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e8f9289e..21158534f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### 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) ([#PLACEHOLDER](https://github.com/databricks/dbt-databricks/pull/PLACEHOLDER)). ### 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..5bc791b62 100644 --- a/dbt/include/databricks/macros/materializations/materialized_view.sql +++ b/dbt/include/databricks/macros/materializations/materialized_view.sql @@ -74,6 +74,9 @@ {{ execute_multiple_statements(build_sql) }} + {#-- Warn (post-build) about documented columns absent from the materialized view. --#} + {% 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..49a32a379 100644 --- a/dbt/include/databricks/macros/materializations/streaming_table.sql +++ b/dbt/include/databricks/macros/materializations/streaming_table.sql @@ -74,6 +74,9 @@ {{ execute_multiple_statements(build_sql) }} + {#-- Warn (post-build) about documented columns absent from the streaming table. --#} + {% 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 433afde1e..7e2863672 100644 --- a/dbt/include/databricks/macros/materializations/view.sql +++ b/dbt/include/databricks/macros/materializations/view.sql @@ -36,6 +36,8 @@ {{ apply_column_tags(target_relation, column_tags) }} {% endif %} {% endif %} + {#-- Warn (post-build) about documented columns absent from the view. --#} + {% 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 +68,9 @@ {{ apply_column_tags(target_relation, column_tags) }} {% endif %} + {#-- Warn (post-build) about documented columns absent from the view. --#} + {% 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..a06e65644 100644 --- a/tests/functional/adapter/persist_docs/fixtures.py +++ b/tests/functional/adapter/persist_docs/fixtures.py @@ -124,3 +124,59 @@ - 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 0b5a3a713..d2184c7f1 100644 --- a/tests/functional/adapter/persist_docs/test_persist_docs.py +++ b/tests/functional/adapter/persist_docs/test_persist_docs.py @@ -423,6 +423,11 @@ def test_column_comment_suppressed_when_columns_false(self, adapter, table_relat _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") @@ -739,3 +744,90 @@ 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): + 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 From 4fc5837fca4cca3a04d939a8f82eae21b0672386 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 28 Jul 2026 12:35:49 +0530 Subject: [PATCH 15/16] Trim what-comments at persist_docs validation call sites --- .../databricks/macros/materializations/materialized_view.sql | 1 - .../databricks/macros/materializations/streaming_table.sql | 1 - dbt/include/databricks/macros/materializations/view.sql | 2 -- 3 files changed, 4 deletions(-) diff --git a/dbt/include/databricks/macros/materializations/materialized_view.sql b/dbt/include/databricks/macros/materializations/materialized_view.sql index 5bc791b62..9b978a0c5 100644 --- a/dbt/include/databricks/macros/materializations/materialized_view.sql +++ b/dbt/include/databricks/macros/materializations/materialized_view.sql @@ -74,7 +74,6 @@ {{ execute_multiple_statements(build_sql) }} - {#-- Warn (post-build) about documented columns absent from the materialized view. --#} {% do validate_persist_doc_columns(target_relation, model) %} {%- do apply_tags(target_relation, tags) -%} diff --git a/dbt/include/databricks/macros/materializations/streaming_table.sql b/dbt/include/databricks/macros/materializations/streaming_table.sql index 49a32a379..7e5f712d6 100644 --- a/dbt/include/databricks/macros/materializations/streaming_table.sql +++ b/dbt/include/databricks/macros/materializations/streaming_table.sql @@ -74,7 +74,6 @@ {{ execute_multiple_statements(build_sql) }} - {#-- Warn (post-build) about documented columns absent from the streaming table. --#} {% do validate_persist_doc_columns(target_relation, model) %} {%- do apply_tags(target_relation, tags) -%} diff --git a/dbt/include/databricks/macros/materializations/view.sql b/dbt/include/databricks/macros/materializations/view.sql index 7e2863672..4b9a76ad2 100644 --- a/dbt/include/databricks/macros/materializations/view.sql +++ b/dbt/include/databricks/macros/materializations/view.sql @@ -36,7 +36,6 @@ {{ apply_column_tags(target_relation, column_tags) }} {% endif %} {% endif %} - {#-- Warn (post-build) about documented columns absent from the view. --#} {% 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) %} @@ -68,7 +67,6 @@ {{ apply_column_tags(target_relation, column_tags) }} {% endif %} - {#-- Warn (post-build) about documented columns absent from the view. --#} {% do validate_persist_doc_columns(target_relation, model) %} {{ run_hooks(post_hooks) }} From f1b69e6a7399aac0b23a5e37e05c179b673c4247 Mon Sep 17 00:00:00 2001 From: Rashi Jaiswal Date: Tue, 28 Jul 2026 12:19:18 +0530 Subject: [PATCH 16/16] Fill in PR number in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21158534f..98782f8df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ### 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) ([#PLACEHOLDER](https://github.com/databricks/dbt-databricks/pull/PLACEHOLDER)). +- 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