chore(proto): remove never-released deprecated PhysicalPlanNodeExt scaffolding - #24269
Merged
alamb merged 1 commit intoAug 11, 2026
Merged
Conversation
26 tasks
…affolding Deletes the 59 `#[deprecated(since = "55.0.0")]` methods from the `PhysicalPlanNodeExt` trait. These were promoted from private inherent methods to public trait methods by apache#21929 (merged after the 54.x line was cut), so no released version of DataFusion ever exposed them. 55.0.0 would have been the first release to ship them, already deprecated. Also drops the imports that became unused (`DataSinkExec`, `BoundedWindowAggExec`, `SortMergeJoinExecNode`) and the test that only exercised the `try_into_projection_physical_plan` shim; `ProjectionExec` encode/decode stays covered by the existing `roundtrip_test` cases. No wire-format or dispatch changes. Part of apache#23494. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adriangb
force-pushed
the
claude/physicalplannodeext-deprecation-a0351c
branch
from
August 11, 2026 13:52
d4c243f to
312a347
Compare
|
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24269 +/- ##
==========================================
+ Coverage 81.03% 81.24% +0.20%
==========================================
Files 1107 1107
Lines 384477 383469 -1008
Branches 384477 383469 -1008
==========================================
- Hits 311567 311539 -28
+ Misses 54565 53572 -993
- Partials 18345 18358 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
50 tasks
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
…affolding (apache#24269) ## Which issue does this PR close? - Part of apache#23494. This closes out that EPIC's "Remove the `#[deprecated(since = "55.0.0")]` `PhysicalPlanNodeExt` scaffolding" cleanup item. ## Rationale for this change `PhysicalPlanNodeExt` currently carries 59 `#[deprecated(since = "55.0.0")]` methods — per-operator `try_into_*_physical_plan` / `try_from_*_exec` shims left behind by the `ExecutionPlan::try_to_proto` / `try_from_proto` migration. Each is a thin forwarder to the operator's own hook; none is called by DataFusion. **These methods have never shipped in any DataFusion release**, so no deprecation window is owed: - At the `54.1.0` tag they were **private inherent** methods on `impl protobuf::PhysicalPlanNode` — not `pub`, not on a public trait: ```console $ git show 54.1.0:datafusion/proto/src/physical_plan/mod.rs | sed -n '700,701p' impl protobuf::PhysicalPlanNode { fn try_into_explain_physical_plan( ``` - apache#21929 (merge commit `077f08a9a6632324c95275dd15b5dd5b1f14006f`, merged 2026-05-22) is what promoted them into the public `PhysicalPlanNodeExt` trait, and it is **not** an ancestor of `54.1.0` (54.1.0 was cut off the 54.0.x line): ```console $ git merge-base --is-ancestor 077f08a 54.1.0; echo $? 1 ``` - Every one of them is marked `#[deprecated(since = "55.0.0")]`, so 55.0.0 would be the very first release to expose them — already deprecated. The [API health policy](https://github.com/apache/datafusion/blob/main/docs/source/contributor-guide/api-health.md) exists to protect API that users could have depended on *from a release*. Nothing released ever exposed these. Deleting them before the 55.0.0 branch is cut avoids shipping 59 dead-on-arrival public methods that we would then be obliged to carry for a full deprecation cycle. Removing them also deletes ~1350 lines from `datafusion/proto/src/physical_plan/mod.rs`, which makes the remaining, load-bearing surface of the trait much easier to read. ## What changes are included in this PR? - Delete the 59 `#[deprecated(since = "55.0.0")]` methods from `PhysicalPlanNodeExt` (29 `try_from_*_exec` encoders, 30 `try_into_*` decoders). - Drop three `use` statements that became unused as a result: `DataSinkExec`, `BoundedWindowAggExec`, `SortMergeJoinExecNode`. - Delete `deprecated_projection_shim_decodes_argument_not_self`, the one test that existed solely to pin the behaviour of the `try_into_projection_physical_plan` shim. Deliberately **not** changed: - The trait itself and its 16 non-deprecated methods stay: `node()`, `try_into_physical_plan_with_converter`, `try_into_physical_plan_with_context`, `try_from_physical_plan_with_converter`, the scan/extension/generate-series decoders and `try_from_data_source_exec` / `try_from_lazy_memory_exec` that the central dispatch still calls. - `AsExecutionPlan`, `PhysicalExtensionCodec`, `PhysicalProtoConverterExtension` are untouched. - **No `.proto` files and no encode/decode dispatch behaviour are touched — the wire format is unchanged.** - The `TryFromProto<&protobuf::{Json,Csv,Parquet}Sink>` impls in `from_proto.rs` mentioned in apache#23494 are **not** removed here: they are not marked `#[deprecated]` (there are no `deprecated` attributes anywhere in `from_proto.rs` / `to_proto.rs`), so they don't fall under the "never-released deprecated scaffolding" argument above and deserve their own decision. ## Are these changes tested? Covered by the existing test suite; the change is a pure deletion of unreferenced code. The one deleted test only exercised the deprecated shim. The underlying `ProjectionExec::try_to_proto` / `ProjectionExec::try_from_proto` hook keeps full roundtrip coverage through the existing `roundtrip_test` cases in `datafusion/proto/tests/cases/roundtrip_physical_plan.rs` (e.g. `roundtrip_like`, `roundtrip_projection_source`, `roundtrip_empty_projection`). Verified locally: - `cargo fmt --all` - `./ci/scripts/rust_clippy.sh` (CI's exact workspace + `--all-targets` clippy, clean) - `RUST_BACKTRACE=1 cargo test --profile ci -p datafusion-proto --features avro,json` — 238 tests pass, 0 failures - `cargo check -p datafusion-examples --examples` (two examples import `PhysicalPlanNodeExt`; both only use methods that stay) - `cargo doc -p datafusion-proto --no-deps` (no dangling intra-doc links) ## Are there any user-facing changes? Removal of 59 public trait methods, all of which were already `#[deprecated]` and none of which ever appeared in a published release. No Upgrade Guide entry is needed, because there is no released version anyone could be upgrading *from* that had this API. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
3 participants
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?
#[deprecated(since = "55.0.0")]PhysicalPlanNodeExtscaffolding" cleanup item.Rationale for this change
PhysicalPlanNodeExtcurrently carries 59#[deprecated(since = "55.0.0")]methods — per-operatortry_into_*_physical_plan/try_from_*_execshims left behind by theExecutionPlan::try_to_proto/try_from_protomigration. Each is a thin forwarder to the operator's own hook; none is called by DataFusion.These methods have never shipped in any DataFusion release, so no deprecation window is owed:
At the
54.1.0tag they were private inherent methods onimpl protobuf::PhysicalPlanNode— notpub, not on a public trait:Split proto serialization to encapsulate private state (#21835) #21929 (merge commit
077f08a9a6632324c95275dd15b5dd5b1f14006f, merged 2026-05-22) is what promoted them into the publicPhysicalPlanNodeExttrait, and it is not an ancestor of54.1.0(54.1.0 was cut off the 54.0.x line):Every one of them is marked
#[deprecated(since = "55.0.0")], so 55.0.0 would be the very first release to expose them — already deprecated.The API health policy exists to protect API that users could have depended on from a release. Nothing released ever exposed these. Deleting them before the 55.0.0 branch is cut avoids shipping 59 dead-on-arrival public methods that we would then be obliged to carry for a full deprecation cycle.
Removing them also deletes ~1350 lines from
datafusion/proto/src/physical_plan/mod.rs, which makes the remaining, load-bearing surface of the trait much easier to read.What changes are included in this PR?
#[deprecated(since = "55.0.0")]methods fromPhysicalPlanNodeExt(29try_from_*_execencoders, 30try_into_*decoders).usestatements that became unused as a result:DataSinkExec,BoundedWindowAggExec,SortMergeJoinExecNode.deprecated_projection_shim_decodes_argument_not_self, the one test that existed solely to pin the behaviour of thetry_into_projection_physical_planshim.Deliberately not changed:
node(),try_into_physical_plan_with_converter,try_into_physical_plan_with_context,try_from_physical_plan_with_converter, the scan/extension/generate-series decoders andtry_from_data_source_exec/try_from_lazy_memory_execthat the central dispatch still calls.AsExecutionPlan,PhysicalExtensionCodec,PhysicalProtoConverterExtensionare untouched..protofiles and no encode/decode dispatch behaviour are touched — the wire format is unchanged.TryFromProto<&protobuf::{Json,Csv,Parquet}Sink>impls infrom_proto.rsmentioned in [EPIC] Port ExecutionPlan serialization to try_to_proto / try_from_proto hooks #23494 are not removed here: they are not marked#[deprecated](there are nodeprecatedattributes anywhere infrom_proto.rs/to_proto.rs), so they don't fall under the "never-released deprecated scaffolding" argument above and deserve their own decision.Are these changes tested?
Covered by the existing test suite; the change is a pure deletion of unreferenced code.
The one deleted test only exercised the deprecated shim. The underlying
ProjectionExec::try_to_proto/ProjectionExec::try_from_protohook keeps full roundtrip coverage through the existingroundtrip_testcases indatafusion/proto/tests/cases/roundtrip_physical_plan.rs(e.g.roundtrip_like,roundtrip_projection_source,roundtrip_empty_projection).Verified locally:
cargo fmt --all./ci/scripts/rust_clippy.sh(CI's exact workspace +--all-targetsclippy, clean)RUST_BACKTRACE=1 cargo test --profile ci -p datafusion-proto --features avro,json— 238 tests pass, 0 failurescargo check -p datafusion-examples --examples(two examples importPhysicalPlanNodeExt; both only use methods that stay)cargo doc -p datafusion-proto --no-deps(no dangling intra-doc links)Are there any user-facing changes?
Removal of 59 public trait methods, all of which were already
#[deprecated]and none of which ever appeared in a published release. No Upgrade Guide entry is needed, because there is no released version anyone could be upgrading from that had this API.