feat(proto): plumb PhysicalProtoConverterExtension into try_encode_expr / try_decode_expr - #22922
Open
zhuqi-lucas wants to merge 1 commit into
Open
feat(proto): plumb PhysicalProtoConverterExtension into try_encode_expr / try_decode_expr#22922zhuqi-lucas wants to merge 1 commit into
zhuqi-lucas wants to merge 1 commit into
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends DataFusion’s physical-expression protobuf conversion pipeline so extension codecs can participate in expression deduplication (notably for DynamicFilterPhysicalExpr) by threading the active PhysicalProtoConverterExtension through PhysicalExtensionCodec::{try_encode_expr, try_decode_expr}. This closes the gap where expressions serialized inside extension-codec blobs could not benefit from a deduplicating converter on decode, breaking referential integrity across extension boundaries.
Changes:
- Add a
proto_converter: &dyn PhysicalProtoConverterExtensionparameter toPhysicalExtensionCodec::try_encode_exprandtry_decode_expr. - Plumb the converter through the internal extension-codec expression fallback paths in
serialize_physical_expr_with_converterandparse_physical_expr_with_converter. - Add a regression test proving a nested extension expression can share the same decoded
DynamicFilterPhysicalExpr::Innervia deduplication.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| datafusion/proto/src/physical_plan/mod.rs | Extends PhysicalExtensionCodec expr-level API to accept proto_converter and documents intended usage. |
| datafusion/proto/src/physical_plan/to_proto.rs | Forwards the active converter into codec.try_encode_expr from the extension-expression fallback path. |
| datafusion/proto/src/physical_plan/from_proto.rs | Forwards the active converter into codec.try_decode_expr when decoding extension expressions. |
| datafusion/proto/tests/cases/roundtrip_physical_plan.rs | Adds a proto-roundtrip test validating deduplication works across an extension-expr boundary; updates an existing codec impl for the new signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…pr / try_decode_expr Closes apache#22920. apache#21807 introduced the `DynamicFilterPhysicalExpr` dedup pipeline so that identical references on the wire reconstruct to one shared `Arc<Inner>` on the decode side via `expression_id` cache keys. The pipeline works end-to-end for plans built from upstream types, and apache#22011 hooked it through Sort/Aggregate/HashJoin plan codecs. However, the pipeline stops at the `PhysicalExtensionCodec` boundary. Downstream users with custom file sources or plan extension types encode/decode their nested `PhysicalExprNode` fields via `try_encode_expr` / `try_decode_expr`, which did not receive the active `PhysicalProtoConverterExtension`. The only entry points available inside such a codec were the free `serialize_physical_expr` / `parse_physical_expr` helpers, hardwired to `DefaultPhysicalProtoConverter`. So any expression inside an extension codec's serialized blob got `expr_id = None` on the wire, and the deduplicating deserializer's cache never hit for refs that crossed an extension boundary -- even when an outer `DeduplicatingProtoConverter` was in effect on the rest of the plan. Two references to the same `DynamicFilterPhysicalExpr` (one inside an extension expr, one outside) reconstructed as distinct `Inner` allocations, so heap-max updates from a TopK SortExec failed to propagate to a FileScan predicate carried by a custom file source. This matches the existing plan-level pattern (`try_encode` and `try_decode` already take a `proto_converter` parameter, added when the plan codec was wired into the converter pipeline). This change extends the expr-level methods the same way: fn try_decode_expr( &self, buf: &[u8], inputs: &[Arc<dyn PhysicalExpr>], proto_converter: &dyn PhysicalProtoConverterExtension, // new ) -> Result<Arc<dyn PhysicalExpr>>; fn try_encode_expr( &self, node: &Arc<dyn PhysicalExpr>, buf: &mut Vec<u8>, proto_converter: &dyn PhysicalProtoConverterExtension, // new ) -> Result<()>; The two internal call sites in `serialize_physical_expr_with_converter` and `parse_physical_expr_with_converter` are updated to thread the active converter through. Downstream codecs can now route nested expression encode/decode through `proto_converter.physical_expr_to_proto` / `proto_converter.proto_to_physical_expr`, picking up dedup automatically. This is a breaking change for downstream codecs that override `try_encode_expr` / `try_decode_expr` -- they will need to add the new parameter (and may pass `_proto_converter` if they don't carry nested expressions). Codecs that only override `try_encode` / `try_decode` (plan-level) are unaffected. Wire format is unchanged (`PhysicalExprNode.expr_id` is unchanged); the fix is purely on the Rust trait API. Test: a new `extension_codec_expr_participates_in_deduplication` proto-roundtrip test constructs a `BinaryExpr` whose left operand is a bare `DynamicFilterPhysicalExpr` and right operand is a custom `WrapperExpr` whose extension codec embeds the same dynamic filter inside its serialized blob. After a `DeduplicatingProtoConverter` roundtrip, an `update()` applied to the bare-side decoded filter is observed by `current()` on the wrapped-side decoded filter, proving both refs back the same `Inner`. Without the converter-aware extension codec, this test fails because the two refs end up with distinct `Inner` allocations.
zhuqi-lucas
force-pushed
the
qizhu/extension-codec-with-converter
branch
from
June 12, 2026 06:29
3ca0edd to
7a58afe
Compare
Contributor
|
I want to take a deep look at this. It points out a real problem but I worry that there are like 4 different traits now going around... and I want to make sure we pass the right one / the API shapes are what we need. |
pull Bot
pushed a commit
to TCeason/arrow-datafusion
that referenced
this pull request
Jul 21, 2026
… try_decode_expr (apache#23733) ## Which issue does this PR close? Closes apache#22920. Alternative to apache#22922 — same goal, but plumbs the per-expr encode/decode **context** (from the apache#22418 hook machinery) instead of the raw `PhysicalProtoConverterExtension`. ## Rationale for this change apache#21807 introduced the `DynamicFilterPhysicalExpr` dedup pipeline so identical references on the wire reconstruct to one shared `Arc<Inner>` via `expr_id` cache keys, and apache#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 apache#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. apache#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 apache#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) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
… try_decode_expr (apache#23733) ## Which issue does this PR close? Closes apache#22920. Alternative to apache#22922 — same goal, but plumbs the per-expr encode/decode **context** (from the apache#22418 hook machinery) instead of the raw `PhysicalProtoConverterExtension`. ## Rationale for this change apache#21807 introduced the `DynamicFilterPhysicalExpr` dedup pipeline so identical references on the wire reconstruct to one shared `Arc<Inner>` via `expr_id` cache keys, and apache#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 apache#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. apache#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 apache#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) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #22920.
Rationale for this change
#21807 introduced the
DynamicFilterPhysicalExprdedup pipeline so that identical references on the wire reconstruct to one sharedArc<Inner>on the decode side viaexpression_idcache keys. The pipeline works end-to-end for plans built from upstream types, and #22011 hooked it throughSortExec/AggregateExec/HashJoinExecplan codecs.In
to_proto.rs::serialize_physical_expr_with_converter, the extension codec'stry_encode_expris reached only as a fallback after the built-in path (expr.try_to_proto(&ctx)?) and theScalarFunctionExprdowncast. Sotry_encode_expronly fires for downstream-defined customPhysicalExprtypes. The same applies symmetrically toparse_physical_expr_with_converterandtry_decode_expron the decode side.When such a custom-
PhysicalExprcodec needs to serialize nestedPhysicalExprNodefields inside its blob, the only helper available today is the freeserialize_physical_expr(hardwired toDefaultPhysicalProtoConverter). So nested expressions inside a custom-PhysicalExprblob getexpr_id: Noneon the wire, and theDeduplicatingDeserializer's cache never hits for refs that travel through a custom-expr boundary — even when an outerDeduplicatingProtoConverteris in effect on the rest of the plan.Concretely: two references to the same
DynamicFilterPhysicalExpr(one inside a custom-expr's nested field, one outside) reconstruct as distinctInnerallocations after roundtrip, so heap-max updates from aSortExecfail to propagate to the wrapped reference.What changes are included in this PR?
Matches the existing plan-level pattern.
try_encodeandtry_decodealready take aproto_converterparameter; this change extends the expr-level methods the same way:The two internal call sites in
serialize_physical_expr_with_converterandparse_physical_expr_with_converterare updated to thread the active converter through. Downstream codecs whose customPhysicalExprtypes embed nestedPhysicalExprNodefields can now route those throughproto_converter.physical_expr_to_proto/proto_converter.proto_to_physical_exprand pick up dedup automatically.Are these changes tested?
Yes — a new
extension_codec_expr_participates_in_deduplicationproto-roundtrip test constructs aBinaryExprwhose left operand is a bareDynamicFilterPhysicalExprand right operand is a customWrapperExprwhose extension codec embeds the same dynamic filter inside its serialized blob. After aDeduplicatingProtoConverterroundtrip, anupdate()applied to the bare-side decoded filter is observed bycurrent()on the wrapped-side decoded filter, proving both refs back the sameInner. Without the converter-aware extension codec the test fails because the two refs end up with distinctInnerallocations.Are there any user-facing changes?
Yes — this is a breaking change for downstream codecs that override
try_encode_expr/try_decode_expr(i.e. codecs that handle customPhysicalExprtypes). They need to add the newproto_converterparameter (and may name it_proto_converterif they don't carry nested expressions). Codecs that only override the plan-leveltry_encode/try_decodeare unaffected.Wire format is unchanged (
PhysicalExprNode.expr_idis unchanged); the fix is purely on the Rust trait API.