Port ScalarFunctionExpr to try_to_proto/try_from_proto via typed ctx function-codec capabilities

[adriangb]

Which issue does this PR close?

• Closes Port ScalarFunctionExpr to use try_to_proto / try_from_proto #22430.

Part of epic #22418. Alternative to #23415 — see "Rationale" for the relationship.

Rationale for this change

ScalarFunctionExpr was the last expression on the central proto downcast chain whose serialization needs session-level objects: the embedded ScalarUDF must go through PhysicalExtensionCodec on encode and resolve via codec + function registry on decode, and the reconstructed expression needs the session's ConfigOptions.

The tension: the try_to_proto / try_from_proto contexts live in datafusion-physical-expr-common, which sits below datafusion-expr in the crate graph (datafusion-expr depends on it for Arc<dyn PhysicalExpr> in the UDAF/UDWF APIs), so the contexts cannot name ScalarUDF without a dependency cycle. #23415 resolves this with caller-visible type erasure — encode_udf(&dyn Any) / decode_udf(..) -> Arc<dyn Any> on the public ctx API, with docs saying "the concrete type is always ScalarUDF" and downcasts at every use — plus a config_options() default that silently fabricates ConfigOptions::default().

This PR takes a different shape: generic, fully-typed capability accessors on the contexts, with the erasure confined to the internal dispatch traits.

// encode side — call site in ScalarFunctionExpr::try_to_proto:
fun_definition: ctx.encode_function(self.fun.as_ref())?,

// decode side — call site in ScalarFunctionExpr::try_from_proto:
let udf: Arc<ScalarUDF> = ctx.decode_function(&node.name, node.fun_definition.as_deref())?;
let config = ctx.config_options()?;

• PhysicalExprEncodeCtx::encode_function<F: Any>(&F) -> Result<Option<Vec<u8>>> and PhysicalExprDecodeCtx::decode_function<F: Any>(name, payload) -> Result<Arc<F>> are generic over the function type. Call sites — including third-party PhysicalExpr implementations that wrap a ScalarUDF (or later an AggregateUDF/WindowUDF) — are fully typed and never see Any or write a downcast.
• The internal dispatch traits (PhysicalExprEncode/PhysicalExprDecode, documented as implemented only by serialization layers) carry monomorphic erased channels (encode_function_erased, decode_function_erased) that route on TypeId and receive type_name for error messages. The ctx verifies the returned type and converts any contract violation into a clean internal error. Which function types a layer supports is an explicit, documented part of its contract; requesting an unsupported type errors naming it.
• PhysicalExprDecodeCtx::config_options() returns Result and the dispatch default errors — a layer without a session must opt in explicitly rather than silently substituting defaults (avoiding the footgun in the ConfigOptions::default() fallback).

With that in place, ScalarFunctionExpr rides the standard hooks like every other migrated expression: try_to_proto inside impl PhysicalExpr, try_from_proto(node, ctx) with the standard signature, one-line dispatch in from_proto.rs, no downcast arm in to_proto.rs at all — the encode chain shrinks by one special case, and datafusion-proto no longer imports ScalarFunctionExpr for encoding.

datafusion-proto's ConverterEncoder/ConverterDecoder implement the erased channels by routing ScalarUDF through try_encode_udf/try_decode_udf with the task-context registry fallback (identical lookup order to the old inline arm). Aggregate/window UDFs slot into the same channels when their expressions migrate.

Honest limitation: which F a layer supports is a runtime contract, not a compile-time one. That is the floor imposed by the crate graph — a compile-time signature would need ScalarUDF below physical-expr-common, which its logical half (simplify, call, preimage) rules out.

What changes are included in this PR?

Commit 1 — datafusion/physical-expr-common/src/physical_expr.rs:

• PhysicalExprEncodeCtx::encode_function / PhysicalExprDecodeCtx::decode_function / PhysicalExprDecodeCtx::config_options (typed public surface).
• PhysicalExprEncode::encode_function_erased / PhysicalExprDecode::decode_function_erased / PhysicalExprDecode::config_options (internal dispatch, erroring defaults so existing implementors keep compiling).
• Module docs describing the capability contract.

Commit 2:

• datafusion/physical-expr/src/scalar_function.rs: try_to_proto in impl PhysicalExpr (feature-gated proto), try_from_proto inherent fn with the standard signature, plus a proto_tests module.
• datafusion/proto/src/physical_plan/to_proto.rs: ConverterEncoder::encode_function_erased; the ScalarFunctionExpr downcast arm is deleted.
• datafusion/proto/src/physical_plan/from_proto.rs: ConverterDecoder::decode_function_erased + config_options; the inline ExprType::ScalarUdf arm becomes the standard one-line dispatch.

The wire format is byte-for-byte unchanged: same PhysicalScalarUdfNode fields, fun_definition only set when the codec writes bytes, return_field_name from the stored return field (identical to the old return_field(&Schema::empty())?.name()), expr_id: None (ScalarFunctionExpr never overrides expression_id, so the old arm always wrote None here too).

Are these changes tested?

• 11 new direct tests in scalar_function.rs::proto_tests, driving try_to_proto through dyn PhysicalExpr (so inherent-method shadowing cannot silently pass): encode happy path (field-by-field), codec payload passthrough in both directions, decode happy path, wrong ExprType variant, missing return_type, child + function-codec error propagation on both sides, the ctx downcast guard against a contract-violating layer, and the no-session config_options error.
• cargo test -p datafusion-proto --test proto_integration: 190 passed. With both inline arms deleted, scalar-UDF roundtrip coverage can only pass through the new hooks.
• cargo fmt --all and cargo clippy --all-targets --all-features -- -D warnings are clean.

Are there any user-facing changes?

No wire-format or behavior changes. New public API: the three typed ctx methods and the three defaulted dispatch-trait methods (all feature proto, all additive/non-breaking). No existing signatures change.

🤖 Generated with Claude Code

https://claude.ai/code/session_018MmNo1SfPHXZNaMW26KmaG

[adriangb]

Hi @timsaucer , would you mind giving this beast a look? Thanks!

[Copilot]

Pull request overview

Ports ScalarFunctionExpr onto the standard PhysicalExpr::try_to_proto / try_from_proto hooks by extending the encode/decode contexts with typed “function codec” capabilities (plus session ConfigOptions access), allowing scalar UDF payloads to be routed through the extension codec without reintroducing crate dependency cycles.

Changes:

• Added typed encode_function / decode_function and config_options helpers on the proto (de)serialization contexts, backed by internal type-erased dispatch.
• Migrated ScalarFunctionExpr to implement try_to_proto (trait override) and try_from_proto (inherent constructor), with comprehensive direct hook tests.
• Removed the special-case ScalarFunctionExpr downcast arm from to_proto.rs and replaced the inline ScalarUDF decode logic in from_proto.rs with standard dispatch.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
datafusion/physical-expr-common/src/physical_expr.rs	Introduces typed context capabilities for function (de)serialization and session config access, backed by erased dispatch traits.
datafusion/physical-expr/src/scalar_function.rs	Implements ScalarFunctionExpr proto hooks and adds direct hook/unit tests.
datafusion/proto/src/physical_plan/to_proto.rs	Implements encode_function_erased and removes the ScalarFunctionExpr downcast serialization arm.
datafusion/proto/src/physical_plan/from_proto.rs	Implements decode_function_erased + config_options and routes ScalarUdf decoding through ScalarFunctionExpr::try_from_proto.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[timsaucer]

This adds a bit of complexity, but not a ton, to the encoding/decoding approach. I'm not a huge fan of having to jump through the erasing to an Arc<dyn Any>. I think as we discussed this is entirely because the circular dependency that would be added, right? Since I'm dealing with a related problem in #23348 where I have to either hoist traits to a higher level crate or do type erasure like you're doing.

The solution here is tactical and it solves your immediate problem, but I'm wondering if we should take a more strategic approach and think about reorganizing some of our traits & crates so that you never had to do that in the first place. What do you think?

[adriangb]

I think as we discussed this is entirely because the circular dependency that would be added, right?

Yes, pretty much. The issue in this case is that PhysicalExpr declares the trait method. For ScalarUDF to serialize itself it needs to call PhysicalExtensionCodec::try_encode_udf. But in order to get a PhysicalExtensionCodec that needs to be passed in via PhysicalExpr::to_proto which results in the requirement that PhysicalExpr reference PhysicalExtensionCodec which then references ScalarUDF. But ScalarUDF also references PhysicalExpr, creating a circular dependency.

The solution here is tactical and it solves your immediate problem, but I'm wondering if we should take a more strategic approach and think about reorganizing some of our traits & crates so that you never had to do that in the first place.

That sounds great. I spent quite some time trying something along these lines, but kept hitting dead ends. Do you have a feeling for this being possible?

[timsaucer]

That sounds great. I spent quite some time trying something along these lines, but kept hitting dead ends. Do you have a feeling for this being possible?

I just spent some time looking at it again and I don't have a great solution at this time :|

[timsaucer]

Looks good.

[timsaucer]

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.

[timsaucer]

Personally I don't see value in tests like this, but my agent insists on adding them.

[timsaucer]

Ordinarily I'd say these kinds of comments can become stale, but I think agents are really good at updating them these days.

[adriangb]

@timsaucer I tried #23449 as an alternative, it seems the worst of all worlds:

• Still need a downcast match & special casing
• Introduces bridge traits / more complexity

The one genuine win it has over main is moving code closer to the expression. But that's minor in this case.

So the real choice is between this PR and doing nothing / leaving it as it is on main. I think it's actually okay to leave it as is and close this as a wontfix until someone comes along and says "I have an expression that wraps ScalarUDF I would like to serialize" or "I have another shape of expression that needs SessionContext / codecs to de-serialize itself.

My guess is we'll end up with a clean implementation for expressions and execution plans but functions (including window, aggregates) will be special handled until we come up with some bigger picture refactor.

So my proposal would be to close this PR, mark #22430 as wontfix and move onto working on execution plans (where I think there is a much more obvious win).

Does that sound good to you?

[timsaucer]

That sounds reasonable. I'm also not opposed to merging this PR