adriangb opened a new pull request, #23421:
URL: https://github.com/apache/datafusion/pull/23421
## Which issue does this PR close?
- Closes #22430.
Part of epic #22418. Alternative to #23415 — see "Rationale" for the
relationship.
## Rationale for this change
`ScalarFunctionExpr` is 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`.
That makes it structurally different from the expressions migrated so far.
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 traffic in `ScalarUDF` without a
dependency cycle. #23415 resolves that tension with type erasure (`&dyn Any` /
`Arc<dyn Any>` on the ctx traits, plus a `config_options()` default that
fabricates `ConfigOptions::default()`); I'd rather not stabilize either of
those in a public API, and the fact that only `datafusion-proto` can ever back
those trait methods is the tell that this expression shouldn't ride the
polymorphic hook at all.
This PR draws that line explicitly and keeps everything typed:
- **New `ScalarUdfCodec` trait in `datafusion-expr`** (the crate that owns
`ScalarUDF`): byte-level, format-agnostic UDF (de)serialization —
`try_encode_udf(&ScalarUDF) -> Result<Option<Vec<u8>>>`, `try_decode_udf(name,
payload) -> Result<Arc<ScalarUDF>>`. No proto dependency.
- **`ScalarFunctionExpr::to_proto` / `try_from_proto` as typed inherent
constructors** — colocated with the type exactly as the epic wants, taking the
existing child-expr contexts *plus* `&dyn ScalarUdfCodec` and
`Arc<ConfigOptions>`. `ScalarFunctionExpr` does **not** implement the
`PhysicalExpr::try_to_proto` hook.
- **`datafusion-proto` keeps one small dispatch arm on each side**, now
containing only an adapter (`ExtensionScalarUdfCodec`, wrapping
`PhysicalExtensionCodec` + the task context's `FunctionRegistry` with the same
lookup order as before) and a call to the constructor. The ~30-line inline
bodies are deleted.
This mirrors how aggregate/window UDF expressions are already handled (their
codec resolution stays in `datafusion-proto`; they were excluded from #22418
for exactly this reason), and `ScalarUdfCodec` is the template for typed
`FunctionCodec` variants if those expressions migrate later. It also sets up a
future dedup: `LogicalExtensionCodec` and `PhysicalExtensionCodec` currently
duplicate their `try_{en,de}code_udf/udaf/udwf` methods, which could eventually
be factored onto shared typed codec traits.
The module docs in `physical-expr-common` are updated to state the
architecture line: the hooks/contexts are for **self-contained** expressions;
**session-dependent** expressions expose typed inherent constructors and are
dispatched by the serialization layer that owns the session.
## What changes are included in this PR?
Commit 1:
- `datafusion/expr/src/udf_codec.rs` (new): the `ScalarUdfCodec` trait,
re-exported at the crate root.
Commit 2:
- `datafusion/physical-expr/src/scalar_function.rs`: `to_proto` /
`try_from_proto` inherent constructors (feature-gated `proto`) + a
`proto_tests` module.
- `datafusion/proto/src/physical_plan/mod.rs`: `ExtensionScalarUdfCodec`
adapter (codec + optional registry fallback).
- `datafusion/proto/src/physical_plan/to_proto.rs` / `from_proto.rs`: the
`ScalarFunctionExpr` arms shrink to adapter + dispatch; inline serialization
bodies deleted.
- `datafusion/physical-expr-common/src/physical_expr.rs`: doc-only — records
the self-contained vs. session-dependent line on the `proto_encode` /
`proto_decode` modules.
The wire format is 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?
- New direct tests in `scalar_function.rs::proto_tests` (9): encode happy
path (field-by-field), codec payload passthrough in both directions, decode
happy path, wrong `ExprType` variant, missing `return_type`, child
encode/decode error propagation, and UDF-codec error propagation on both sides.
Note there is no "decode returned the wrong type" test — with a typed codec
that failure mode is unrepresentable.
- `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 constructors.
- `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:
`datafusion_expr::udf_codec::ScalarUdfCodec` and the two inherent constructors
on `ScalarFunctionExpr` (feature `proto`). No existing signatures change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_018MmNo1SfPHXZNaMW26KmaG
--
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]
