Port ScalarFunctionExpr to use try_to_proto / try_from_proto#23415
Closed
Olawoyin007 wants to merge 1 commit into
Closed
Port ScalarFunctionExpr to use try_to_proto / try_from_proto#23415Olawoyin007 wants to merge 1 commit into
ScalarFunctionExpr to use try_to_proto / try_from_proto#23415Olawoyin007 wants to merge 1 commit into
Conversation
Sub-issue of apache#22418; closes apache#22430. ScalarFunctionExpr needs the extension codec and session config, so this also adds the ctx helpers discussed on the epic: encode_udf on PhysicalExprEncodeCtx, decode_udf and config_options on PhysicalExprDecodeCtx. The UDF signatures are type-erased because datafusion-expr depends on datafusion-physical-expr-common, so the ctx traits cannot name ScalarUDF without a dependency cycle; the downcasts live in ConverterEncoder and ScalarFunctionExpr::try_from_proto and fail with clean internal errors. Both central arms (to_proto.rs downcast, from_proto.rs inline decode) are deleted in this change, so the roundtrip tests only pass through the new hooks. Wire format is unchanged.
Author
|
Closing in favor of #23421 - the typed 🤖 Generated with Claude Code |
Contributor
|
Hi @Olawoyin007 , thanks for your work though. Would you be interested in reviewing #23421? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
ScalarFunctionExprto usetry_to_proto/try_from_proto#22430.Part of epic #22418.
Rationale for this change
ScalarFunctionExprwas still serialized through the central downcast chain indatafusion-proto. This ports it to own its protobuf (de)serialization via thetry_to_proto/try_from_protohooks (#21929), following the helper pattern from #22596.Unlike the previously migrated expressions,
ScalarFunctionExprneeds the extension codec (UDF payloads) and the session config on the decode side. Per the discussion on #22418, this PR extends the encode/decode contexts with those helpers in the same change:PhysicalExprEncodeCtx::encode_udf- routes a scalar UDF throughPhysicalExtensionCodec::try_encode_udf, returning the opaquefun_definitionpayload (Nonewhen the codec writes nothing).PhysicalExprDecodeCtx::decode_udf- resolves a UDF from the payload viatry_decode_udf, falling back to the task context's function registry by name (same lookup order as the old inline arm).PhysicalExprDecodeCtx::config_options- sessionConfigOptionsfor expressions that carry them.On the type-erased UDF signatures: as flagged in the issue discussion,
ScalarUDFlives indatafusion-expr, which depends ondatafusion-physical-expr-common, so the ctx traits cannot name it without a dependency cycle.encode_udftakes&(dyn Any + Send + Sync)anddecode_udfreturnsArc<dyn Any + Send + Sync>; the concrete type is always (Arc<)ScalarUDFand both sides are documented as such. The downcasts live in one place each (ConverterEncoderindatafusion-proto,ScalarFunctionExpr::try_from_proto) and fail with a clean internal error rather than a panic. Happy to change the shape if a different indirection is preferred.The new trait methods have default implementations (erroring for the UDF pair, default options for
config_options) so existingPhysicalExprEncode/PhysicalExprDecodeimplementors, including the test stubs, keep compiling.What changes are included in this PR?
datafusion/physical-expr-common/src/physical_expr.rs: the three ctx/trait helpers above.datafusion/physical-expr/src/scalar_function.rs:try_to_protoinsideimpl PhysicalExpr for ScalarFunctionExpr(feature-gatedproto),try_from_protoas an inherent fn, plus aproto_testsmodule at the end of the file.datafusion/proto/src/physical_plan/to_proto.rs:ConverterEncoder::encode_udf; theScalarFunctionExprdowncast arm is deleted.datafusion/proto/src/physical_plan/from_proto.rs:ConverterDecoder::decode_udf/config_options; the inlineExprType::ScalarUdfarm now dispatches toScalarFunctionExpr::try_from_proto.datafusion/physical-expr/src/proto_test_util.rs:StubEncodergets anencode_udfreturningOk(None)(a codec that writes nothing).The wire format is unchanged: same
PhysicalScalarUdfNodefields,expr_id: None,fun_definitiononly set when the codec wrote bytes,return_field_namefrom the stored return field (identical to the oldreturn_field(&Schema::empty())?.name()sinceScalarFunctionExpr::return_fieldreturns the stored field).Are these changes tested?
scalar_function.rs::proto_tests: encode happy path (field-by-field node assertion), decode happy path, wrongExprTypevariant, missingreturn_type, child encode/decode error propagation, and adecode_udf-returns-wrong-type case for the downcast guard.cargo test -p datafusion-proto --test proto_integrationpasses - with both central arms deleted, the roundtrip coverage for scalar UDFs can only pass through the new hooks.cargo fmt --allandcargo clippy --all-targets --all-features -- -D warningsare clean.Are there any user-facing changes?
No changes to the wire format or public behavior.
PhysicalExprEncode/PhysicalExprDecodegain defaulted methods (non-breaking);PhysicalExprEncodeCtx/PhysicalExprDecodeCtxgain new helper methods.🤖 Generated with Claude Code