adriangb opened a new pull request, #23449:
URL: https://github.com/apache/datafusion/pull/23449
## Which issue does this PR close?
- Part of epic #22418; addresses the same goal as #22430.
**Alternative to #23421** — same objective (`ScalarFunctionExpr` declares
its own
proto ser/de), different resolution of the `ScalarUDF` crate-graph problem.
Opening
this so the two shapes can be compared side by side.
## 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 + registry
on
decode, and reconstruction needs the session `ConfigOptions`.
The tension: the crate-wide `try_to_proto` / `try_from_proto` contexts live
in
`physical-expr-common`, which sits *below* `datafusion-expr` in the crate
graph
(`datafusion-expr` depends on it for `Arc<dyn PhysicalExpr>`), so those
contexts
cannot name `ScalarUDF` without a dependency cycle.
**#23421's answer:** absorb `ScalarFunctionExpr` into the crate-wide hook by
adding
`dyn Any`-erased function-codec channels to the contexts
(`encode_function<F: Any>` /
`decode_function<F: Any>`), with the erasure confined internally and the
concrete
type verified at the boundary. Headline win: **zero special cases in
`to_proto.rs`**.
**This PR's answer (option B):** keep the erasure out of the codebase
entirely. The
expression still declares its own ser/de, but against two **fully-typed**
bridge
traits defined next to it in `datafusion-physical-expr` (which *can* name
`ScalarUDF`):
```rust
pub trait ScalarUdfProtoEncoder {
fn encode_child(&self, expr: &Arc<dyn PhysicalExpr>) ->
Result<PhysicalExprNode>;
fn encode_udf(&self, udf: &ScalarUDF) -> Result<Option<Vec<u8>>>;
}
pub trait ScalarUdfProtoDecoder {
fn decode_child(&self, node: &PhysicalExprNode) -> Result<Arc<dyn
PhysicalExpr>>;
fn decode_udf(&self, name: &str, fun_definition: Option<&[u8]>) ->
Result<Arc<ScalarUDF>>;
fn config_options(&self) -> Arc<ConfigOptions>;
}
```
`datafusion-proto` implements them (it can name every type), and the
encode/decode
sites downcast to `ScalarFunctionExpr` and delegate to its inherent
`try_to_proto` / `try_from_proto`. **No `dyn Any` anywhere.**
### The trade
The two designs sit on opposite sides of one unavoidable fork (dispatching a
session-dependent expression from a bottom-crate trait *forces* erasure;
keeping the
type means keeping a concrete arm):
| | #23421 | This PR (B) |
|---|---|---|
| `to_proto.rs` special cases | 0 | 1 typed downcast arm |
| `dyn Any` in the codebase | internal channels + `TypeId` routing | none |
| ctx `config_options()` | `Result`, erroring default | typed, provided by
the bridge |
| future `AggregateUDF`/`WindowUDF` | ride the same erased channels | one
typed bridge trait each |
The motivating case for the whole mechanism (#21835,
`DynamicFilterPhysicalExpr`
reaching private state) is **session-free** and rides the crate-wide hook
happily
under either design; `ScalarFunctionExpr` is the only expression dragging
session
objects across the boundary. B treats it as the one justified exception
rather than
reshaping the shared contract to absorb it.
## What changes are included in this PR?
Commit 1 — `datafusion/physical-expr`:
- `ScalarUdfProtoEncoder` / `ScalarUdfProtoDecoder` bridge traits + module
docs
explaining the crate-graph constraint.
- Inherent `ScalarFunctionExpr::try_to_proto` / `try_from_proto`.
- 6 direct tests over mock bridges (shape, codec-payload passthrough, missing
`return_type`, child encode/decode error propagation, decode round-trip).
Commit 2 — `datafusion/proto`:
- `impl ScalarUdfProtoEncoder for ConverterEncoder`; the encode downcast arm
delegates.
- `ScalarUdfConverterDecoder` (same registry-then-codec lookup order); the
`ExprType::ScalarUdf` arm delegates.
## Are these changes tested?
- 6 new direct tests in `scalar_function.rs::proto_tests`.
- `cargo test -p datafusion-proto --test proto_integration`: 190 passed. The
scalar-UDF
round-trip can now only pass through the new bridge.
- `cargo fmt --all` and `cargo clippy -p datafusion-physical-expr -p
datafusion-proto
--all-features -- -D warnings` are clean.
## Are there any user-facing changes?
Wire format is byte-for-byte unchanged. New public API: the two bridge
traits and the
two inherent methods (all feature `proto`, additive/non-breaking).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_018LmymuRdTRmGthjMhtCijn
--
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]
