feat: implement support for INSERT ... ON CONFLICT (DO NOTHING / DO UPDATE) clauses - #24982
Nachiket-Roy wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24982 +/- ##
==========================================
+ Coverage 81.72% 81.90% +0.18%
==========================================
Files 1127 1132 +5
Lines 416310 421969 +5659
Branches 416310 421969 +5659
==========================================
+ Hits 340219 345625 +5406
+ Misses 56100 55901 -199
- Partials 19991 20443 +452 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e3c90f3 to
0994ef4
Compare
There was a problem hiding this comment.
Thanks @Nachiket-Roy!
I've found what I think is an execution bug while reviewing, where WHEN NOT MATCHED BY SOURCE AND <predicate> THEN DELETE ignores the predicate and deletes every unmatched target row.
In datafusion/catalog/src/memory/table.rs, the loop evaluates clause.predicate only in the MergeIntoAction::Update arm.
The MergeIntoAction::Delete arm pushes the row into row_deletions and increments affected_count without checking the predicate.
That means that the rows the user asked to keep (predicate false or NULL) are deleted.
Repro (run as a sqllogictest case):
CREATE TABLE t_repro(id int, flag boolean);
INSERT INTO t_repro VALUES (1, true), (2, false), (3, NULL);
CREATE TABLE s_repro(id int);
INSERT INTO s_repro VALUES (2);
MERGE INTO t_repro USING s_repro ON t_repro.id = s_repro.id
WHEN MATCHED THEN UPDATE SET flag = false
WHEN NOT MATCHED BY SOURCE AND t_repro.flag THEN DELETE;
SELECT id, flag FROM t_repro ORDER BY id;Expected: 2 false and 3 NULL.
Actual: only 2 false.
Suggested fix: evaluate clause.predicate in the MergeIntoAction::Delete arm and add an execution test for the predicate-false and predicate-NULL cases.
The current merge_into.slt coverage for this clause is explain-only.
c296aa1 to
9faa820
Compare
|
Hello @quwin, |
Which issue does this PR close?
Rationale for this change
DataFusion previously rejected PostgreSQL-style
INSERT INTO ... ON CONFLICT (col, ...) DO NOTHINGandINSERT INTO ... ON CONFLICT (col, ...) DO UPDATE SET ... [WHERE ...]upsert statements during SQL planning with"This feature is not implemented: ON CONFLICT is not supported".Design & Architectural Approach
Rather than modifying
InsertOpor adding new dispatch methods onTableProvider(which would break external connectors such asdelta-rs,iceberg-rust, and custom catalogs across upgrades) or adding protobuf schema churn, this change desugarsINSERT ... ON CONFLICTintoWriteOp::MergeInto(Box<MergeIntoOp>)at the SQL planning stage:InsertOpremains unchanged (Append,Overwrite,Replace), andTableProvider::insert_intois untouched.dml_node::Type::MergeIntoprotobuf representation.TableProvider::merge_intoautomatically gainsON CONFLICTupsert capabilities for free.excluded.DO NOTHINGmaps toWHEN NOT MATCHED THEN INSERT.DO UPDATEmaps toWHEN MATCHED [AND predicate] THEN UPDATE SET ...followed byWHEN NOT MATCHED THEN INSERT.NULLconflict keys never conflict (NULL != NULL), taking the insert path."ON CONFLICT DO UPDATE command cannot affect row a second time", whileDO NOTHINGcoalesces/deduplicates them.constraints() == Noneor empty): validated against existing schema columns, and guarded against duplicate rows at runtime.What changes are included in this PR?
1. SQL Planning & Desugaring (
datafusion-sql)onconflict clause.ON CONFLICTcannot be combined withINSERT OVERWRITEorREPLACE INTO.excluded.SubqueryAlias("excluded").table_source.constraints()contains defined constraints (!constraints.is_empty()), strictly validates that target columns match aConstraint::PrimaryKeyorConstraint::Unique(failing with"There is no unique or exclusion constraint matching the ON CONFLICT specification"if mismatched). When constraints are empty orNone, allows the operation so real connectors are not blocked.MergeIntoOpwith equality join conditions between target columns andexcludedcolumns.2. MemTable Reference Execution (
datafusion-catalog)TableProvider::merge_intoonMemTablein datafusion/catalog/src/memory/table.rs:0..N-1).HashMap<RowKey, (partition_idx, batch_idx, row_idx)>.DO UPDATEand coalesces duplicates forDO NOTHING.NULLin any conflict column bypass conflict detection and take the insert path.extract_columnto unwrap nestedExpr::AliasandExpr::Castwhen extracting equi-join keys.DmlResultExec.What is the testing strategy for this PR?
End-to-End Sqllogictests:
DO NOTHINGrow skipping.DO UPDATEmodifications with and withoutWHEREpredicates (where excluded.score > users.score).NULLconflict keys bypassing conflict detection (NULL != NULL).DO UPDATEand coalescing onDO NOTHING.(a, b).excluded, non-existent columns, duplicate assignments).MemTable.Unit & Integration Tests:
datafusion/sql/tests/sql_integration.rs: Added integration tests verifying plannedMergeIntoOpstructures, predicates, and aliases forDO NOTHINGandDO UPDATE.datafusion/sql/tests/sql_integration.rs&datafusion/sql/tests/common/mod.rs: Added 6 negative test cases intest_insert_schema_errorschecking mutual exclusivity, non-existent columns, duplicate assignments, and constraint mismatches.Format & Lint:
cargo fmt --all -- --check.cargo clippy -p datafusion-sql -p datafusion-catalog --all-targets --all-features -- -D warningswith zero warnings.Are there any user-facing changes?
INSERT INTO ... ON CONFLICT (...) DO NOTHINGandINSERT INTO ... ON CONFLICT (...) DO UPDATE SET ...statements in SQL.MemTablenow supports executingMERGE INTOqueries in-memory.InsertOpis completely unchanged, and existing connector implementations remain 100% compatible.