adriangb opened a new issue, #24626:
URL: https://github.com/apache/datafusion/issues/24626
## Is your feature request related to a problem or challenge?
Companion to #24625, which proposes a per-type decoder registry for
extension `ExecutionPlan`s. The expression side has the same gap, and a code
comment in-tree already anticipates closing it.
`PhysicalExprDecodeCtx::decode` documents the current state:
> Routes built-in `ExprType` variants through `datafusion-proto`'s central
match and forwards extension nodes to the registered codec (today via
`PhysicalExtensionCodec::try_decode_expr`; later via a per-type registry — see
#21835).
But #21835 does not actually propose a registry. It's closed, it delivered
`datafusion-proto-models` and the `try_to_proto` hook, and it explicitly keeps
third-party expressions on `try_encode_expr` / `try_decode_expr` for "parity
between built-in and third-party". So that comment is a dangling aspiration
pointing at an issue that never covered it. This issue is intended to be the
thing it should point at.
The concrete blocker is identical to the plan side — no discriminator on the
wire:
```rust
pub struct PhysicalExtensionExprNode {
expr: Vec<u8>,
inputs: Vec<PhysicalExprNode>,
}
```
## Describe the solution you'd like
The same three-part shape as the plan-side proposal:
1. `optional string expr_name = 3;` on `PhysicalExtensionExprNode` —
additive and proto3-compatible.
2. A session-scoped registry from that name to a decoder fn.
3. Decode rule: name present and registered → registry; otherwise →
`PhysicalExtensionCodec::try_decode_expr`, unchanged.
`PhysicalExpr::try_to_proto` already exists with the same
`Ok(None)`-falls-back-to-codec contract, so the encode half needs nothing
beyond an `encode_extension`-style helper to build the wrapper.
### One extra piece of plumbing
`PhysicalExprDecodeCtx` exposes `schema()` and `decode()` but **no
`task_ctx()`**, unlike `ExecutionPlanDecodeCtx`. Without it, a registry-decoded
extension expression can't reach session state.
This turns out to be an unforwarded accessor rather than a structural gap.
The driver behind the ctx already holds the session:
```rust
struct ConverterDecoder<'a, 'b> {
ctx: &'a PhysicalPlanDecodeContext<'b>,
proto_converter: &'a dyn PhysicalProtoConverterExtension,
}
```
and `PhysicalPlanDecodeContext` holds `task_ctx: &'a TaskContext`
(non-optional) with a `pub fn task_ctx()` accessor already on it. So it's three
lines:
1. Add `fn task_ctx(&self) -> &TaskContext;` to the `PhysicalExprDecode`
dispatch trait.
2. Implement it on `ConverterDecoder` as `self.ctx.task_ctx()`.
3. Add a delegating `pub fn task_ctx(&self)` on `PhysicalExprDecodeCtx`.
No struct-layout change and no change to the `pub fn
PhysicalExprDecodeCtx::new(schema, decoder)` signature, because the ctx stores
only `schema` + `&dyn PhysicalExprDecode` and would delegate — which is exactly
how `ExecutionPlanDecodeCtx::task_ctx()` already works on the plan side. No
wire change. Purely additive for expression authors.
Worth doing on its own merits even if the registry is deferred.
### Small API-headroom note
Adding a required method to `PhysicalExprDecode` is technically breaking for
any downstream implementor. The plan-side dispatch traits reserved that right
explicitly — they are `#[doc(hidden)]` and say so in prose:
> `pub` only because the implementors live in another crate;
`#[doc(hidden)]` records that, so encoding primitives can be added here as the
serialization hooks grow without breaking downstream code.
`PhysicalExprDecode` has neither the attribute nor that note. Two cheap,
non-exclusive fixes: give the new method a `not_impl_err!` default, and/or mark
the trait `#[doc(hidden)]` with the same reservation the plan side carries. The
latter is a one-line change worth making regardless, since it buys headroom for
every future addition to these hooks.
## Worked example: datafusion-distributed
Included for scope calibration rather than as a motivating case.
[datafusion-distributed](https://github.com/datafusion-contrib/datafusion-distributed)
is a heavy user of extension-plan serialization — 833 lines of
`PhysicalExtensionCodec` covering six plan types — but it implements **no**
expression or UDF codec methods at all.
That's a useful data point in two directions: the plan-side registry alone
would take that project completely off `PhysicalExtensionCodec`, and the
expression-side registry should be scoped and prioritized independently rather
than bundled in as a blocker. Projects that do define custom `PhysicalExpr`s
would benefit; that one wouldn't.
## Describe alternatives you've considered
Leaving extension expressions on `try_decode_expr` indefinitely. That's a
reasonable outcome — the expression surface is smaller and the codec path
works. The cost is that the asymmetry becomes permanent: built-in and
third-party expressions keep using different mechanisms, which is the exact
split #21835 set out to close for built-ins. At minimum, the
`PhysicalExprDecodeCtx::decode` doc comment should be repointed at whatever the
real plan is, since it currently promises a registry that no open issue tracks.
## Additional context
As on the plan side, registered names should probably be namespaced so
collisions surface at registration.
--
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]
