wirybeaver commented on code in PR #22988:
URL: https://github.com/apache/datafusion/pull/22988#discussion_r3614960602
##########
datafusion/expr/src/logical_plan/dml.rs:
##########
@@ -299,12 +299,129 @@ impl Display for InsertOp {
}
/// Describes a MERGE INTO operation's parameters.
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct MergeIntoOp {
/// The join condition from `ON <expr>`.
pub on: Expr,
/// The WHEN clauses, in the order they appeared in the SQL.
pub clauses: Vec<MergeIntoClause>,
+ /// Schema of the target table (qualified with the alias or table name used
+ /// in the SQL). Stored here so the analyzer can build a combined
+ /// target+source schema for type coercion and function rewrites.
+ pub target_schema: DFSchemaRef,
Review Comment:
Good call — done. I removed the `target_schema` field from `MergeIntoOp`, so
it's back to `{ on, clauses }` and the struct is no longer SemVer-breaking to
construct with a struct literal.
The target schema is now **derived on demand** in the analyzer passes (type
coercion and function rewrites) from the existing `DmlStatement { table_name,
target }`, mirroring how `TableScan` already merges its source schema in the
same functions:
```rust
if let LogicalPlan::Dml(DmlStatement {
op: WriteOp::MergeInto(_),
table_name,
target,
..
}) = &plan
{
let target_schema =
DFSchema::try_from_qualified_schema(table_name.clone(),
&target.schema())?;
schema.merge(&target_schema);
}
```
To make that derivation sufficient, SQL planning now canonicalizes target
alias-qualified column references (`t.col`) to the real target table qualifier
before storing the plan, so no alias/planning state leaks onto the public
struct. Added a planner test (`plan_merge_into_canonicalizes_target_alias`)
verifying `MERGE INTO j1 AS t USING j2 AS s ON t.j1_id = s.j2_id ...` produces
`j1.j1_id = s.j2_id` (target alias rewritten, source alias preserved).
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -1258,11 +1258,21 @@ impl AsLogicalPlan for LogicalPlanNode {
.build()
}
LogicalPlanType::Dml(dml_node) => {
- let write_op =
+ let table_name =
+ from_table_reference(dml_node.table_name.as_ref(), "DML
")?;
+ let target = to_table_source(&dml_node.target, ctx,
extension_codec)?;
+ let mut write_op =
from_proto::parse_write_op(dml_node, ctx,
extension_codec)?;
+ if let WriteOp::MergeInto(ref mut merge_op) = write_op {
+ merge_op.target_schema =
Review Comment:
Agreed, and I went with your preferred approach: canonicalize target alias
references to the real target table qualifier during SQL planning, then avoid
serializing any extra state.
`merge_to_plan` now rewrites target-side columns so `t.id` is stored as
`target.id` (source aliases like `s.id` are left untouched). Because the stored
expressions always use the `table_name` qualifier, there's nothing
alias-specific left to lose across a proto roundtrip — so I removed this
reconstruction block entirely. Proto deserialization just rebuilds the
`DmlStatement` from `table_name` + `target`, and the analyzer derives the
target schema from those on the fly when it needs to resolve MERGE expressions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
