kosiew commented on code in PR #24429:
URL: https://github.com/apache/datafusion/pull/24429#discussion_r3835625064
##########
datafusion/proto-models/src/generated/prost.rs:
##########
@@ -527,13 +527,17 @@ pub mod dml_node {
}
}
}
-/// Carries the ON condition and WHEN clauses of a MERGE INTO operation.
+/// Carries the target qualifier, ON condition, and WHEN clauses of a MERGE
INTO operation.
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct MergeIntoOpNode {
#[prost(message, optional, boxed, tag = "1")]
pub on:
::core::option::Option<::prost::alloc::boxed::Box<LogicalExprNode>>,
#[prost(message, repeated, tag = "2")]
pub clauses: ::prost::alloc::vec::Vec<MergeIntoClauseNode>,
+ /// SQL-visible target qualifier. Absent in payloads written before this
field
+ /// was introduced; readers then fall back to DmlNode.table_name.
+ #[prost(message, optional, tag = "3")]
+ pub target_qualifier: ::core::option::Option<TableReference>,
Review Comment:
It would be good to cover this generated-type source break in the 55.1
migration material too. Downstream callers constructing `MergeIntoOpNode` with
a struct literal now need to provide `target_qualifier`, normally as
`Some(...)`.
I would keep the existing wire-compatibility explanation alongside that
note. New readers can still accept old payloads through the
`DmlNode.table_name` fallback, while old readers cannot safely preserve the
alias semantics carried by new payloads.
##########
datafusion/expr/src/logical_plan/dml.rs:
##########
@@ -299,15 +311,71 @@ impl Display for InsertOp {
}
/// Describes a MERGE INTO operation's parameters.
+///
+/// [`Self::target_qualifier`] is the SQL-visible relation name used by
+/// expressions. The target's catalog/provider identity remains in
+/// [`DmlStatement::table_name`].
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
+#[non_exhaustive]
Review Comment:
Could we make sure the 55.1 migration or release note includes the upgrade
path for this API change? Downstream callers using `MergeIntoOp { on, clauses
}` will now need to switch to `MergeIntoOp::new(target_qualifier, on, clauses)`.
`cargo-semver-checks` correctly reports this as a source break. Since #24462
accepts this PR as planned 55.1 content. It would be useful for the note to
explain why the new argument exists: the SQL-visible target qualifier is now
intentionally separate from the target provider identity.
##########
datafusion/core/tests/sql/sql_api.rs:
##########
Review Comment:
Could we move this SQL-only boolean-condition case into `merge_into.slt` as
well? The non-boolean `ON` and `WHEN` cases are already covered there, and this
test does not inspect bindings, protobuf state, or physical column indices.
That would also let us remove `assert_merge_physical_error`. I see this as
test maintenance rather than a correctness blocker.
--
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]
