Re: [PR] feat: Add SQL planner, physical planner, and TableProvider hook for MERGE INTO [datafusion]

["raghav-reglobe (via GitHub)" <gi...@apache.org>]

raghav-reglobe commented on PR #22988:
URL: https://github.com/apache/datafusion/pull/22988#issuecomment-4878506556

   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`):
   
   ```sql
   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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org