fix: match the whole key tuple in legacy delete+insert - #1612
fix: match the whole key tuple in legacy delete+insert#1612SreeramaYeshwanthGowd wants to merge 7 commits into
Conversation
Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
…posite-unique-key # Conflicts: # CHANGELOG.md
|
@sd-db @jprakash-db Merged main and cleared the changelog conflict. Still glad to hear your thoughts on the open question about the DELETE syntax whenever you get a chance. |
sd-db
left a comment
There was a problem hiding this comment.
Thanks for putting this together — the underlying bug is real. I reproduced the baseline behavior: the legacy predicate deletes the crossed, unmatched tuples.
I did hit one blocker with the current patch: Delta rejects the row-valued IN in a DELETE condition with [DELTA_UNSUPPORTED_MULTI_COL_IN_PREDICATE] Multi-column In predicates are not supported in the DELETE condition. The new forced-legacy functional test therefore fails on the second dbt run before reaching its result assertion. A correlated EXISTS with one null-safe <=> comparison per key should preserve tuple correlation while using a supported DELETE shape.
When updating the regression test, it would also be useful to include at least one source tuple that already exists in the target with a changed payload. That proves the corrected strategy still deletes/replaces exact matches, rather than only preserving the crossed unmatched rows.
Could you share whether you or another user are currently hitting this row-loss behavior in an active workload — and, if so, which DBR version and how blocking it is — or whether this was found proactively? That context would help gauge the urgency. If it is actively impacting workloads, I am happy to chip in on the macro and regression-test changes as well.
…lued IN Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
|
@sd-db Thanks for catching this. Swapped the row valued IN for a correlated EXISTS with null safe equality per key, matched on the full tuple. Tests updated with a real key match case, to confirm true matches still get replaced and not just the false positive cases. Also ran the actual statement directly on a DBR 16.4 LTS single node cluster against a composite key table. The DELETE succeeded with no error, and only the true tuple match was removed, the unrelated row stayed. So the internal Delta flag does not block this on Databricks Runtime, at least on 16.4 LTS. Raw output from that run: |
Resolves #1611
Description
On the
delete+insertstrategy with a compositeunique_key, the legacy DBR < 17.1 path builds oneIN (SELECT ...)per key column and ANDs them, so the DELETE predicate is the cross product of thekey 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 producingonly
(1,10) (2,20), the predicatea 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 withthe same config produces different data depending on runtime version.
Fix: emit a single tuple predicate for a composite key.
The single-key branch is left byte for byte identical, so nearly all users see no SQL change and the
existing single-key and non-ASCII tests are untouched.
Note on the changed tests:
test_delete_insert_legacy_sql__multiple_unique_keyspreviouslyasserted 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: unit tests cover the composite key, the composite key with incremental predicates, and
the single-key path. Needs an
/integration-testrun on compute below DBR 17.1 to exercise thechanged branch end to end.
Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.