Merge 1.13.latest: persist_docs warnings, MV schema drift, dead event cleanup - #1644
Closed
saishreeeee wants to merge 6 commits into
Closed
Merge 1.13.latest: persist_docs warnings, MV schema drift, dead event cleanup#1644saishreeeee wants to merge 6 commits into
saishreeeee wants to merge 6 commits into
Conversation
### 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.
…rsist_docs (#1563) Ports [dbt-labs/dbt-adapters#1684](dbt-labs/dbt-adapters#1684) (closes [dbt-labs/dbt-adapters#1690](dbt-labs/dbt-adapters#1690)) into dbt-databricks. dbt-databricks overrides `persist_docs` with its own native handling, so the upstream macro change does not reach this adapter — the warning is added here directly. ### Description When `persist_docs.columns` is enabled, columns documented in a model's `schema.yml` that are **not** present in the relation being commented (typo, renamed column, stale docs) are silently skipped — no comment is applied and no feedback is given. This PR adds a warning that names those columns on the two **column-comment comparison paths**, where the adapter diffs documented columns against the relation's actual columns to build the comment `ALTER`: - **V1 materialization** — `DatabricksAdapter.get_persist_doc_columns` (`impl.py`), which iterates only the columns that actually exist in the relation. - **V2 materialization** — `ColumnCommentsConfig.get_diff` (`relation_configs/column_comments.py`), which previously emitted the missing column into the diff (targeting a nonexistent column on the `ALTER`). Filtering behavior is unchanged (missing columns are still skipped so the `ALTER` never errors); the columns are now surfaced via `warn_or_error`, so the warning respects `--warn-error` like other adapter warnings. Both paths route through a shared helper (`persist_doc_column_warnings`) that dedupes, so a single model materialization warns at most once per unique missing-column set. The dedupe cache is thread-local and reset in `pre_model_hook` at the start of each model — dbt pins each model to one worker thread, so parallel model runs never suppress each other's warnings. Example warning: ``` The following columns are specified in the schema but are not present in the database and will be skipped: col2 ``` ### Scope Covered: the two column-comment comparison paths above (V1 `get_persist_doc_columns`, V2 `ColumnCommentsConfig.get_diff`). These run when documented columns are diffed against an *existing* relation to build the comment `ALTER`: - **V1** — post-write on table and incremental materializations. - **V2** — the relation-config **changeset path**, reached by incremental (and materialized-view / streaming-table) re-runs. Note a V2 **table** rebuilds via `CREATE OR REPLACE` and applies comments inline every run, so it does not go through `get_diff` — V2 table coverage is the create-time follow-up below. ### Out of scope / follow-ups Deliberately not addressed here to keep the change focused on the comparison paths. Filed as follow-ups: - **Create-time inline comments (`parse_columns_and_constraints`).** This builds inline `CREATE`/replace comments and never warns. It covers V2 table models (every run) and the first create / `--full-refresh` of any model. A warning here must be gated explicitly on `config.persist_column_docs()` — an unconditional warning in `parse_columns_and_constraints` would fire even when column persistence is disabled, and `--warn-error` would turn those false positives into failures. - **View create (V1 + V2).** `get_persist_docs_column_list` iterates query columns only, so YAML-only columns stay silent. - **Typed persist gate.** `ColumnCommentsProcessor` gates column comments on `persist_docs.relation`; it should gate on `persist_docs.columns` (`config.persist_column_docs()`). - **V2 with `incremental_apply_config_changes: false`.** Skips `get_diff` entirely, with no V1-style persist_docs fallback, so no warning is emitted. ### Tests - **Unit** — both comparison paths, warn and no-warn branches (`tests/unit/test_adapter.py`, `tests/unit/relation_configs/test_column_comments_config.py`); dedupe helper (`tests/unit/test_persist_doc_column_warnings.py`) covering warn-once-per-set, shared V1/V2 dedupe, distinct sets, and thread isolation. - **Functional** (`tests/functional/adapter/persist_docs/test_persist_docs.py`) — V1 warns and still comments present columns; V2 warns on the changeset path (second run of an **incremental** model — a table rebuild re-applies comments inline and never reaches `get_diff`); `--warn-error` escalates the warning to a run failure. Verified against a live UC SQL warehouse. ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests (unit + functional; see above) - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt-databricks next" section. --------- Co-authored-by: Shubham Dhal <shubham.dhal@databricks.com>
## Summary - Detect when a materialized view's create-time column list no longer matches the query-inferred schema (e.g. upstream `select *` gains a column) and recreate via replace instead of issuing `REFRESH`, which Databricks rejects with an incompatible user-specified schema error. - Names-only comparison by design (type-label variance across DESCRIBE paths would cause spurious recreates). Streaming tables (#1303) are intentionally out of scope — they need a different approach around locking columns on CREATE. Resolves #1359 ## Test plan - [x] Unit: `tests/unit/test_column_schemas_differ.py` - [x] Functional (SQL warehouse): `tests/functional/adapter/materialized_view_tests/test_mv_schema_evolution.py` - [x] Smoke: existing `test_mv_alter_no_rebuild` still passes - [ ] CI integration tests
…streaming_table create (#1615) ### Description resolves #1399 Follow-up to #1563. That PR added the "warn when a documented column is absent from the relation" check as a **post-build validation** (`validate_persist_doc_columns`, mirroring the shared `validate_doc_columns` behavior every other adapter uses) and wired it into the **table** and **incremental** materializations. This PR completes the create-time coverage by calling the same gated, post-build validation on the materializations #1563 didn't touch: - **view** create (V1 + V2) — `dbt/include/databricks/macros/materializations/view.sql` - **materialized view** create/replace/refresh — `.../materialized_view.sql` - **streaming table** create/replace/refresh — `.../streaming_table.sql` Each is a single `validate_persist_doc_columns(target_relation, model)` call after the relation is built. The macro is gated on `config.persist_column_docs()` and applies no comments, so it never fires when column persistence is off and stays safe under `--warn-error` (per the caution in #1563 about not warning unconditionally in `parse_columns_and_constraints`). Previously these paths iterated only the query's output columns (`get_persist_docs_column_list`) or built inline comments via `parse_columns_and_constraints`, so a YAML-only documented column was silently dropped. ### Stacking > **This PR is stacked on #1563** and currently shows its commits too. It targets `main`; once #1563 merges, this diff reduces to just the view/materialized-view/streaming-table changes. Review/merge #1563 first. ### Tests Functional (`tests/functional/adapter/persist_docs/test_persist_docs.py`): create-time missing-column warning for **view**, **materialized view**, and **streaming table** (each: documented column absent from the relation → warns exactly once, gated on `persist_docs.columns`). New fixtures in `tests/functional/adapter/persist_docs/fixtures.py`. The core `validate_persist_doc_columns` logic is unit-tested in #1563. ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt-databricks next" section.
Collaborator
Author
|
Closing — opened by mistake (cancel requested). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #
Description
Draft PR of
1.13.latestintomain. No new commits were created for this PR; it contains the existing 1.13.latest work that is not yet onmain.schema.ymldocuments columns that are missing from the relation underpersist_docs.columns, including create-time coverage for views, materialized views, and streaming tables (#1563,#1615/#1399).REFRESH, which Databricks rejects (#1621/#1359).#1547).maininto1.13.latestafter the 1.12.2 and 1.12.4 releases.maincurrently has two commits that are not in1.13.latest(#1640,#1642); this PR may need a merge/rebase before it is mergeable.Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.dbt-databricks-pr-readyproject skill for this PR and addressed its merge-readiness feedback