Skip to content

Allow extension PhysicalExprs to decode via a per-type registry instead of PhysicalExtensionCodec #24626

Description

@adriangb

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:

  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:

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 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions