Is your feature request related to a problem or challenge?
Companion to #24625, which proposes a per-type decoder registry for extension ExecutionPlans. 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:
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:
optional string expr_name = 3; on PhysicalExtensionExprNode — additive and proto3-compatible.
- A session-scoped registry from that name to a decoder fn.
- 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:
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:
- Add
fn task_ctx(&self) -> &TaskContext; to the PhysicalExprDecode dispatch trait.
- Implement it on
ConverterDecoder as self.ctx.task_ctx().
- 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 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 PhysicalExprs 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.
Is your feature request related to a problem or challenge?
Companion to #24625, which proposes a per-type decoder registry for extension
ExecutionPlans. The expression side has the same gap, and a code comment in-tree already anticipates closing it.PhysicalExprDecodeCtx::decodedocuments the current state:But #21835 does not actually propose a registry. It's closed, it delivered
datafusion-proto-modelsand thetry_to_protohook, and it explicitly keeps third-party expressions ontry_encode_expr/try_decode_exprfor "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:
Describe the solution you'd like
The same three-part shape as the plan-side proposal:
optional string expr_name = 3;onPhysicalExtensionExprNode— additive and proto3-compatible.PhysicalExtensionCodec::try_decode_expr, unchanged.PhysicalExpr::try_to_protoalready exists with the sameOk(None)-falls-back-to-codec contract, so the encode half needs nothing beyond anencode_extension-style helper to build the wrapper.One extra piece of plumbing
PhysicalExprDecodeCtxexposesschema()anddecode()but notask_ctx(), unlikeExecutionPlanDecodeCtx. 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:
and
PhysicalPlanDecodeContextholdstask_ctx: &'a TaskContext(non-optional) with apub fn task_ctx()accessor already on it. So it's three lines:fn task_ctx(&self) -> &TaskContext;to thePhysicalExprDecodedispatch trait.ConverterDecoderasself.ctx.task_ctx().pub fn task_ctx(&self)onPhysicalExprDecodeCtx.No struct-layout change and no change to the
pub fn PhysicalExprDecodeCtx::new(schema, decoder)signature, because the ctx stores onlyschema+&dyn PhysicalExprDecodeand would delegate — which is exactly howExecutionPlanDecodeCtx::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
PhysicalExprDecodeis technically breaking for any downstream implementor. The plan-side dispatch traits reserved that right explicitly — they are#[doc(hidden)]and say so in prose:PhysicalExprDecodehas neither the attribute nor that note. Two cheap, non-exclusive fixes: give the new method anot_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 is a heavy user of extension-plan serialization — 833 lines of
PhysicalExtensionCodeccovering 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 customPhysicalExprs would benefit; that one wouldn't.Describe alternatives you've considered
Leaving extension expressions on
try_decode_exprindefinitely. 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, thePhysicalExprDecodeCtx::decodedoc 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.