Uh oh!
There was an error while loading. Please reload this page.
refactor(physical-plan): Simplify ExecutionPlan API with replace_children - #23903
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #23903 +/- ##
==========================================
- Coverage 81.29% 81.14% -0.15%
==========================================
Files 1110 1110 Lines 385197 386132 +935 Branches 385197 386132 +935 ==========================================
+ Hits 313132 313323 +191 - Misses 53588 54341 +753 + Partials 18477 18468 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ExecutionPlan API with replace_childrenThank 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 |
JSOD11
commented
Jul 30, 2026
cc @zhuqi-lucas Took a stab at this, let me know what you think! |
zhuqi-lucas
commented
Jul 31, 2026
run benchmarks |
adriangbot
commented
Jul 31, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing jsod/replace-children-07-25-26 (a2ccc1a) to 88365dd (merge-base) diff using: tpch File an issue against this benchmark runner |
adriangbot
commented
Jul 31, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing jsod/replace-children-07-25-26 (a2ccc1a) to 88365dd (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
adriangbot
commented
Jul 31, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing jsod/replace-children-07-25-26 (a2ccc1a) to 88365dd (merge-base) diff using: tpcds File an issue against this benchmark runner |
alamb
commented
Aug 7, 2026
(the performance benchmarks also look good -- thanks for running those @zhuqi-lucas ) |
zhuqi-lucas
commented
Aug 9, 2026
run benchmark sql_planner |
| fn replace_children( | ||
| self: Arc<Self>, | ||
| children: Vec<Arc<dyn ExecutionPlan>>, | ||
| hint: ChildrenPropertiesHint, |
There was a problem hiding this comment.
Would it make sense to pass a ReplaceChildrenHints structure here (that includes ChildrenPropertiesHint as a member)? That could make the API easier to extend in the future.
There was a problem hiding this comment.
Yeah this is a good point, see comment below.
| fn reset_state(self: Arc<Self>) -> Result<Arc<dyn ExecutionPlan>> { | ||
| let children = self.children().into_iter().cloned().collect(); | ||
| self.with_new_children(children) | ||
| self.replace_children(children, ChildrenPropertiesHint::Recompute) |
There was a problem hiding this comment.
Why does reset_state require recomputing properties? It seems to me that resetting runtime state should not affect the plan properties.
For performance, it would be nice to avoid recomputing properties in reset_plan_states, especially for plans without an explicit reset_state override. For example, a simple filter and projection should not need to recompute anything when their state is reset.
There was a problem hiding this comment.
Good call, agreed. Just pushed a commit swapping this to SameProperties.
| /// A hint from `replace_children_if_necessary` to `replace_children` indicating | ||
| /// whether the properties of the new children must be recomputed. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum ChildrenPropertiesHint { |
There was a problem hiding this comment.
A "hint" suggests that its absence should not logically break anything. However, if children are not recomputed when required, it could lead to bugs.
Would ChildrenPropertiesRequirement be a better name?
There was a problem hiding this comment.
This is a good point, how about a design like this to tie in with your comment above?
pub struct ReplaceChildrenOptions {
pub children_properties: ChildrenPropertiesMode,
}
pub enum ChildrenPropertiesMode {
SameProperties,
Recompute,
}
Which gives us something like this:
fn replace_children(
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
options: ReplaceChildrenOptions,
) -> Result<Arc<dyn ExecutionPlan>> {
match options.children_properties {
ChildrenPropertiesMode::SameProperties => {
self.with_new_children_and_same_properties(children)
}
ChildrenPropertiesMode::Recompute => self.with_new_children(children),
}
}
Interested in hearing what everyone thinks. If we agree on this design, I'll go ahead and swap all the implementations and call sites. cc @askalt@zhuqi-lucas@alamb
There was a problem hiding this comment.
I see a thumbs up, so I went ahead and pushed a new commit moving us to this shape. Looking forward to hearing all of your thoughts.
# Conflicts: # datafusion/proto/tests/cases/roundtrip_physical_plan.rs
…-07-25-26 # Conflicts: # datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
alamb
commented
Aug 11, 2026
I merged up from main |
alamb
left a comment
There was a problem hiding this comment.
Thank you @JSOD11@zhuqi-lucas and @askalt
I think this looks good to me.
I took the liberty of pushing some additional documentation changes to the ExecutionPlan trait to make it clearer what was going on here and what users should do.
| the properties do not match the children, `ChildrenPropertiesMode::Recompute`, | ||
| follow the body of `with_new_children`. | ||
| For example, take a look at the implementation for `FilterExec`: |
| /// A hint from `replace_children_if_necessary` to `replace_children` indicating | ||
| /// whether the properties of the new children must be recomputed. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum ChildrenPropertiesHint { |
| } | ||
| #[deprecated(since = "55.0.0", note = "Use `replace_children_if_necessary`")] | ||
| pub fn with_new_children_if_necessary( |
…into jsod/replace-children-07-25-26
| ReplaceChildrenOptions { | ||
| children_properties: ChildrenPropertiesMode::SameProperties, | ||
| }, |
There was a problem hiding this comment.
nit: we can add a constructor to reduce this a bit, e.g.
ReplaceChildrenOptions::new(ChildrenPropertiesMode::SameProperties)
There was a problem hiding this comment.
Sounds good to me, added.
| pub enum ChildrenPropertiesMode { | ||
| /// The plan properties of the new children are identical to the properties | ||
| /// of the existing children, so we can skip recomputation. | ||
| SameProperties, |
There was a problem hiding this comment.
nit: to be consistent with the second variant:
| SameProperties, | |
| Keep, |
There was a problem hiding this comment.
I like this name, matches the other variant better. Just changed.
JSOD11
commented
Aug 12, 2026
Friendly bump on this one before finalizing the release, think all that's left is to hit the merge button. |
alamb
commented
Aug 12, 2026
I think this one missed the 55 branch cut: #22393 (comment) We can potentially open a proposed backport on the branch-55 line It would be good to put a note on #22393 to coordinate with @timsaucer |
timsaucer
commented
Aug 12, 2026
I need to make another RC anyways, so after you merge this into main if you make a PR targeting branch-55 we should be able to include it. |
alamb
commented
Aug 12, 2026
Merging to main@ |
Uh oh!
There was an error while loading. Please reload this page.
Which issue does this PR close?
ExecutionPlanchildren-replacement API #23441User-facing changes: Deprecating
with_new_childrenandwith_new_children_and_same_propertiesin favor ofreplace_childrenAs noted here, while the addition of
with_new_children_and_same_propertieshas 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 ofExecutionPlanin a way that could be confusing for users.Thus, to rectify this, we unify these methods by introducing
replace_children, and we shift towardswith_new_children_if_necessaryas the universal entry point for replacing the children of anExecutionPlan.replace_childrensimplifies the interface for users by taking an enum calledChildrenPropertiesHintas an argument. The enum has two variants,SamePropertiesandRecompute, which function as a hint toreplace_childrenfrom the caller as to whether or not the properties need to be recomputed.Trait implementation migration
To migrate from
with_new_childrenandwith_new_children_and_same_propertiestoreplace_children, I went through all 93 implementations ofwith_new_childrenand implementedreplace_childrenwith amatchstatement matching on theChildrenPropertiesHint. In the case that the properties match,ChildrenPropertiesHint::SameProperties, and we have an implementation ofwith_new_children_and_same_properties, then we follow the body ofwith_new_children_and_same_properties. In the case that the properties do not match,ChildrenPropertiesHint::Recompute, we follow the body ofwith_new_children. In the cases in which there was no implementation ofwith_new_children_and_same_properties, I simply move the body ofwith_new_childrenintoreplace_childrenand ignore the hint.I mark
with_new_childrenandwith_new_children_and_same_propertiesas deprecated with a migration note pointing toreplace_children. After a couple releases, we'll drop the deprecated methods.Example
For example, here is what the implementation looks like for
FilterExecafter this change: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_necessaryrather than having them scattered around many methods. However, for this to all work we must ensure that users actually do usewith_new_children_if_necessaryby making it obvious to them somehow. I feelreplace_childrenis a step in the right direction, but it could still be easy for a user to misswith_new_children_if_necessaryand just jump to usingreplace_childreninstead.Usage Migration
replace_childrenis called fromwith_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_childrenandwith_children_and_same_propertiestowith_new_children_if_necessarywhere it made sense to do so, and I migratedwith_new_children_if_necessaryto usereplace_childrenwith the correct hint filled in at each branch.Testing
cargo fmt --allcargo check -p datafusion-physical-plancargo check -p datafusion-physical-optimizercargo check -p datafusion --libcargo check -p datafusion-ffi