Uh oh!
There was an error while loading. Please reload this page.
[branch-55] Backport of refactor(physical-plan): Simplify ExecutionPlan API with replace_children - #24296
Merged
Conversation
…hildren` (apache#23903) ## Which issue does this PR close? - Closesapache#23441 ## User-facing changes: Deprecating `with_new_children` and `with_new_children_and_same_properties` in favor of `replace_children` As noted [here](apache#23332 (comment)), while the addition of `with_new_children_and_same_properties` has the benefit of skipping potentially expensive computation in the case that replacement children have the same properties as the original children, it widens the API surface area of `ExecutionPlan` in a way that could be confusing for users. Thus, to rectify this, we unify these methods by introducing `replace_children`, and we shift towards `with_new_children_if_necessary` as the universal entry point for replacing the children of an `ExecutionPlan`. `replace_children` simplifies the interface for users by taking an enum called `ChildrenPropertiesHint` as an argument. The enum has two variants, `SameProperties` and `Recompute`, which function as a hint to `replace_children` from the caller as to whether or not the properties need to be recomputed. ## Trait implementation migration To migrate from `with_new_children` and `with_new_children_and_same_properties` to `replace_children`, I went through all 93 implementations of `with_new_children` and implemented `replace_children` with a `match` statement matching on the `ChildrenPropertiesHint`. In the case that the properties match, `ChildrenPropertiesHint::SameProperties`, and we have an implementation of `with_new_children_and_same_properties`, then we follow the body of `with_new_children_and_same_properties`. In the case that the properties do not match, `ChildrenPropertiesHint::Recompute`, we follow the body of `with_new_children`. In the cases in which there was no implementation of `with_new_children_and_same_properties`, I simply move the body of `with_new_children` into `replace_children` and ignore the hint. I mark `with_new_children` and `with_new_children_and_same_properties` as deprecated with a migration note pointing to `replace_children`. After a couple releases, we'll drop the deprecated methods. ## Example For example, here is what the implementation looks like for `FilterExec` after this change: ``` fn replace_children( self: Arc<Self>, mut children: Vec<Arc<dyn ExecutionPlan>>, hint: ChildrenPropertiesHint, ) -> Result<Arc<dyn ExecutionPlan>> { validate_child_count!(self, children); match hint { ChildrenPropertiesHint::SameProperties => Ok(Arc::new(Self { input: children.swap_remove(0), metrics: ExecutionPlanMetricsSet::new(), ..Self::clone(&*self) })), ChildrenPropertiesHint::Recompute => { let new_input = children.swap_remove(0); FilterExecBuilder::from(&*self) .with_input(new_input) .build() .map(|e| Arc::new(e) as _) } } } ``` We see here that in the case that the hint suggests the properties are the same, we can simply swap the children without having to recompute the properties. In the case that the properties are not the same, we create a new node from scratch. We achieve this functionality by moving the hint calculations definitively into `with_new_children_if_necessary` rather than having them scattered around many methods. However, for this to all work we must ensure that users actually do use `with_new_children_if_necessary` by making it obvious to them somehow. I feel `replace_children` is a step in the right direction, but it could still be easy for a user to miss `with_new_children_if_necessary` and just jump to using `replace_children` instead. ## Usage Migration `replace_children` is called from `with_new_children_if_necessary`, which is the standard entry point that should be used for replacing the children of a node. To model the intended behavior for our users, I took the time here to migrate usages of `with_new_children` and `with_children_and_same_properties` to `with_new_children_if_necessary` where it made sense to do so, and I migrated `with_new_children_if_necessary` to use `replace_children` with the correct hint filled in at each branch. ## Testing - `cargo fmt --all` - `cargo check -p datafusion-physical-plan` - `cargo check -p datafusion-physical-optimizer` - `cargo check -p datafusion --lib` - `cargo check -p datafusion-ffi` - CI passing --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
JSOD11
marked this pull request as ready for review
August 12, 2026 19:50
50 tasks
ExecutionPlan API with replace_childrenExecutionPlan API with replace_childrencodecov-commenter
commented
Aug 12, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## branch-55 #24296 +/- ##
=============================================
- Coverage 81.29% 81.14% -0.16%
=============================================
Files 1110 1110 Lines 385336 386132 +796 Branches 385336 386132 +796 =============================================
+ Hits 313258 313318 +60 - Misses 53594 54346 +752 + Partials 18484 18468 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
alamb
commented
Aug 12, 2026
Contributor
Thanks again @JSOD11 |
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 freeto 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.
Backport of #23903 to
branch-55.