timsaucer commented on code in PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#discussion_r3254905345
##########
datafusion/proto/src/logical_plan/file_formats.rs:
##########
@@ -79,8 +80,8 @@ impl CsvOptionsProto {
}
}
-impl From<&CsvOptionsProto> for CsvOptions {
- fn from(proto: &CsvOptionsProto) -> Self {
Review Comment:
Same argument holds for json options, parquet, etc
##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -152,19 +154,25 @@ impl From<&protobuf::StringifiedPlan> for StringifiedPlan
{
}
}
-impl TryFrom<protobuf::WindowFrame> for WindowFrame {
Review Comment:
Ok, so I spent way more time than I wanted to understand the problem with
the dependencies and why you had to introduce this.
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -610,6 +610,124 @@ impl PhysicalExpr for BinaryExpr {
write!(f, " {} ", self.op)?;
write_child(f, self.right.as_ref(), precedence)
}
+
+ #[cfg(feature = "proto")]
+ fn try_to_proto(
+ &self,
+ ctx:
&datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx<'_>,
+ ) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {
+ use datafusion_proto_models::protobuf;
+
+ // Linearize a nested binary expression tree of the same operator
+ // into a flat vector of operands to avoid deep recursion in proto.
+ let op = self.op;
+ let mut operand_refs: Vec<&Arc<dyn PhysicalExpr>> = vec![&self.right];
+ let mut current_expr: &BinaryExpr = self;
+ loop {
+ match current_expr.left.downcast_ref::<BinaryExpr>() {
+ Some(bin) if bin.op == op => {
+ operand_refs.push(&bin.right);
+ current_expr = bin;
+ }
+ _ => {
+ operand_refs.push(¤t_expr.left);
+ break;
+ }
+ }
+ }
+ // Reverse so operands are ordered from left innermost to right
outermost.
+ operand_refs.reverse();
+
+ let operands = operand_refs
+ .iter()
+ .map(|e| ctx.encode_child(e))
+ .collect::<Result<Vec<_>>>()?;
+
+ Ok(Some(protobuf::PhysicalExprNode {
+ expr_id: None,
+ expr_type: Some(protobuf::physical_expr_node::ExprType::BinaryExpr(
+ Box::new(protobuf::PhysicalBinaryExprNode {
+ l: None,
+ r: None,
+ op: format!("{op:?}"),
+ operands,
+ }),
+ )),
+ }))
+ }
+}
+
+#[cfg(feature = "proto")]
+impl BinaryExpr {
+ /// Reconstruct a [`BinaryExpr`] (or a left-deep tree of them when the
proto
+ /// uses the linearized `operands` form) from its protobuf representation.
+ ///
+ /// Takes the whole [`PhysicalExprNode`] — the exact inverse of what
+ /// [`PhysicalExpr::try_to_proto`] produces — so every expression's
+ /// `try_from_proto` shares one signature. The operator string is parsed
+ /// via the canonical [`Operator::from_proto_name`] mapping, so no `op`
+ /// argument needs to be threaded in by the caller.
+ ///
+ /// [`PhysicalExprNode`]:
datafusion_proto_models::protobuf::PhysicalExprNode
+ /// [`PhysicalExpr::try_to_proto`]:
datafusion_physical_expr_common::physical_expr::PhysicalExpr::try_to_proto
+ /// [`PhysicalExprDecodeCtx::decode`]:
datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx::decode
Review Comment:
Seems like some generic text that doesn't necessarily need to be in each
implementation
##########
datafusion/proto/src/logical_plan/file_formats.rs:
##########
@@ -79,8 +80,8 @@ impl CsvOptionsProto {
}
}
-impl From<&CsvOptionsProto> for CsvOptions {
- fn from(proto: &CsvOptionsProto) -> Self {
Review Comment:
On second thought, `from_factory` wasn't `pub` so maybe this argument is
moot.
##########
datafusion/proto/src/logical_plan/file_formats.rs:
##########
@@ -79,8 +80,8 @@ impl CsvOptionsProto {
}
}
-impl From<&CsvOptionsProto> for CsvOptions {
- fn from(proto: &CsvOptionsProto) -> Self {
Review Comment:
For this and also `from_factory` probably needs a line in the upgrade doc
##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -152,19 +154,25 @@ impl From<&protobuf::StringifiedPlan> for StringifiedPlan
{
}
}
-impl TryFrom<protobuf::WindowFrame> for WindowFrame {
Review Comment:
TBH Since the methods are so similar it feels like keeping `impl TryFrom` is
better.
--
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]
