Skip to content

feat: Add SQL planner, physical planner, and TableProvider hook for MERGE INTO#22988

Open
wirybeaver wants to merge 2 commits into
apache:mainfrom
wirybeaver:feature/mergeinto
Open

feat: Add SQL planner, physical planner, and TableProvider hook for MERGE INTO#22988
wirybeaver wants to merge 2 commits into
apache:mainfrom
wirybeaver:feature/mergeinto

Conversation

@wirybeaver

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Follow-up to #20763 (merged) which added MergeIntoOp, MergeIntoClause, and proto types.

Rationale for this change

MERGE INTO (SQL:2003) is a widely-used DML statement for upsert/conditional update workloads. This PR wires the types introduced in #20763 through the SQL planner, physical planner, and TableProvider trait so that table implementations can actually execute merge operations.

What changes are included in this PR?

datafusion/catalogTableProvider trait extension

  • Add merge_into(source, on, clauses) async method with a default not_impl_err impl so existing providers are unaffected.

datafusion/sql — SQL → LogicalPlan

  • statement.rs: parse Statement::Merge into LogicalPlan::Dml with WriteOp::MergeInto.
  • Resolve the target table and plan the USING source into a LogicalPlan.
  • Build a combined target+source schema to resolve ON and WHEN expressions.
  • Convert ON condition and WHEN MATCHED / NOT MATCHED clauses to DataFusion Expr.

datafusion/expr — expression plumbing

  • MergeIntoOp::exprs(): stable iteration over all expressions (ON, then per-clause predicate + action values).
  • MergeIntoOp::with_new_exprs(): rebuild op from a transformed expr vector.
  • Branch LogicalPlan::apply_expressions, map_expressions, and with_new_exprs on WriteOp::MergeInto so optimizers can rewrite merge expressions. Other WriteOp variants are unchanged.

datafusion/core — physical planner dispatch

  • Dispatch WriteOp::MergeInto in the physical planner.
  • Recover the TableProvider via source_as_provider(), extract the source ExecutionPlan, and call TableProvider::merge_into.

Are these changes tested?

  • The SQL planner path is exercised by datafusion/proto/tests/cases/roundtrip_logical_plan.rs (proto round-trip for MergeInto).
  • Unit tests for MergeIntoOp::exprs / with_new_exprs are included in dml.rs.
  • End-to-end integration tests require a TableProvider that implements merge_into; that is left to follow-up once a concrete provider (e.g. Delta Lake) adopts the hook.

Are there any user-facing changes?

  • TableProvider gains a new merge_into method. The default implementation returns not_impl_err, so existing implementations compile without changes.
  • MERGE INTO <table> USING <source> ON <cond> WHEN ... SQL syntax is now accepted by the DataFusion SQL parser and planner.

@github-actions github-actions Bot added sql SQL Planner logical-expr Logical plan and expressions core Core DataFusion crate catalog Related to the catalog crate labels Jun 17, 2026

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wirybeaver
Thanks for the work on MERGE planning. I found two issues that look like they should be fixed before this lands.

let target_table_source = self
.context_provider
.get_table_source(target_table_ref.clone())?;
let target_schema = Arc::new(DFSchema::try_from_qualified_schema(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think target aliases need a bit more handling here. Right now MERGE INTO target AS t USING source AS s ON t.id = s.id ... accepts the alias, but target_schema is still qualified with the table name rather than t. That means normal alias-qualified references in the ON clause or WHEN AND expressions will not resolve.

Could we either reject target aliases explicitly for now, or qualify the target side with the alias so this matches the rest of SQL planning?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — when an alias is present we now use it as the schema qualifier via TableReference::bare(alias.name.value.clone()), so t.col resolves correctly in ON and WHEN expressions.

MergeIntoAction::Update(assignments)
}
ast::MergeAction::Insert(insert_expr) => {
let columns: Vec<String> = insert_expr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the MERGE INSERT actions need the same kind of validation that regular INSERT planning does. At the moment, unknown columns, duplicate columns, and mismatched columns / values lengths can all plan successfully. Empty columns also does not appear to require values for every target column.

That can leave TableProviders receiving malformed actions instead of a validated logical operation. Could we normalize and validate target column names, reject duplicates, and verify that the value count matches either the explicit columns or the full target schema?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — we now validate before building the action: duplicate column names are rejected, each named column is checked against the target schema via field_with_unqualified_name, and the value count must equal either the explicit column count (when a column list is given) or the full target schema width (when none is given).

})
.collect::<Result<Vec<_>>>()?;

// 6. Build the DmlStatement

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small cleanup suggestion: this could return Ok(LogicalPlan::Dml(DmlStatement::new(...))) directly instead of binding let plan and then returning Ok(plan).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, returns Ok(LogicalPlan::Dml(...)) directly now.

.assignments
.into_iter()
.map(|assign| {
let col_name = match &assign.target {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small follow-up suggestion: the update-column extraction and insert-column extraction both walk an ObjectName and take the last identifier. It might be worth adding a small private helper that returns Result<String> and produces a planner error for non-ident parts, rather than using unwrap() at each call site.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — extracted ident_from_object_name_last as a private associated function that returns Result<String> and errors on non-ident parts. Both UPDATE and INSERT column extraction now go through it.

@wirybeaver wirybeaver mentioned this pull request Jun 28, 2026
2 tasks

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wirybeaver
Thanks for the updates here. I think a couple of identifier normalization details still need to be tightened up before this is ready.

Comment thread datafusion/sql/src/statement.rs Outdated
// `MERGE INTO target AS t`. Fall back to the table reference itself.
let target_qualifier = target_alias
.as_ref()
.map(|a| TableReference::bare(a.name.value.clone()))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix to use the target alias as the schema qualifier here. One remaining issue is that this uses a.name.value.clone() directly, so unquoted aliases are not normalized the same way as the rest of planning.

For example, MERGE INTO target AS T ... ON T.id = s.id can store the target qualifier as T, while expression planning normalizes T.id to t.id. That can still make alias-qualified target references fail.

Could we build the alias qualifier with self.ident_normalizer.normalize(a.name.clone()) before passing it to TableReference::bare(...)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — now using self.ident_normalizer.normalize(a.name.clone()) before passing to TableReference::bare, so unquoted aliases like T are lowercased to match expression planning.

Comment thread datafusion/sql/src/statement.rs Outdated
let columns: Vec<String> = insert_expr
.columns
.iter()
.map(|c| Self::ident_from_object_name_last(c))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the MERGE INSERT column handling still needs one more normalization step. The extracted column names currently keep the raw Ident.value from ident_from_object_name_last, so validation happens before SQL identifier normalization.

That means something like MERGE ... INSERT (ID) VALUES (...) can be rejected against a target column named id, and duplicate detection may not match regular INSERT behavior.

Could we normalize the extracted column identifiers before duplicate checks and before the field_with_unqualified_name lookup, matching regular INSERT planning?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — both UPDATE and INSERT column names are now normalized through self.ident_normalizer.normalize(Ident::new(...)) before duplicate checks and schema lookups, matching regular INSERT planning behavior.

@raghav-reglobe

Copy link
Copy Markdown

Took this branch for a spin against a provider-side consumer (we're building an Iceberg SCD2 upsert on this hook), and I think there's one blocking issue the current tests don't reach: any target-column reference in the ON or WHEN expressions fails analysis, so a realistic MERGE can't execute through SessionContext::sql().

Minimal repro (custom provider registered as t, MemTable as batch):

MERGE INTO t USING batch s ON t.id = s.id
WHEN MATCHED THEN UPDATE SET val = s.val
Error: Context("type_coercion", SchemaError(FieldNotFound { field: Column { relation: Some(Bare { table: "t" }), name: "id" }, valid_fields: [s.id, s.val, …] }))

What's happening: the Dml node's only input is the planned USING source, but MergeIntoOp's expressions reference the combined target×source namespace. Since this PR wires those expressions into the generic tree-node traversal (tree_node.rs), the analyzer's TypeCoercion pass rewrites them against the node's input schema — which is source-only — and errors on the first target reference. As a probe, ON s.id = s.id (source-only) sails through analysis and reaches merge_into() fine, which pins the root cause. Everything else held up nicely in testing, for what it's worth — a window-function subquery as the USING source (ROW_NUMBER/LEAD), multi-condition ON, clause predicates, and the INSERT validation all plan exactly as expected.

Two directions I can see:

  1. Minimal: don't expose MergeIntoOp exprs through the generic traversal (keep exprs()/with_new_exprs as inherent methods only). The analyzer/optimizer then leave the node alone, and providers receive un-coerced expressions — acceptable for providers that do their own resolution, and it ships a working v1.
  2. Fuller: record the target's resolution qualifier on MergeIntoOp (the alias when present, else the table reference — today the alias is dropped after planning), and teach TypeCoercion to resolve MergeInto exprs against target-schema ⋈ input-schema. Coercion on these expressions genuinely matters (e.g. an Int32 target key against an Int64 window output in ON), and the recorded qualifier also gives providers an explicit way to tell target references from source references instead of inferring by absence.

I'd lean toward 2, but 1 seems fine to unblock if you'd rather keep this PR small. Happy to share the test module I used — a CaptureMergeProvider in the style of dml_planning.rs with basic + SCD2-shaped (window-subquery USING, triple-condition ON, clause predicate) cases — if it's useful as a starting point for end-to-end coverage here. Also happy to put up a patch for either direction if that helps.

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wirybeaver
Thanks for the updates. I think there are still a couple of issues that need another pass before this is ready.

}
_ => Ok(TreeNodeRecursion::Continue),
},
LogicalPlan::Dml(DmlStatement {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid exposing MergeIntoOp expressions through generic logical-plan expression traversal for now.

The analyzer and type coercion rewrite schema is built from plan.inputs(), but the Dml node only has the USING source plan as input. MERGE ON and WHEN expressions are planned against both the target and source schemas, so a query like MERGE INTO t USING s ON t.id = s.id can fail during SessionContext::sql() analysis because t.id is not present in the source-only schema.

Could we either keep these expressions out of generic traversal for the initial implementation, or store enough target schema and qualifier information on the MERGE operation so analyzer/type coercion can rewrite them against the combined target plus source schema?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with storing target schema info on the MERGE operation. Added target_schema: DFSchemaRef to MergeIntoOp, populated from the qualified target schema computed during SQL planning. TypeCoercion and ApplyFunctionRewrites now merge merge_op.target_schema into the schema they build from plan.inputs() (same pattern already used for TableScan's source schema), so both target and source columns are resolvable when coercing MERGE ON/WHEN expressions. For proto, target_schema isn't serialized — it's reconstructed from table_name + target.schema() at deserialization time, the same way Join/Union reconstruct their schemas from inputs rather than serializing them.

Comment thread datafusion/sql/src/statement.rs Outdated
.iter()
.map(|c| {
let raw = Self::ident_from_object_name_last(c)?;
Ok(self.ident_normalizer.normalize(Ident::new(raw)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this one. I think the current fix still has a quoted-identifier edge case.

The UPDATE/INSERT column extraction now returns only Ident.value, then reconstructs it with Ident::new(raw) before normalization. That loses quote_style, so quoted identifiers get normalized as if they were unquoted. For example, INSERT ("ID") or UPDATE SET "ID" = ... can become id, fail to match a target column named ID, or be treated as a duplicate of unquoted ID.

Regular INSERT preserves the original Ident and passes that to ident_normalizer.normalize. Could we make this helper return the last Ident, or otherwise preserve the quote style, and normalize the original ident before duplicate checks and schema lookup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed. ident_from_object_name_last now returns the last Ident (preserving quote_style) instead of a bare String. Both UPDATE and INSERT column extraction pass that Ident straight into self.ident_normalizer.normalize(), so quoted "ID" stays ID and unquoted ID normalizes to id, matching regular INSERT/UPDATE behavior.

@github-actions github-actions Bot added optimizer Optimizer rules proto Related to proto crate labels Jul 10, 2026
@kosiew

kosiew commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

hi @wirybeaver
Can you resolve the merge conflicts?

Add merge_into async method to TableProvider trait for MERGE INTO
DML support. The method accepts:
- source: ExecutionPlan representing the USING clause
- on: Expr representing the ON join condition
- clauses: Vec<MergeIntoClause> for WHEN MATCHED/NOT MATCHED actions

Default implementation returns not_impl_err for tables that don't
support MERGE INTO operations.
Implement merge_to_plan and merge_clause_to_plan in SQL planner:
- Parse Statement::Merge into LogicalPlan::Dml with WriteOp::MergeInto
- Resolve target table and plan source (USING clause) as LogicalPlan
- Build combined schema for target + source to resolve ON and WHEN expressions
- Convert ON condition and WHEN clauses to DataFusion Expr
- Handle UPDATE, INSERT, and DELETE actions in WHEN clauses

Add physical planner dispatch for WriteOp::MergeInto:
- Use source_as_provider() to recover the TableProvider from the TableSource
- Extract source ExecutionPlan from children
- Call TableProvider::merge_into with source plan, ON condition, and clauses
- Wrap errors with MERGE INTO operation context

Wire MergeInto's expressions through LogicalPlan tree-traversal so
optimizers can rewrite them: add MergeIntoOp::exprs() (stable iteration
order: on, then per-clause predicate + action value Exprs) and
MergeIntoOp::with_new_exprs() to rebuild the op from a transformed
expr vector. Branch LogicalPlan::apply_expressions, map_expressions,
and with_new_exprs on WriteOp::MergeInto to use these helpers; other
WriteOp variants continue to expose no expressions as before.
@github-actions

Copy link
Copy Markdown

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details
     Cloning apache/main
    Building datafusion v54.0.0 (current)
       Built [ 118.159s] (current)
     Parsing datafusion v54.0.0 (current)
      Parsed [   0.036s] (current)
    Building datafusion v54.0.0 (baseline)
       Built [ 107.862s] (baseline)
     Parsing datafusion v54.0.0 (baseline)
      Parsed [   0.036s] (baseline)
    Checking datafusion v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.605s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 228.654s] datafusion
    Building datafusion-catalog v54.0.0 (current)
       Built [  40.399s] (current)
     Parsing datafusion-catalog v54.0.0 (current)
      Parsed [   0.025s] (current)
    Building datafusion-catalog v54.0.0 (baseline)
       Built [  41.198s] (baseline)
     Parsing datafusion-catalog v54.0.0 (baseline)
      Parsed [   0.028s] (baseline)
    Checking datafusion-catalog v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.161s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  82.815s] datafusion-catalog
    Building datafusion-expr v54.0.0 (current)
       Built [  28.576s] (current)
     Parsing datafusion-expr v54.0.0 (current)
      Parsed [   0.077s] (current)
    Building datafusion-expr v54.0.0 (baseline)
       Built [  28.510s] (baseline)
     Parsing datafusion-expr v54.0.0 (baseline)
      Parsed [   0.079s] (baseline)
    Checking datafusion-expr v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   1.363s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field MergeIntoOp.target_schema in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/dml.rs:311
  field MergeIntoOp.target_schema in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/dml.rs:311
  field MergeIntoOp.target_schema in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/dml.rs:311
  field MergeIntoOp.target_schema in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/dml.rs:311

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  59.576s] datafusion-expr
    Building datafusion-optimizer v54.0.0 (current)
       Built [  28.622s] (current)
     Parsing datafusion-optimizer v54.0.0 (current)
      Parsed [   0.030s] (current)
    Building datafusion-optimizer v54.0.0 (baseline)
       Built [  27.854s] (baseline)
     Parsing datafusion-optimizer v54.0.0 (baseline)
      Parsed [   0.032s] (baseline)
    Checking datafusion-optimizer v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.182s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  57.507s] datafusion-optimizer
    Building datafusion-proto v54.0.0 (current)
       Built [  62.873s] (current)
     Parsing datafusion-proto v54.0.0 (current)
      Parsed [   0.018s] (current)
    Building datafusion-proto v54.0.0 (baseline)
       Built [  61.908s] (baseline)
     Parsing datafusion-proto v54.0.0 (baseline)
      Parsed [   0.018s] (baseline)
    Checking datafusion-proto v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.248s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 126.452s] datafusion-proto
    Building datafusion-sql v54.0.0 (current)
       Built [  43.432s] (current)
     Parsing datafusion-sql v54.0.0 (current)
      Parsed [   0.031s] (current)
    Building datafusion-sql v54.0.0 (baseline)
       Built [  43.840s] (baseline)
     Parsing datafusion-sql v54.0.0 (baseline)
      Parsed [   0.033s] (baseline)
    Checking datafusion-sql v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.256s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  88.955s] datafusion-sql

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change catalog Related to the catalog crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules proto Related to proto crate sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants