refactor(physical-plan): Simplify ExecutionPlan API with replace_children - #23903
Open
JSOD11 wants to merge 11 commits into
Open
refactor(physical-plan): Simplify ExecutionPlan API with replace_children#23903JSOD11 wants to merge 11 commits into
ExecutionPlan API with replace_children#23903JSOD11 wants to merge 11 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23903 +/- ##
==========================================
- Coverage 81.02% 80.91% -0.12%
==========================================
Files 1105 1105
Lines 379704 380274 +570
Branches 379704 380274 +570
==========================================
+ Hits 307671 307690 +19
- Misses 53814 54375 +561
+ Partials 18219 18209 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JSOD11
changed the title
ExecutionPlan API with replace_children
github-actions
Bot
added
core
github-actions
Bot
added
datasource
|
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 |
github-actions
Bot
added
the
documentation
JSOD11
marked this pull request as ready for review
July 30, 2026 17:30
Contributor
Author
|
cc @zhuqi-lucas Took a stab at this, let me know what you think! |
Contributor
|
run benchmarks |
|
🤖 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 |
|
🤖 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 |
|
🤖 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 |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
41 tasks
Contributor
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing jsod/replace-children-07-25-26 (739fcb6) to 5eba27f (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing jsod/replace-children-07-25-26 (739fcb6) to 5eba27f (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
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?
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