Copilot commented on code in PR #22463:
URL: https://github.com/apache/datafusion/pull/22463#discussion_r3320124198
##########
datafusion/physical-expr/src/expressions/not.rs:
##########
@@ -181,13 +183,111 @@ impl PhysicalExpr for NotExpr {
write!(f, "NOT ")?;
self.arg.fmt_sql(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::NotExpr(Box::new(
+ protobuf::PhysicalNot {
+ expr: Some(Box::new(ctx.encode_child(&self.arg)?)),
+ },
+ ))),
+ }))
+ }
+}
+
+#[cfg(feature = "proto")]
+impl NotExpr {
+ /// Reconstruct a [`NotExpr`] from its protobuf representation.
+ 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_physical_expr_common::expect_expr_variant;
+ use datafusion_proto_models::protobuf;
+
+ let not_expr = expect_expr_variant!(
+ node,
+ protobuf::physical_expr_node::ExprType::NotExpr,
+ "NotExpr",
+ );
+ let expr = ctx
+ .decode_required_expression(not_expr.expr.as_deref(), "NotExpr",
"expr")
+ .map_err(|err| match err {
+ datafusion_common::DataFusionError::Internal(msg)
+ if msg.starts_with("NotExpr is missing required field
'expr'") =>
+ {
+ internal_datafusion_err!(
+ "NotExpr is missing required field 'expr' (expr_id:
{:?}, expr_type: {:?})",
+ node.expr_id,
+ &node.expr_type
+ )
+ }
+ other => other,
+ })?;
Review Comment:
This error-mapping closure is fragile and inconsistent with all other ported
expressions. None of `NegativeExpr`, `CastExpr`, `LikeExpr`, or `InListExpr`
post-process the error from `decode_required_expression` — they just propagate
it (see e.g. `negative.rs:211-212`). Here, the closure matches on the exact
prefix of the underlying `Internal` error message ("NotExpr is missing required
field 'expr'") and then re-emits an error with that same prefix plus extra
context. If `decode_required_expression` ever changes its error wording, this
branch silently falls through to the `other => other` arm and the extra context
is lost without any compile-time signal. Recommend dropping the `.map_err(...)`
entirely and matching the simple pattern used by the sibling ports; if the
additional `expr_id`/`expr_type` context is desired, it should be added inside
`decode_required_expression` (or via a non-string-matching mechanism) so all
callers benefit.
##########
datafusion/physical-expr/src/expressions/not.rs:
##########
@@ -181,13 +183,111 @@ impl PhysicalExpr for NotExpr {
write!(f, "NOT ")?;
self.arg.fmt_sql(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::NotExpr(Box::new(
+ protobuf::PhysicalNot {
+ expr: Some(Box::new(ctx.encode_child(&self.arg)?)),
+ },
+ ))),
+ }))
+ }
+}
+
+#[cfg(feature = "proto")]
+impl NotExpr {
+ /// Reconstruct a [`NotExpr`] from its protobuf representation.
+ 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_physical_expr_common::expect_expr_variant;
+ use datafusion_proto_models::protobuf;
+
+ let not_expr = expect_expr_variant!(
+ node,
+ protobuf::physical_expr_node::ExprType::NotExpr,
+ "NotExpr",
+ );
+ let expr = ctx
+ .decode_required_expression(not_expr.expr.as_deref(), "NotExpr",
"expr")
+ .map_err(|err| match err {
+ datafusion_common::DataFusionError::Internal(msg)
+ if msg.starts_with("NotExpr is missing required field
'expr'") =>
+ {
+ internal_datafusion_err!(
+ "NotExpr is missing required field 'expr' (expr_id:
{:?}, expr_type: {:?})",
+ node.expr_id,
+ &node.expr_type
+ )
+ }
+ other => other,
+ })?;
+
+ Ok(Arc::new(NotExpr::new(expr)))
+ }
}
/// Creates a unary expression NOT
pub fn not(arg: Arc<dyn PhysicalExpr>) -> Result<Arc<dyn PhysicalExpr>> {
Ok(Arc::new(NotExpr::new(arg)))
}
+#[cfg(all(test, feature = "proto"))]
+mod proto_tests {
+ use std::sync::Arc;
+
+ use super::*;
+
+ use arrow::datatypes::Schema;
+ use datafusion_common::DataFusionError;
+ use datafusion_physical_expr_common::physical_expr::proto_decode::{
+ PhysicalExprDecode, PhysicalExprDecodeCtx,
+ };
+ use datafusion_proto_models::protobuf::{
+ PhysicalExprNode, PhysicalNot, physical_expr_node,
+ };
+
+ struct NoopDecoder;
+
+ impl PhysicalExprDecode for NoopDecoder {
+ fn decode(
+ &self,
+ _node: &PhysicalExprNode,
+ _schema: &Schema,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ unreachable!("missing child should be rejected before decoding")
+ }
+ }
+
+ #[test]
+ fn test_from_proto_missing_child() {
+ let node = PhysicalExprNode {
+ expr_id: Some(42),
+ expr_type: Some(physical_expr_node::ExprType::NotExpr(Box::new(
+ PhysicalNot { expr: None },
+ ))),
+ };
+ let schema = Schema::empty();
+ let decoder = NoopDecoder;
+ let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
+
+ let err = NotExpr::try_from_proto(&node, &ctx).unwrap_err();
+ assert!(matches!(err, DataFusionError::Internal(msg)
+ if msg.contains("NotExpr is missing required field 'expr'")
+ && msg.contains("expr_id: Some(42)")
+ && msg.contains("expr_type: Some(NotExpr")));
+ }
+}
Review Comment:
The linked issue explicitly asks for a direct hook test "covering both
directions and a bad-input case". This module only covers the bad-input case —
there is no test that exercises `try_to_proto` (encode) and no happy-path
`try_from_proto` decode test. Compare with `negative.rs`
(`try_from_proto_decodes_negative_expr`,
`try_from_proto_propagates_expr_decode_error`), `cast.rs`, `like.rs`, and
`in_list.rs`, which all include a successful-decode test, a wrong-variant
rejection test, and a child-decode-error propagation test in addition to the
missing-field case. Please add at least a successful round-trip / decode test
and a non-`NotExpr` variant rejection test for parity.
##########
datafusion/physical-expr/src/expressions/not.rs:
##########
@@ -181,13 +183,111 @@ impl PhysicalExpr for NotExpr {
write!(f, "NOT ")?;
self.arg.fmt_sql(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::NotExpr(Box::new(
+ protobuf::PhysicalNot {
+ expr: Some(Box::new(ctx.encode_child(&self.arg)?)),
+ },
+ ))),
+ }))
+ }
+}
+
+#[cfg(feature = "proto")]
+impl NotExpr {
+ /// Reconstruct a [`NotExpr`] from its protobuf representation.
+ 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_physical_expr_common::expect_expr_variant;
+ use datafusion_proto_models::protobuf;
+
+ let not_expr = expect_expr_variant!(
+ node,
+ protobuf::physical_expr_node::ExprType::NotExpr,
+ "NotExpr",
+ );
+ let expr = ctx
+ .decode_required_expression(not_expr.expr.as_deref(), "NotExpr",
"expr")
+ .map_err(|err| match err {
+ datafusion_common::DataFusionError::Internal(msg)
+ if msg.starts_with("NotExpr is missing required field
'expr'") =>
+ {
+ internal_datafusion_err!(
+ "NotExpr is missing required field 'expr' (expr_id:
{:?}, expr_type: {:?})",
+ node.expr_id,
+ &node.expr_type
+ )
+ }
+ other => other,
+ })?;
+
+ Ok(Arc::new(NotExpr::new(expr)))
+ }
}
/// Creates a unary expression NOT
pub fn not(arg: Arc<dyn PhysicalExpr>) -> Result<Arc<dyn PhysicalExpr>> {
Ok(Arc::new(NotExpr::new(arg)))
}
+#[cfg(all(test, feature = "proto"))]
+mod proto_tests {
+ use std::sync::Arc;
+
+ use super::*;
+
+ use arrow::datatypes::Schema;
+ use datafusion_common::DataFusionError;
+ use datafusion_physical_expr_common::physical_expr::proto_decode::{
+ PhysicalExprDecode, PhysicalExprDecodeCtx,
+ };
+ use datafusion_proto_models::protobuf::{
+ PhysicalExprNode, PhysicalNot, physical_expr_node,
+ };
+
+ struct NoopDecoder;
+
+ impl PhysicalExprDecode for NoopDecoder {
+ fn decode(
+ &self,
+ _node: &PhysicalExprNode,
+ _schema: &Schema,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ unreachable!("missing child should be rejected before decoding")
+ }
+ }
+
+ #[test]
+ fn test_from_proto_missing_child() {
+ let node = PhysicalExprNode {
+ expr_id: Some(42),
+ expr_type: Some(physical_expr_node::ExprType::NotExpr(Box::new(
+ PhysicalNot { expr: None },
+ ))),
+ };
+ let schema = Schema::empty();
+ let decoder = NoopDecoder;
+ let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
+
+ let err = NotExpr::try_from_proto(&node, &ctx).unwrap_err();
+ assert!(matches!(err, DataFusionError::Internal(msg)
+ if msg.contains("NotExpr is missing required field 'expr'")
+ && msg.contains("expr_id: Some(42)")
+ && msg.contains("expr_type: Some(NotExpr")));
+ }
+}
+
Review Comment:
`mod proto_tests` is placed before the existing `#[cfg(test)] mod tests` at
line 291. The linked issue specifically calls out that the test module must be
at the end of the file to avoid the clippy `items-after-test-module` lint, and
the established convention in sibling ports puts the proto test module after
the regular `mod tests` (see `like.rs:240` regular `mod test` followed by
`like.rs:346` `mod proto_hook_tests`, and `cast.rs:431` followed by
`cast.rs:1213`). Please move `mod proto_tests` to after `mod tests` to match
that convention.
--
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]
