Copilot commented on code in PR #22569:
URL: https://github.com/apache/datafusion/pull/22569#discussion_r3311741862
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -298,6 +298,63 @@ impl PhysicalExpr for CastExpr {
write!(f, ")")
}
+
+ #[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;
+
+ Ok(Some(protobuf::PhysicalExprNode {
+ expr_id: None,
+ expr_type:
Some(protobuf::physical_expr_node::ExprType::Cast(Box::new(
+ protobuf::PhysicalCastNode {
+ expr: Some(Box::new(ctx.encode_child(self.expr())?)),
+ arrow_type: Some(self.cast_type().try_into()?),
+ },
+ ))),
+ }))
Review Comment:
This hard-codes expr_id to None. Previously the proto layer populated
expr_id (via the local expr_id variable) when encoding CastExpr, so this is a
behavioral change that can break any downstream logic that relies on IDs being
present/stable. Prefer to have the encoder layer set expr_id after calling
try_to_proto, or plumb an expr_id into the encode context and use it
here—whichever convention is used by other migrated expressions.
##########
datafusion/proto/src/physical_plan/from_proto.rs:
##########
@@ -364,17 +364,7 @@ pub fn parse_physical_expr_with_converter(
})
.transpose()?,
)?),
- ExprType::Cast(e) => Arc::new(CastExpr::new(
- parse_required_physical_expr(
- e.expr.as_deref(),
- ctx,
- "expr",
- input_schema,
- proto_converter,
- )?,
- convert_required!(e.arrow_type)?,
- None,
- )),
+ ExprType::Cast(_) => CastExpr::try_from_proto(proto, &decode_ctx)?,
Review Comment:
CastExpr::try_from_proto is behind #[cfg(feature = \"proto\")] in
datafusion-physical-expr, but this call site is unconditional. If any build
enables the proto crate without enabling the matching dependency feature flag,
this will fail to compile. Ensure the Cargo feature wiring enables
datafusion-physical-expr's proto feature whenever this module is built, or gate
this match arm/module with the same cfg.
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -298,6 +298,63 @@ impl PhysicalExpr for CastExpr {
write!(f, ")")
}
+
+ #[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;
+
+ Ok(Some(protobuf::PhysicalExprNode {
+ expr_id: None,
+ expr_type:
Some(protobuf::physical_expr_node::ExprType::Cast(Box::new(
+ protobuf::PhysicalCastNode {
+ expr: Some(Box::new(ctx.encode_child(self.expr())?)),
+ arrow_type: Some(self.cast_type().try_into()?),
+ },
+ ))),
+ }))
+ }
+}
+
+#[cfg(feature = "proto")]
+impl CastExpr {
+ /// Reconstruct a [`CastExpr`] from its protobuf representation.
+ ///
+ /// Takes the whole [`PhysicalExprNode`] so the decode signature matches
+ /// other migrated expressions and can inspect outer-node metadata if
+ /// needed in the future.
+ ///
+ /// [`PhysicalExprNode`]:
datafusion_proto_models::protobuf::PhysicalExprNode
+ pub fn try_from_proto(
+ node: &datafusion_proto_models::protobuf::PhysicalExprNode,
+ ctx:
&datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx<'_>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ use datafusion_common::DataFusionError;
+ use datafusion_common::internal_err;
+ use datafusion_proto_models::protobuf;
+
+ let cast_expr = match &node.expr_type {
+ Some(protobuf::physical_expr_node::ExprType::Cast(cast_expr)) => {
+ cast_expr.as_ref()
+ }
+ _ => return internal_err!("PhysicalExprNode is not a CastExpr"),
+ };
+
+ let expr = ctx.decode_required_expression(
+ cast_expr.expr.as_deref(),
+ "CastExpr",
+ "expr",
+ )?;
+ let arrow_type = cast_expr.arrow_type.as_ref().ok_or_else(|| {
+ DataFusionError::Internal(
+ "CastExpr is missing required field 'arrow_type'".to_string(),
+ )
+ })?;
Review Comment:
This uses a manually constructed DataFusionError::Internal while nearby
errors use internal_err! (and the decode ctx likely has helpers for required
fields). Using the same error helper/macro here would keep error formatting
consistent across expressions and reduce duplicated boilerplate.
--
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]
