refactor(proto): delegate deprecated ProjectionExec serde shims to new hooks#23731
Open
adriangb wants to merge 2 commits into
Open
refactor(proto): delegate deprecated ProjectionExec serde shims to new hooks#23731adriangb wants to merge 2 commits into
adriangb wants to merge 2 commits into
Conversation
…w hooks The deprecated `try_from_projection_exec` / `try_into_projection_physical_plan` compatibility methods (kept from apache#23495) duplicated the wire-format logic that now lives in `ProjectionExec::try_to_proto` / `try_from_proto`. Replace their bodies with thin shims that build an `ExecutionPlanEncodeCtx` / `DecodeCtx` from the codec + converter and delegate to the hook, so the format is single-sourced. No wire-format or behavior change. Matches the delegate style used by FilterExec in apache#23708. Part of apache#23494. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9qX6kekbJGpmTSxvrU8e5
24 tasks
Contributor
Author
|
@kumarUjjawal can you take a look please? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23731 +/- ##
==========================================
+ Coverage 80.70% 80.72% +0.01%
==========================================
Files 1089 1089
Lines 368137 368253 +116
Branches 368137 368253 +116
==========================================
+ Hits 297121 297255 +134
+ Misses 53311 53281 -30
- Partials 17705 17717 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| proto_converter, | ||
| }; | ||
| let decode_ctx = ExecutionPlanDecodeCtx::new(&decoder); | ||
| ProjectionExec::try_from_proto(self.node(), &decode_ctx) |
Contributor
There was a problem hiding this comment.
We used to decode the supplied projection and now we ignore it and decodes self.node(). Wouldn't a caller get the wrong plan or a wrong-variant error? Was this intentional?
`try_from_proto` reads the enclosing `PhysicalPlanNode`, so delegating with `self.node()` changed the deprecated shim's contract: an out-of-tree caller passing a `ProjectionExecNode` unrelated to `self` would get a wrong-variant error instead of the decoded argument. Re-wrap the argument in a `PhysicalPlanNode` before delegating, and add a regression test that calls the shim with a `self` of a different plan variant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FiNDkYXSeuTbCxUPDnu6dB
kumarUjjawal
approved these changes
Jul 21, 2026
kumarUjjawal
left a comment
kumarUjjawal
left a comment
Contributor
There was a problem hiding this comment.
Thanks @adriangb Looks good
adriangb
enabled auto-merge
July 21, 2026 06:30
Contributor
Author
|
Thank you for the reviews! |
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
4 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?
Rationale for this change
#23495 landed the deprecated
ProjectionExeccompatibility shims(
try_from_projection_exec/try_into_projection_physical_plan) with theiroriginal bodies fully duplicated in
datafusion-proto, even though the samewire-format logic now lives in
ProjectionExec::try_to_proto/try_from_proto(the #22419 hook pattern). This PR makes those deprecated shims delegate to
the new hooks so the wire format is single-sourced in
datafusion-physical-planrather than duplicated. This matches the
FilterExecdelegate style in #23708.No wire-format change.
What changes are included in this PR?
try_from_projection_execwith a thin shim that builds anExecutionPlanEncodeCtxfrom thecodec+proto_converter(viaConverterPlanEncoder) and delegates toProjectionExec::try_to_proto.try_into_projection_physical_planwith a thin shim thatbuilds an
ExecutionPlanDecodeCtx(viaConverterPlanDecoder) and delegatesto
ProjectionExec::try_from_proto; the now-unusedprojectionparam isrenamed to
_projection.ProjectionExprimport.The
#[deprecated(...)]attributes and signatures are otherwise unchanged, andthe live dispatch arms and the
ProjectionExechook impls are untouched.Are these changes tested?
Yes — the existing
roundtrip_physical_planprojection/filter roundtrip testsexercise this path and pass (
roundtrip_projection_source,roundtrip_filter_with_fetch,roundtrip_filter_with_not_and_in_list, and thefull 203-test
roundtripsuite).cargo clippy -p datafusion-proto --all-features -- -D warningsis clean.Are there any user-facing changes?
No. The methods remain
#[deprecated]with identical signatures and wireformat; only their internals change.
🤖 Generated with Claude Code
https://claude.ai/code/session_01U9qX6kekbJGpmTSxvrU8e5