wirybeaver commented on code in PR #20763:
URL: https://github.com/apache/datafusion/pull/20763#discussion_r3214252260
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -299,13 +299,63 @@ message DmlNode{
INSERT_OVERWRITE = 4;
INSERT_REPLACE = 5;
TRUNCATE = 6;
+ MERGE_INTO = 7;
}
Type dml_type = 1;
LogicalPlanNode input = 2;
TableReference table_name = 3;
LogicalPlanNode target = 5;
+ // Populated only when dml_type == MERGE_INTO.
+ MergeIntoOpNode merge_into = 6;
Review Comment:
Done in the amended commit. Ran `datafusion/proto/regen.sh`; prost-build
boxed two fields — `DmlNode.merge_into` (`Option<Box<MergeIntoOpNode>>`) and
`MergeIntoOpNode.on` (`Option<Box<LogicalExprNode>>`). Updated
`serialize_merge_into_op` and the `LogicalPlanType::Dml` arm in `mod.rs` to
wrap the payload in `Box::new`; on the read side `parse_write_op` uses
`Option::as_deref()` to strip the `Box` transparently before handing off to
`parse_merge_into_op`.
##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -243,10 +245,137 @@ impl From<protobuf::dml_node::Type> for WriteOp {
protobuf::dml_node::Type::InsertReplace =>
WriteOp::Insert(InsertOp::Replace),
protobuf::dml_node::Type::Ctas => WriteOp::Ctas,
protobuf::dml_node::Type::Truncate => WriteOp::Truncate,
+ // MERGE_INTO carries a payload (`MergeIntoOpNode`) that this
+ // tag-only conversion cannot read; callers must use
+ // [`parse_write_op`] for `DmlNode`s with a MergeInto payload.
+ protobuf::dml_node::Type::MergeInto => unreachable!(
+ "WriteOp::MergeInto requires the MergeIntoOpNode payload; use
parse_write_op",
Review Comment:
Agreed. Removed `From<protobuf::dml_node::Type> for WriteOp` entirely and
inlined the match into `parse_write_op` — that was the only caller, so dropping
the impl removes the panic surface without losing functionality. For symmetry I
also dropped `From<&WriteOp> for protobuf::dml_node::Type` (its only production
caller is the `LogicalPlanType::Dml` arm in `mod.rs`, which now matches
`WriteOp` explicitly). This also avoids re-introducing a wildcard
`unreachable!` arm there after marking `WriteOp` `#[non_exhaustive]` (see the
next thread).
##########
datafusion/expr/src/logical_plan/dml.rs:
##########
@@ -239,6 +239,8 @@ pub enum WriteOp {
Ctas,
/// `TRUNCATE` operation
Truncate,
+ /// `MERGE INTO` operation
+ MergeInto(MergeIntoOp),
Review Comment:
Marked `WriteOp` `#[non_exhaustive]` in the amended commit. The current
`MergeInto` add is still a one-time SemVer break that the next minor accepts,
but `#[non_exhaustive]` means future variants — the planner work in #20746 will
likely add more — are no longer a break for downstream matchers. The bot’s
other findings (the proto-generated `DmlNode.merge_into` field add and
`dml_node::Type::MergeInto` enum variant) are inherent to extending the proto
schema and aren’t avoidable from the Rust side.
--
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]
