fix: warn when documented columns are missing from the relation in persist_docs#1563
fix: warn when documented columns are missing from the relation in persist_docs#1563r-jais wants to merge 5 commits into
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
sd-db
left a comment
There was a problem hiding this comment.
Changes look good, but the current change just is for the materialisation-v1 path. We need to make similar changes for mat_v2 path as well
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 databricks#1563.
e568f12 to
a248840
Compare
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 databricks#1563.
sd-db
left a comment
There was a problem hiding this comment.
Current changes look good, had a minor comment on changelog but the current coverage is limited to mostly V1 persist_docs + V2 alter/config-diff
|
@r-jais — follow-up coverage review of this PR at head Coverage report: missing-column
|
| Materialization | Scenario | Verdict | Path |
|---|---|---|---|
| table | V1 create/replace | WARNS | persist_docs → get_persist_doc_columns |
| table | V2 create/replace | SILENT | create_table_at → parse_columns_and_constraints |
| incremental | V1 create/full refresh | WARNS | post-create persist_docs |
| incremental | V1 subsequent, config changes on | WARNS once | typed diff is gated off; later persist_docs warns |
| incremental | V1 subsequent, config changes off | WARNS once | config diff is skipped; persist_docs still runs |
| incremental | V2 create/full refresh | SILENT | create_table_at → parse_columns_and_constraints |
| incremental | V2 subsequent, config changes on | SILENT | ColumnCommentsConfig.persist is false because relation is false |
| incremental | V2 subsequent, config changes off | SILENT | no config diff and no persist_docs fallback |
| view | V1 create/replace | SILENT | get_persist_docs_column_list iterates query columns only |
| view | V2 create/replace_with_view |
SILENT | same shared create helper |
| view | V2 comment-only alter | SILENT | typed diff is gated off by relation: false |
| view | V2 query-changing alter/reapply | WARNS once | get_persist_doc_columns after ALTER VIEW AS |
| snapshot | after materialize | WARNS | persist_docs |
| clone | table path | WARNS | persist_docs |
| clone | view fallback | SILENT | shared view-create helper |
| seed | create/reset | SILENT | iterates agate/query columns only |
| materialized_view | create/replace/full refresh | SILENT | parse_columns_and_constraints |
| materialized_view | in-place alter | N/A | no ColumnCommentsProcessor |
| streaming_table | create/replace/full refresh | SILENT | parse_columns_and_constraints |
| streaming_table | in-place alter | N/A | no ColumnCommentsProcessor |
I removed the headline counts because the earlier matrix combined multiple scenarios and flag combinations, making the totals non-reproducible.
Outcomes when both relation: true and columns: true
These rows change:
- V1 incremental subsequent, config changes on → DOUBLE warning risk. It runs
model_config.get_changeset(...)and laterpersist_docs. - V2 incremental subsequent, config changes on → WARNS. The typed
get_diffgate is now enabled. - V2 view comment-only alter → WARNS. The typed
get_diffgate is now enabled. - V2 view query-changing alter/reapply → DOUBLE warning risk. Config diff warns, then query reapplication calls
get_persist_doc_columns.
The wrong-key gate also has the inverse effect: relation: true, columns: false can warn through the typed diff even though column persistence is disabled. Fixing the gate is therefore needed to avoid both false negatives and false positives.
What this PR already covers well
- V1 table/incremental/snapshot/clone-table paths through
get_persist_doc_columns - Relation-config diff paths when their current
persistgate is enabled - V2 view query reapplication through
get_persist_doc_columns - Filtering missing columns out of the V2 alter diff so no ALTER targets a nonexistent column
Highest-priority gaps to consider
- V2 table/incremental create-time —
parse_columns_and_constraintsnever warns; first create and full refresh remain silent. - View create (V1 + V2) —
get_persist_docs_column_listiterates query columns only, so YAML-only columns remain silent. - Typed persist gate —
ColumnCommentsProcessorshould gate column comments onpersist_docs.columns, notpersist_docs.relation. - V2 +
incremental_apply_config_changes: false— skipsget_diffwith no V1-stylepersist_docsfallback. - Duplicate warnings — warning in multiple low-level comparison helpers can emit twice during one model run.
Suggested next steps
- If keeping the current scope: narrow the PR description/CHANGELOG to the comparison points actually covered, and call out typed-create and view-create as follow-ups.
- If expanding scope here: centralize or otherwise deduplicate the missing-column warning; fix the typed gate to use
columns; cover view create; and gate any typed-create warning explicitly onconfig.persist_column_docs(). - Do not add an unconditional warning inside
parse_columns_and_constraints: that helper runs even when column persistence is disabled, and--warn-errorwould turn those false positives into failures.
Happy to clarify any row in the matrix if useful.
I would also recommend to add functional tests to verify on behaviour as well as there can be cases on Silent/Double warnings so need to balance coverage/correctness |
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 databricks#1563.
a248840 to
2e8a553
Compare
…_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).
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 databricks#1563.
2e8a553 to
6607028
Compare
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
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.
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.
Ports dbt-labs/dbt-adapters#1684 (closes dbt-labs/dbt-adapters#1690) into dbt-databricks. dbt-databricks overrides
persist_docswith its own native handling, so the upstream macro change does not reach this adapter — the warning is added here directly.Description
When
persist_docs.columnsis enabled, columns documented in a model'sschema.ymlthat 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:DatabricksAdapter.get_persist_doc_columns(impl.py), which iterates only the columns that actually exist in the relation.ColumnCommentsConfig.get_diff(relation_configs/column_comments.py), which previously emitted the missing column into the diff (targeting a nonexistent column on theALTER).Filtering behavior is unchanged (missing columns are still skipped so the
ALTERnever errors); the columns are now surfaced viawarn_or_error, so the warning respects--warn-errorlike other adapter warnings.Example warning:
Scope
Covered: the two column-comment comparison paths above (V1
get_persist_doc_columns, V2ColumnCommentsConfig.get_diff). These run when documented columns are diffed against an existing relation to build the commentALTER:CREATE OR REPLACEand applies comments inline every run, so it does not go throughget_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:
parse_columns_and_constraints). This builds inlineCREATE/replace comments and never warns. It covers V2 table models (every run) and the first create /--full-refreshof any model. A warning here must be gated explicitly onconfig.persist_column_docs()— an unconditional warning inparse_columns_and_constraintswould fire even when column persistence is disabled, and--warn-errorwould turn those false positives into failures.get_persist_docs_column_listiterates query columns only, so YAML-only columns stay silent.ColumnCommentsProcessorgates column comments onpersist_docs.relation; it should gate onpersist_docs.columns(config.persist_column_docs()).incremental_apply_config_changes: false. Skipsget_diffentirely, with no V1-style persist_docs fallback, so no warning is emitted.use_materialization_v2selects one), and each fires once, so a run warns exactly once — asserted in the functional tests. Duplication would only become possible if the warning were later added to an additional helper that can co-fire with these (e.g. create-timeparse_columns_and_constraints); at that point a centralized/deduplicated warning would be preferable.Tests
tests/unit/test_adapter.py,tests/unit/relation_configs/test_column_comments_config.py).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 reachesget_diff);--warn-errorescalates the warning to a run failure. Verified against a live UC SQL warehouse.Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.