Skip to content

refactor: register proto decoders with T::register instead of a free function - #26565

Draft
adriangb wants to merge 1 commit into
apache:mainfrom
pydantic:register-method
Draft

refactor: register proto decoders with T::register instead of a free function#26565
adriangb wants to merge 1 commit into
apache:mainfrom
pydantic:register-method

Conversation

@adriangb

@adriangb adriangb commented Oct 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

To register an extension type, a library must import a free function for each kind, in addition to the trait it already implements:

// Before
use datafusion::physical_plan::proto::{
    ExtensionExprFromProto, ExtensionPlanFromProto, ProtoDecoderRegistry,
    register_execution_plan, register_physical_expr,
};
config.update_extension(|registry: &mut ProtoDecoderRegistry| {
    register_execution_plan::<MyExec>(registry)?;
    register_physical_expr::<MyExpr>(registry)
})?;
// After: the traits are already imported to implement them,
// and `ProtoDecoderRegistry` is inferred
config.update_extension(MyExec::register)?;
config.update_extension(MyExpr::register)?;

What changes are included in this PR?

  • ExtensionPlanFromProto and ExtensionExprFromProto get a provided method, fn register(registry: &mut ProtoDecoderRegistry) -> Result<()>. It has the same behavior as the free functions: registering one type twice is a no-op, and a name collision or an empty name is an error.
  • register_execution_plan and register_physical_expr are removed. They were added for 56.0.0, which is not released, so there is nothing to deprecate.
  • All callers, the docs, the extension_plan_registry example and the 56.0.0 upgrade guide use the new method. They register with config.update_extension(T::register) from feat: add SessionConfig::update_extension so libraries can share one ProtoDecoderRegistry #26353, for plans and for expressions. Before this PR, the expression docs still said to attach the registry with with_extension.

The method cannot be an inherent method on ProtoDecoderRegistry: that type is in datafusion-proto-models, which cannot name ExecutionPlan or PhysicalExpr.

A built-in still cannot be registered. It does not implement the trait, so FilterExec::register and Column::register do not exist. The compile_fail doctests that check this moved to the new methods.

What is the testing strategy for this PR?

No behavior changes. The existing tests now call the method:

  • proto::registry unit tests in datafusion-physical-plan (6 pass).
  • expr_registry and extensions in datafusion-proto (20 pass).
  • The compile_fail doctests on ExtensionPlanFromProto::register and ExtensionExprFromProto::register, and the extension_plan_registry example.

Are there any user-facing changes?

Yes, to API that is not released yet: register_execution_plan::<T>(&mut registry) is now T::register(&mut registry), and the same for register_physical_expr. The 56.0.0 upgrade guide is updated.

🤖 Generated with Claude Code

@github-actions github-actions Bot added documentation Improvements or additions to documentation physical-expr Changes to the physical-expr crates proto Related to proto crate physical-plan Changes to the physical-plan crate labels Oct 21, 2026
@adriangb @claude
refactor: register proto decoders with a trait method instead of a fr…
0c16f8a 
…ee function

`register_execution_plan::<T>(&mut registry)` and
`register_physical_expr::<T>(&mut registry)` become provided methods on the
traits the extension already implements: `T::register(&mut registry)`. The
crate that defines `T` already imports the trait to implement it, so it needs
no other import, and the two kinds have the same call.

With `SessionConfig::update_extension`, registration needs no import of
`ProtoDecoderRegistry` either: `config.update_extension(MyExec::register)?`.
The docs, the 56.0.0 upgrade guide and the `extension_plan_registry` example
use this form, for plans and for expressions.

The method cannot be inherent on `ProtoDecoderRegistry`: that type is in
`datafusion-proto-models`, which cannot name `ExecutionPlan` or
`PhysicalExpr`.

Both free functions were added for 56.0.0, which is not released, so they are
removed and not deprecated. A built-in still cannot be registered: it does not
implement the trait, so `FilterExec::register` and `Column::register` do not
exist. The `compile_fail` doctests move to the new methods.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 21, 2026 •
edited
Loading

Copy link
Copy Markdown

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-physical-expr v55.1.0 (current)
       Built [  37.025s] (current)
     Parsing datafusion-physical-expr v55.1.0 (current)
      Parsed [   0.057s] (current)
    Building datafusion-physical-expr v55.1.0 (baseline)
       Built [  37.713s] (baseline)
     Parsing datafusion-physical-expr v55.1.0 (baseline)
      Parsed [   0.059s] (baseline)
    Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.369s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/function_missing.ron

Failed in:
  function datafusion_physical_expr::proto::register_physical_expr, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c168d6b21b25c4215b234b3c32cf939c3ed2dd93/datafusion/physical-expr/src/proto.rs:185

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  76.164s] datafusion-physical-expr
    Building datafusion-physical-plan v55.1.0 (current)
       Built [  44.498s] (current)
     Parsing datafusion-physical-plan v55.1.0 (current)
      Parsed [   0.233s] (current)
    Building datafusion-physical-plan v55.1.0 (baseline)
       Built [  48.404s] (baseline)
     Parsing datafusion-physical-plan v55.1.0 (baseline)
      Parsed [   0.241s] (baseline)
    Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.762s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/function_missing.ron

Failed in:
  function datafusion_physical_plan::proto::registry::register_execution_plan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c168d6b21b25c4215b234b3c32cf939c3ed2dd93/datafusion/physical-plan/src/proto/registry.rs:150
  function datafusion_physical_plan::proto::register_execution_plan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c168d6b21b25c4215b234b3c32cf939c3ed2dd93/datafusion/physical-plan/src/proto/registry.rs:150

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  95.321s] datafusion-physical-plan
    Building datafusion-proto v55.1.0 (current)
       Built [  65.468s] (current)
     Parsing datafusion-proto v55.1.0 (current)
      Parsed [   0.013s] (current)
    Building datafusion-proto v55.1.0 (baseline)
       Built [  61.013s] (baseline)
     Parsing datafusion-proto v55.1.0 (baseline)
      Parsed [   0.014s] (baseline)
    Checking datafusion-proto v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.127s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [ 127.874s] datafusion-proto
    Building datafusion-proto-models v55.1.0 (current)
       Built [  30.539s] (current)
     Parsing datafusion-proto-models v55.1.0 (current)
      Parsed [   0.171s] (current)
    Building datafusion-proto-models v55.1.0 (baseline)
       Built [  30.582s] (baseline)
     Parsing datafusion-proto-models v55.1.0 (baseline)
      Parsed [   0.170s] (baseline)
    Checking datafusion-proto-models v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   2.010s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  64.516s] datafusion-proto-models

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Oct 21, 2026
@adriangb

adriangb commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

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

It is compatible with the last release, the parts we are breaking have not been released yet.

@adriangb

adriangb commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@gabotechs curious what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

auto detected api change Auto detected API change documentation Improvements or additions to documentation physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate proto Related to proto crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@adriangb