timsaucer commented on code in PR #23421:
URL: https://github.com/apache/datafusion/pull/23421#discussion_r3560594685
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -583,6 +612,25 @@ pub mod proto_encode {
pub trait PhysicalExprEncode {
/// Encode an expression to a protobuf node.
fn encode(&self, expr: &Arc<dyn PhysicalExpr>) ->
Result<PhysicalExprNode>;
+
+ /// Erased channel behind [`PhysicalExprEncodeCtx::encode_function`].
+ ///
+ /// Implementations downcast `function` to each function type they
+ /// support (e.g. `datafusion_expr::ScalarUDF` routed through
+ /// `PhysicalExtensionCodec::try_encode_udf`) and error — naming
+ /// `type_name` — for the rest. The default supports none, so
+ /// encoders that never see function-bearing expressions need not
+ /// implement it.
+ fn encode_function_erased(
+ &self,
+ _function: &(dyn Any + Send + Sync),
+ type_name: &'static str,
+ ) -> Result<Option<Vec<u8>>> {
Review Comment:
Here, or somewhere else, let's add a docstring that explains why the erasure
is necessary so future developers are pointed in the direction of the circular
dependency issue.
##########
datafusion/proto/src/physical_plan/to_proto.rs:
##########
@@ -269,6 +268,26 @@ impl
datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprE
self.proto_converter
.physical_expr_to_proto(expr, self.codec)
}
+
+ fn encode_function_erased(
+ &self,
+ function: &(dyn std::any::Any + Send + Sync),
+ type_name: &'static str,
+ ) -> Result<Option<Vec<u8>>> {
+ // Function types supported by this serialization layer. Aggregate
+ // and window UDFs can be added here when their expressions migrate
Review Comment:
Ordinarily I'd say these kinds of comments can become stale, but I think
agents are really good at updating them these days.
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -1292,3 +1419,91 @@ mod proto_helper_tests {
));
}
}
+
+/// Tests for the erroring defaults of the dispatch traits' capability
+/// channels: a serialization layer that implements only the required
+/// `encode`/`decode` must reject function-codec and session-config requests
+/// with errors naming the requested type, not panic or silently succeed.
+#[cfg(all(test, feature = "proto"))]
+mod proto_capability_default_tests {
+ use std::sync::Arc;
+
+ use arrow::datatypes::Schema;
+ use datafusion_common::{DataFusionError, Result};
+ use datafusion_proto_models::protobuf::PhysicalExprNode;
+
+ use crate::physical_expr::PhysicalExpr;
+ use crate::physical_expr::proto_decode::{PhysicalExprDecode,
PhysicalExprDecodeCtx};
+ use crate::physical_expr::proto_encode::{PhysicalExprEncode,
PhysicalExprEncodeCtx};
+
+ struct DefaultOnlyEncoder;
+
+ impl PhysicalExprEncode for DefaultOnlyEncoder {
+ fn encode(&self, _expr: &Arc<dyn PhysicalExpr>) ->
Result<PhysicalExprNode> {
+ unreachable!("child encoding must not be reached")
+ }
+ }
+
+ struct DefaultOnlyDecoder;
+
+ impl PhysicalExprDecode for DefaultOnlyDecoder {
+ fn decode(
+ &self,
+ _node: &PhysicalExprNode,
+ _schema: &Schema,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ unreachable!("child decoding must not be reached")
+ }
+ }
+
+ /// Stands in for a function type (e.g. `ScalarUDF`) the layer was never
+ /// taught about.
+ #[derive(Debug)]
+ struct NotAFunction;
+
+ #[test]
+ fn encode_function_default_errors_naming_requested_type() {
+ let encoder = DefaultOnlyEncoder;
+ let ctx = PhysicalExprEncodeCtx::new(&encoder);
+
+ let err = ctx.encode_function(&NotAFunction).unwrap_err();
+ assert!(matches!(
+ err,
+ DataFusionError::Internal(msg)
+ if msg.contains("does not support encoding function objects")
+ && msg.contains("NotAFunction")
+ ));
+ }
+
+ #[test]
+ fn decode_function_default_errors_naming_requested_type_and_name() {
+ let schema = Schema::empty();
+ let decoder = DefaultOnlyDecoder;
+ let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
+
+ let err = ctx
+ .decode_function::<NotAFunction>("my_udf", None)
+ .unwrap_err();
+ assert!(matches!(
+ err,
+ DataFusionError::Internal(msg)
+ if msg.contains("does not support resolving function objects")
+ && msg.contains("NotAFunction")
+ && msg.contains("'my_udf'")
+ ));
+ }
+
+ #[test]
+ fn config_options_default_errors() {
+ let schema = Schema::empty();
+ let decoder = DefaultOnlyDecoder;
+ let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
+
+ let err = ctx.config_options().unwrap_err();
+ assert!(matches!(
+ err,
+ DataFusionError::Internal(msg)
+ if msg.contains("does not provide session configuration")
+ ));
+ }
Review Comment:
Personally I don't see value in tests like this, but my agent insists on
adding them.
--
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]
