fix: properly check for unapplied tblproperties configs in existing r… - #1350
Conversation
47df178 to
091b4b7
Compare
…elation Signed-off-by: Can Bekleyici <can.bekleyici@deepl.com>
091b4b7 to
d3abbbe
Compare
|
Cool, |
Resolve conflict in relation_configs/tblproperties.py: keep the new set-diff get_diff() approach over main's ignore_list. main's databricks#1489 added delta.parquet.compression.codec to the ignore_list; that property is now handled automatically by the set-only diff, so the ignore_list is dropped entirely.
Move the entry out of the released 1.11.6 section (where the main merge stranded it) into 1.12.2 (TBD). Reclassify under Fixes since it closes databricks#1314 (materialized views always rebuilding) and add the PR + closing-issue links.
…_results After dropping the ignore_list, from_relation_results retains all properties (including server-set ones like clusterByAuto, clusteringColumns, and delta.parquet.compression.codec). Update test_incremental_config to expect the now-captured properties, and replace test_from_results__drops_ignored_properties with a retention test plus a get_diff test that guards the same guarantee databricks#1489 cared about (server-set properties never register as a change).
Move the entry out of the released 1.11.6 section (where the main merge stranded it) into 1.12.2 (TBD). Reclassify under Fixes since it closes databricks#1314 (materialized views always rebuilding) and add the PR + closing-issue links.
91538be to
7f758a7
Compare
Move the entry out of the released 1.11.6 section (where the main merge stranded it) into 1.12.2 (TBD). Reclassify under Fixes since it closes databricks#1314 (materialized views always rebuilding) and add the PR + closing-issue links.
…nge tests The _check_tblproperties helper filtered the relation's tblproperties by TblPropertiesConfig.ignore_list, which this PR removes — it would raise AttributeError once the PR lands. from_relation_results now returns all properties, so scope the comparison to the keys the model configures instead. Verified live: TestMaterializedViewApplyChanges + TestStreamingTableChangesApply (7 passed) on databricks_uc_sql_endpoint.
7f758a7 to
e9f6ca8
Compare
|
/integration-test |
|
Integration tests dispatched for PR #1350 by @sd-db. Track progress in the Actions tab. |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
|
Integration results for PR #1350 — UC cluster ❌ failure · SQL warehouse ❌ failure · All-purpose cluster ❌ failure · Shard coverage ✅ success |
|
/integration-test |
|
Integration tests dispatched for PR #1350 by @sd-db. Track progress in the Actions tab. |
|
Integration results for PR #1350 — UC cluster ✅ success · SQL warehouse ✅ success · All-purpose cluster ✅ success · Shard coverage ✅ success |
sd-db
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR, I have done some changes to fix a bug that might have affected STs + added tests. Overall looks good !!
…fresh get_diff returned only the unapplied subset of tblproperties, but the streaming-table alter path renders the changeset as a full CREATE OR REFRESH STREAMING TABLE clause, dropping already-applied desired properties. Return the complete desired config when any property differs, per the DatabricksComponentConfig.get_diff contract. Add unit regressions (get_diff contract + ST changeset boundary) and make the ST alter functional test additive: assert an already-applied property survives alongside a newly added one.
Resolves #1314
Description
Aquiring TblProperties changesets is highly error-prone and not scalable. The old code filtered out Databricks-internal properties (like delta.minReaderVersion, clusteringColumns, etc.) when reading from the relation from a static "ignore list" that needed to be updated continuously. With the new approach,
from_relation_resultsnow includes all properties from the relation. Sinceget_diffonly looks at what config tblproperties are currently missing or incorrect, these extra databricks-internal properties in other are harmless, they'll never appear in the diff.Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.