adriangb opened a new pull request, #23733:
URL: https://github.com/apache/datafusion/pull/23733
## Which issue does this PR close?
Closes #22920.
Alternative to #22922 — same goal, but plumbs the per-expr encode/decode
**context** (from the #22418 hook machinery) instead of the raw
`PhysicalProtoConverterExtension`.
## Rationale for this change
#21807 introduced the `DynamicFilterPhysicalExpr` dedup pipeline so
identical references on the wire reconstruct to one shared `Arc<Inner>` via
`expr_id` cache keys, and #22011 hooked it through the `SortExec` /
`AggregateExec` / `HashJoinExec` plan codecs.
The remaining gap is the **expression-level** extension path. In
`serialize_physical_expr_with_converter` /
`parse_physical_expr_with_converter`, the codec's `try_encode_expr` /
`try_decode_expr` is reached only as a fallback after the built-in
`try_to_proto` path and the `ScalarFunctionExpr` downcast — i.e. only for
downstream-defined **custom `PhysicalExpr`** types. When such a codec
serializes nested `PhysicalExprNode` fields *inside its own blob*, the only
helper available today is the free `serialize_physical_expr` /
`parse_physical_expr`, hardwired to `DefaultPhysicalProtoConverter`. So those
nested exprs get `expr_id: None` on the wire and reconstruct as **distinct**
`Inner` allocations — heap-max updates from a `SortExec` never reach the
wrapped reference.
## What changes are included in this PR?
The built-in expressions migrated under #22418 already receive a
`PhysicalExprEncodeCtx` / `PhysicalExprDecodeCtx` in their `try_to_proto` /
`try_from_proto` hooks. Those context objects bundle the dedup-aware converter
**plus** the active schema and task context, and hide
`PhysicalProtoConverterExtension` / `PhysicalExtensionCodec` from the
expression author entirely.
This PR hands the **same context** to the expr-level codec methods:
```rust
fn try_decode_expr(
&self,
buf: &[u8],
inputs: &[Arc<dyn PhysicalExpr>],
ctx: &PhysicalExprDecodeCtx<'_>, // new
) -> Result<Arc<dyn PhysicalExpr>>;
fn try_encode_expr(
&self,
node: &Arc<dyn PhysicalExpr>,
buf: &mut Vec<u8>,
ctx: &PhysicalExprEncodeCtx<'_>, // new
) -> Result<()>;
```
A codec that embeds nested `PhysicalExprNode`s now decodes them with
`ctx.decode(..)` and encodes them with `ctx.encode_child(..)`, which:
- route through any active `DeduplicatingProtoConverter` /
`DeduplicatingDeserializer`, so shared inner expressions cache-hit on
`expr_id`; **and**
- carry the real schema and task context, so nested UDF / column references
resolve against the actual registry.
### Why the context, not the raw converter (cf. #22922)
Threading the bare `PhysicalProtoConverterExtension` still leaves the codec
without the schema/registry, forcing it to fabricate a `SessionContext::new()`
and hard-code a schema to call `proto_to_physical_expr` — an empty-registry
footgun for any nested expr that references a UDF or column. Passing the
existing `Physical{Encode,Decode}Ctx` avoids that, keeps the extension
escape-hatch consistent with the per-expr proto hooks, and adds no third
converter parameter to the codec API (the concern raised on #22922).
## Are these changes tested?
Yes — `extension_codec_expr_participates_in_deduplication` builds a
`BinaryExpr` whose left operand is a bare `DynamicFilterPhysicalExpr` and whose
right operand is a custom `WrapperExpr` whose codec embeds the same dynamic
filter inside its serialized blob (via `ctx.encode_child`). After a
`DeduplicatingProtoConverter` roundtrip, an `update()` on the bare-side decoded
filter is observed via `current()` on the wrapped-side filter, proving both
refs back the same `Inner`. The codec needs no fabricated `SessionContext` — it
decodes the nested expr through `ctx.decode`.
## Are there any user-facing changes?
Yes — a **breaking change** for downstream codecs that override
`try_encode_expr` / `try_decode_expr`: they must add the new `ctx` parameter
(name it `_ctx` if the custom expr carries no nested `PhysicalExprNode`s).
Codecs that only override the plan-level `try_encode` / `try_decode` are
unaffected. Wire format is unchanged.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
