kosiew commented on code in PR #22988:
URL: https://github.com/apache/datafusion/pull/22988#discussion_r3607281710
##########
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:
Could we avoid adding `target_schema` as a new public field on `MergeIntoOp`?
Because `MergeIntoOp` is a public struct that downstream users can construct
with struct literals, adding a required field is a SemVer-breaking change. This
also matches the issue reported by the cargo-semver-checks bot.
I do not think that break is necessary for this PR. `target_schema` looks
like analyzer and planning context rather than essential public MERGE operation
data. Exposing it here makes downstream callers provide internal state they
should not need to know about.
Please keep the additional planning state outside the publicly constructible
struct, or introduce a SemVer-compatible constructor or internal representation
before requiring new state from callers. One possible approach is to keep
`MergeIntoOp { on, clauses }`, derive the target schema in the analyzer, type
coercion, or function rewrite passes from the existing `DmlStatement {
table_name, target, .. }`, and canonicalize alias-qualified target references
during SQL planning.
##########
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:
I think this reconstruction loses the target alias used during SQL
expression resolution.
For example, with `MERGE INTO target AS t USING source AS s ON t.id = s.id`,
the expressions may still refer to `t.id` after a proto roundtrip. However,
this code rebuilds `target_schema` using `table_name`, so the qualifier becomes
`target` instead of `t`. A later analyzer, type coercion, or function rewrite
pass that relies on this schema may then fail to resolve the target-side
columns.
Could we either preserve the target resolution qualifier during
serialization or reconstruct it from serialized plan state that contains the
SQL alias? My preference would be to avoid serializing this additional state by
canonicalizing target alias references to the real target table qualifier
before storing the logical plan. That would allow proto deserialization to
safely rebuild the schema from `DmlStatement.table_name` and `target.schema()`.
--
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]
