proto: add proto converter reference to PhysicalExtensionCodec trait by jayshrivastava · Pull Request #21055 · apache/datafusion

jayshrivastava · 2026-03-19T15:47:09Z

Which issue does this PR close?

Closes add PhysicalProtoConverterExtension to PhysicalExtensionCodec #21056

Rationale for this change

Custom ExecutionPlan nodes that store PhysicalExpr fields cannot participate in expression deduplication during serialization because PhysicalExtensionCodec::try_encode/try_decode lack access to the PhysicalProtoConverterExtension.

What changes are included in this PR?

Added proto_converter: &dyn PhysicalProtoConverterExtension parameter
to PhysicalExtensionCodec::try_decode and try_encode
Updated all implementations: DefaultPhysicalExtensionCodec,
ComposedPhysicalExtensionCodec, ForeignPhysicalExtensionCodec (FFI),
and all examples
Updated call sites in PhysicalPlanNode serialization/deserialization
FFI bridge passes DefaultPhysicalProtoConverter as a fallback

Are these changes tested?

Yes — added test_custom_node_with_dynamic_filter_dedup_roundtrip which
verifies that a DynamicFilterPhysicalExpr shared between a FilterExec
and a custom ExecutionPlan node preserves its shared inner state after a
roundtrip through DeduplicatingProtoConverter.

Are there any user-facing changes?

Breaking API change: PhysicalExtensionCodec::try_decode and try_encode
now take an additional &dyn PhysicalProtoConverterExtension parameter.
All custom codec implementations must be updated.

…mic filter deduping to work in custom plan nodes

github-actions · 2026-05-06T18:04:21Z

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

     Cloning apache/main
    Building datafusion-ffi v53.1.0 (current)
       Built [  57.638s] (current)
     Parsing datafusion-ffi v53.1.0 (current)
      Parsed [   0.056s] (current)
    Building datafusion-ffi v53.1.0 (baseline)
       Built [  56.177s] (baseline)
     Parsing datafusion-ffi v53.1.0 (baseline)
      Parsed [   0.056s] (baseline)
    Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.223s] 222 checks: 222 pass, 30 skip
     Summary no semver update required
    Finished [ 116.201s] datafusion-ffi
    Building datafusion-proto v53.1.0 (current)
       Built [  53.301s] (current)
     Parsing datafusion-proto v53.1.0 (current)
      Parsed [   0.133s] (current)
    Building datafusion-proto v53.1.0 (baseline)
       Built [  52.749s] (baseline)
     Parsing datafusion-proto v53.1.0 (baseline)
      Parsed [   0.133s] (baseline)
    Checking datafusion-proto v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   1.572s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip

--- failure trait_method_parameter_count_changed: pub trait method parameter count changed ---

Description:
A trait method now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-item-signature
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/trait_method_parameter_count_changed.ron

Failed in:
  PhysicalExtensionCodec::try_decode now takes 4 instead of 3 parameters, in file /home/runner/work/datafusion/datafusion/datafusion/proto/src/physical_plan/mod.rs:3705
  PhysicalExtensionCodec::try_encode now takes 3 instead of 2 parameters, in file /home/runner/work/datafusion/datafusion/datafusion/proto/src/physical_plan/mod.rs:3713

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [ 110.948s] datafusion-proto

adriangb

Currently this is a breaking change. Would you be okay holding it until we release DF 54? Either way if we keep a breaking change (or add new recommended methods) we should add a note the upgrade guide.

adriangb · 2026-05-14T19:19:10Z

        buf: &[u8],
        inputs: &[Arc<dyn ExecutionPlan>],
        _ctx: &TaskContext,
+        _proto_converter: &dyn PhysicalProtoConverterExtension,


We could add new methods with the param that default to the old methods to avoid a breaking change. Did you consider that as an option?

adriangb · 2026-05-14T19:20:03Z

        &self,
        _node: Arc<dyn ExecutionPlan>,
        _buf: &mut Vec<u8>,
+        _proto_converter: &dyn PhysicalProtoConverterExtension,


Naming these converter would match try_from_physical_plan_with_converter and make the arg name shorter.

adriangb · 2026-05-14T19:20:54Z

+        buf.as_ref(),
+        &inputs,
+        task_ctx.as_ref(),
+        &DefaultPhysicalProtoConverter {},


@timsaucer not sure if you have any suggestions but I think this is fine. Generally we have not made any effort to make dynamic filters / expr deduplication work with plans that go through FFI.

jayshrivastava · 2026-05-19T15:17:50Z

@adriangb TYFR!

If possible, we would prefer v54 to unblock datafusion-distributed. If we merge, the breaking change would go out in v54.0.0 correct?

The change is pretty opt-in, the new parameter in the trait can be safely ignored.

adriangb · 2026-05-19T16:09:56Z

If possible, we would prefer v54 to unblock datafusion-distributed. If we merge, the breaking change would go out in v54.0.0 correct?

Yes and my concern with merging this right before the release is that no one will get a chance to test it and if there are issues we are locked into a 6 month deprecation process.

adriangb · 2026-05-21T13:32:16Z

54 branch has been cut, lets merge this!

…pache#21055) ## Which issue does this PR close?  - Closes apache#21056 ## Rationale for this change Custom `ExecutionPlan` nodes that store `PhysicalExpr` fields cannot participate in expression deduplication during serialization because `PhysicalExtensionCodec::try_encode`/`try_decode` lack access to the `PhysicalProtoConverterExtension`. ## What changes are included in this PR? - Added `proto_converter: &dyn PhysicalProtoConverterExtension` parameter to `PhysicalExtensionCodec::try_decode` and `try_encode` - Updated all implementations: `DefaultPhysicalExtensionCodec`, `ComposedPhysicalExtensionCodec`, `ForeignPhysicalExtensionCodec` (FFI), and all examples - Updated call sites in `PhysicalPlanNode` serialization/deserialization - FFI bridge passes `DefaultPhysicalProtoConverter` as a fallback ## Are these changes tested? Yes — added `test_custom_node_with_dynamic_filter_dedup_roundtrip` which verifies that a `DynamicFilterPhysicalExpr` shared between a `FilterExec` and a custom `ExecutionPlan` node preserves its shared inner state after a roundtrip through `DeduplicatingProtoConverter`. ## Are there any user-facing changes? Breaking API change: `PhysicalExtensionCodec::try_decode` and `try_encode` now take an additional `&dyn PhysicalProtoConverterExtension` parameter. All custom codec implementations must be updated.

github-actions Bot added proto Related to proto crate ffi Changes to the ffi crate labels Mar 19, 2026

jayshrivastava force-pushed the js/add-proto-converter-to-physical-extension-codec-trait branch from 9383cdf to 8a86ed0 Compare April 22, 2026 18:19

jayshrivastava mentioned this pull request Apr 23, 2026

[DISCUSSION] Future of Dynamic Filters Sync #21207

Open

jayshrivastava added 2 commits May 6, 2026 17:34

add proto converter reference to PhysicalExtensionCodec to allow dyna…

dc8addd

…mic filter deduping to work in custom plan nodes

wip

6dc8faf

jayshrivastava force-pushed the js/add-proto-converter-to-physical-extension-codec-trait branch from c5b5d44 to 6dc8faf Compare May 6, 2026 17:59

github-actions Bot added the auto detected api change Auto detected API change label May 6, 2026

jayshrivastava changed the title ~~add proto converter reference to PhysicalExtensionCodec~~ proto: add proto converter reference to PhysicalExtensionCodec trait May 6, 2026

jayshrivastava added 2 commits May 6, 2026 19:17

wip;

e0baa89

more linting

c30ee94

jayshrivastava marked this pull request as ready for review May 14, 2026 14:35

typo

c53347d

adriangb approved these changes May 14, 2026

View reviewed changes

adriangb added this pull request to the merge queue May 21, 2026

Merged via the queue into apache:main with commit 50d74a7 May 21, 2026
35 checks passed

nuno-faria added the v55.0.0 label Oct 3, 2026

Navigation Menu

Repository navigation

proto: add proto converter reference to PhysicalExtensionCodec trait - #21055

proto: add proto converter reference to PhysicalExtensionCodec trait#21055
adriangb merged 5 commits into
apache:mainfrom
jayshrivastava:js/add-proto-converter-to-physical-extension-codec-trait

jayshrivastava commented Mar 19, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026 •

edited

Loading

Uh oh!

adriangb left a comment

Uh oh!

adriangb May 14, 2026

Uh oh!

adriangb May 14, 2026

Uh oh!

adriangb May 14, 2026

Uh oh!

jayshrivastava commented May 19, 2026 •

edited

Loading

Uh oh!

adriangb commented May 19, 2026

Uh oh!

adriangb commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jayshrivastava commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb left a comment

Choose a reason for hiding this comment

Uh oh!

adriangb May 14, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb May 14, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb May 14, 2026

Choose a reason for hiding this comment

Uh oh!

jayshrivastava commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented May 19, 2026

Uh oh!

adriangb commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jayshrivastava commented Mar 19, 2026 •

edited

Loading

github-actions Bot commented May 6, 2026 •

edited

Loading

jayshrivastava commented May 19, 2026 •

edited

Loading