Skip to content

Commit 006d0c2

Browse files
fix: match the whole key tuple in legacy delete+insert (#1612)
Resolves #1611 ### Description On the `delete+insert` strategy with a composite `unique_key`, the legacy DBR < 17.1 path builds one `IN (SELECT ...)` per key column and ANDs them, so the DELETE predicate is the cross product of the key columns' value sets rather than the set of key tuples. Rows whose key tuple is absent from the source are deleted and never re-inserted, silently. With `unique_key: ['a', 'b']`, a target holding `(1,10) (2,20) (1,20) (2,10)` and a source producing only `(1,10) (2,20)`, the predicate `a IN (1,2) AND b IN (10,20)` matches all four rows. `(1,20)` and `(2,10)` are lost. The DBR 17.1+ branch of the same macro is already row-wise, and dbt-core's cross-adapter default is `where (unique_key_str) in (select distinct unique_key_str from source)`, so today the same model with the same config produces different data depending on runtime version. **Fix:** use a correlated `EXISTS` predicate that compares every key column against the same source row with null-safe equality. ```sql delete from target where exists ( select 1 from source where target.`a` <=> source.`a` and target.`b` <=> source.`b` ) ``` The single-key branch remains byte-for-byte identical, so its behavior is unchanged and remains covered by the existing single-key and non-ASCII tests. **Note on the changed tests:** `test_delete_insert_legacy_sql__multiple_unique_keys` previously asserted the per-column form, so it encoded the bug and had to be updated. Two neighbouring tests that asserted against their own hardcoded strings are converted to real macro renders, following your request on #1595. **Testing:** - 17 focused unit tests cover composite keys, incremental predicates, and the unchanged single-key path. - A functional regression forces the legacy `DELETE` + `INSERT` branch on live Databricks and verifies exact replacements, crossed-tuple preservation, and absent-row preservation. - The correlated `DELETE` statement was also run directly on DBR 16.4 LTS and produced the expected final rows. A full dbt end-to-end run on compute below DBR 17.1 would add release-matrix confidence, but the changed predicate and complete dbt execution path are covered by the complementary checks above. ### 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. --------- Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com> Co-authored-by: Shubham Dhal <shubham.dhal@databricks.com>
1 parent f170619 commit 006d0c2

5 files changed

Lines changed: 114 additions & 34 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
### Fixes
44

5+
- Stop `delete+insert` with a composite `unique_key` from deleting unmatched rows on DBR below 17.1 (thanks @SreeramaYeshwanthGowd!) ([#1612](https://github.com/databricks/dbt-databricks/pull/1612) resolves [#1611](https://github.com/databricks/dbt-databricks/issues/1611))
56
- Escape single quotes in relation comments so materialized views and streaming tables with an apostrophe in the description can be created (thanks @SreeramaYeshwanthGowd!) ([#1613](https://github.com/databricks/dbt-databricks/pull/1613) resolves [#1251](https://github.com/databricks/dbt-databricks/issues/1251))
67

78
### Under the Hood

dbt/include/databricks/macros/materializations/incremental/strategies.sql

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,9 +181,18 @@ replace on ({{ replace_on_expr }})
181181

182182
{#-- Build WHERE clause for DELETE statement --#}
183183
{%- set delete_conditions = [] -%}
184-
{%- for key in unique_keys -%}
184+
{%- if unique_keys | length > 1 -%}
185+
{#-- a row-valued IN raises DELTA_UNSUPPORTED_MULTI_COL_IN_PREDICATE; correlate on
186+
the whole tuple instead so unmatched key combinations are not deleted (issue #1611) --#}
187+
{%- set correlation_conditions = [] -%}
188+
{%- for key in unique_keys -%}
189+
{%- do correlation_conditions.append(target_relation ~ '.' ~ adapter.quote(key) ~ ' <=> ' ~ source_relation ~ '.' ~ adapter.quote(key)) -%}
190+
{%- endfor -%}
191+
{%- do delete_conditions.append('EXISTS (SELECT 1 FROM ' ~ source_relation ~ ' WHERE ' ~ correlation_conditions | join(' AND ') ~ ')') -%}
192+
{%- else -%}
193+
{%- set key = unique_keys[0] -%}
185194
{%- do delete_conditions.append(target_relation ~ '.' ~ adapter.quote(key) ~ ' IN (SELECT ' ~ adapter.quote(key) ~ ' FROM ' ~ source_relation ~ ')') -%}
186-
{%- endfor -%}
195+
{%- endif -%}
187196

188197
{#-- Add incremental predicates to DELETE if specified --#}
189198
{%- if incremental_predicates is sequence and incremental_predicates is not string -%}

tests/functional/adapter/incremental/fixtures.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,13 @@
309309
3,anyway
310310
"""
311311

312+
delete_insert_composite_key_expected = """id,color,msg
313+
1,blue,replaced
314+
1,red,updated
315+
2,blue,updated
316+
2,red,goodbye
317+
"""
318+
312319
delete_insert_update_schema_expected = """id
313320
1
314321
2
@@ -442,6 +449,48 @@
442449
{% endif %}
443450
"""
444451

452+
force_legacy_delete_insert_macros = """
453+
{% macro delete_insert_sql_impl(
454+
source_relation, target_relation, target_columns, unique_key, incremental_predicates
455+
) %}
456+
{#-- Force the DBR < 17.1 path so the legacy DELETE predicate runs on any compute --#}
457+
{%- set keys = unique_key
458+
if unique_key is sequence and unique_key is not string
459+
else [unique_key] -%}
460+
{% do return(delete_insert_legacy_sql(
461+
source_relation, target_relation, target_columns, keys, incremental_predicates
462+
)) %}
463+
{% endmacro %}
464+
"""
465+
466+
delete_insert_composite_key_model = """
467+
{{ config(
468+
materialized = 'incremental',
469+
unique_key = ['id', 'color'],
470+
incremental_strategy = 'delete+insert',
471+
) }}
472+
473+
{% if not is_incremental() %}
474+
475+
select cast(1 as bigint) as id, 'blue' as color, 'hello' as msg
476+
union all
477+
select cast(2 as bigint) as id, 'red' as color, 'goodbye' as msg
478+
479+
{% else %}
480+
481+
-- (1, blue) is an exact key match and must be replaced with its new payload.
482+
-- (1, red) and (2, blue) only bait a per-column match; matching each column on
483+
-- its own would wrongly delete both existing rows, so they must only insert.
484+
-- (2, red) is absent from this run and so must survive untouched.
485+
select cast(1 as bigint) as id, 'blue' as color, 'replaced' as msg
486+
union all
487+
select cast(1 as bigint) as id, 'red' as color, 'updated' as msg
488+
union all
489+
select cast(2 as bigint) as id, 'blue' as color, 'updated' as msg
490+
491+
{% endif %}
492+
"""
493+
445494
delete_insert_with_predicates_model = """
446495
{{ config(
447496
materialized = 'incremental',

tests/functional/adapter/incremental/test_incremental_strategies.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,30 @@ def test_incremental(self, project):
451451
)
452452

453453

454+
class TestDeleteInsertCompositeKey(IncrementalBase):
455+
@pytest.fixture(scope="class")
456+
def models(self):
457+
return {
458+
"delete_insert_model.sql": fixtures.delete_insert_composite_key_model,
459+
}
460+
461+
@pytest.fixture(scope="class")
462+
def macros(self):
463+
return {"force_legacy_delete_insert.sql": fixtures.force_legacy_delete_insert_macros}
464+
465+
@pytest.fixture(scope="class")
466+
def seeds(self):
467+
return {
468+
"delete_insert_expected.csv": fixtures.delete_insert_composite_key_expected,
469+
}
470+
471+
def test_incremental(self, project):
472+
self.seed_and_run_twice()
473+
util.check_relations_equal(
474+
project.adapter, ["delete_insert_model", "delete_insert_expected"]
475+
)
476+
477+
454478
class TestDeleteInsertUpdateSchema(IncrementalBase):
455479
@pytest.fixture(scope="class")
456480
def models(self):

tests/unit/macros/materializations/incremental/test_delete_insert.py

Lines changed: 29 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -153,44 +153,41 @@ def test_delete_insert_legacy_sql__non_ascii_unique_key(self, template, context)
153153
assert self.clean_sql(insert_sql).startswith("insert into target")
154154

155155
def test_delete_insert_legacy_sql__multiple_unique_keys(self, template, context):
156-
"""Multiple unique keys are each back-quoted and ANDed in the DELETE predicate."""
156+
"""A composite unique_key correlates the whole tuple, not each column independently."""
157157
delete_sql, _ = self.render_legacy(
158158
template, context, unique_keys=["a", "b"], target_columns=("a", "b")
159159
)
160160
clean_delete = self.clean_sql(delete_sql)
161-
assert "target.`a` in (select `a` from source)" in clean_delete
162-
assert "target.`b` in (select `b` from source)" in clean_delete
163-
assert clean_delete.count(" and ") == 1
164-
165-
def test_legacy_sql_generation__single_unique_key_delete(self, template, context):
166-
"""Test the DELETE SQL generation for single unique key"""
167-
# We'll verify by compiling a test query that uses the same logic
168-
# Mock adapter
169-
context["adapter"].has_dbr_capability = lambda cap: cap == "insert_by_name"
170-
171-
# Build expected DELETE manually using the same logic as the macro
172-
expected_delete = """
173-
delete from target
174-
where target.a IN (SELECT a FROM source)
175-
"""
176-
177-
# The macro builds: target.{key} IN (SELECT {key} FROM source)
178-
# This test documents the expected SQL pattern
179-
assert "delete from" in expected_delete.lower()
180-
assert "target.a in (select a from source)" in expected_delete.lower()
161+
assert (
162+
"exists (select 1 from source where target.`a` <=> source.`a`"
163+
" and target.`b` <=> source.`b`)"
164+
) in clean_delete
165+
assert clean_delete.startswith("delete from target where exists")
181166

182-
def test_legacy_sql_generation__multiple_unique_keys_delete(self, template, context):
183-
"""Test the DELETE SQL generation for multiple unique keys"""
184-
expected_delete = """
185-
delete from target
186-
where target.a IN (SELECT a FROM source)
187-
and target.b IN (SELECT b FROM source)
188-
"""
167+
def test_delete_insert_legacy_sql__multiple_unique_keys_with_predicates(
168+
self, template, context
169+
):
170+
"""Incremental predicates are ANDed after the EXISTS clause, not inside it."""
171+
delete_sql, _ = self.render_legacy(
172+
template,
173+
context,
174+
unique_keys=["a", "b"],
175+
target_columns=("a", "b"),
176+
incremental_predicates=["a > 1"],
177+
)
178+
clean_delete = self.clean_sql(delete_sql)
179+
assert (
180+
"exists (select 1 from source where target.`a` <=> source.`a`"
181+
" and target.`b` <=> source.`b`)"
182+
) in clean_delete
183+
assert clean_delete.endswith("and a > 1")
189184

190-
# The macro builds conditions for each key with AND
191-
assert "target.a in" in expected_delete.lower()
192-
assert "target.b in" in expected_delete.lower()
193-
assert expected_delete.lower().count(" and ") == 1
185+
def test_legacy_sql_generation__single_unique_key_delete(self, template, context):
186+
"""A single unique key keeps the original per-column predicate."""
187+
delete_sql, _ = self.render_legacy(template, context, unique_keys=["a"])
188+
clean_delete = self.clean_sql(delete_sql)
189+
assert clean_delete.startswith("delete from target where")
190+
assert "target.`a` in (select `a` from source)" in clean_delete
194191

195192
def test_legacy_sql_generation__with_predicates_delete(self, template, context):
196193
"""Test that incremental_predicates are added to DELETE WHERE clause"""

0 commit comments

Comments
 (0)