Cache PlanProperties, add fast-path for with_new_children#19792
Open
askalt wants to merge 5 commits intoapache:mainfrom
Open
Cache PlanProperties, add fast-path for with_new_children#19792askalt wants to merge 5 commits intoapache:mainfrom
PlanProperties, add fast-path for with_new_children#19792askalt wants to merge 5 commits intoapache:mainfrom
Conversation
github-actions
bot
added
documentation
askalt
changed the title
with_new_childrenwith_new_children
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
72ff575 to
796f731
Compare
Contributor
Author
|
Also added a typical analytical query plan re-usage benchmark. On the $ cargo bench --profile=release-nonlto --bench plan_reuse |
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
796f731 to
5601c4f
Compare
Contributor
|
I filed a ticket to track this idea |
Contributor
|
run benchmark sql_planner |
|
🤖 |
44 tasks
Contributor
Could you move the plan_reuse benchmark into its own PR (as I think it is valuable both for this PR and others, and it makes it easier to automatically compare performance) |
|
Benchmark script failed with exit code 101. Last 10 lines of output: Click to expand |
Contributor
Author
Done in #19806 |
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
5601c4f to
99cf634
Compare
Contributor
Author
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
99cf634 to
b81dd66
Compare
alamb
reviewed
Jan 14, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @askalt -- this is quite clever and I think it looks very promising
I also think we may be able to potentially make with_new_children even faster by checking the children as well -- and if they are the same there is no reason to recompute everything either.
However, this likely won't help your usecase as the children will likely change (their states need to be reset) 🤔
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
b81dd66 to
741b085
Compare
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
15d2b0f to
ade5cc1
Compare
Contributor
|
Jut FYI @askalt -- this is very much on my list to review carefully, but I am traveling this week at a conference so I am struggling to fine enough uninterrupted time. I will get it done |
Contributor
Author
Ok, thank you! |
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
0c08a40 to
fad95f2
Compare
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
4 times, most recently
from
0ba17e7 to
313e429
Compare
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
2 times, most recently
from
b0e3ec6 to
90fa5ba
Compare
This patch aims to implement a fast-path for the ExecutionPlan::with_new_children function for some plans, moving closer to a physical plan re-use implementation and improving planning performance. If the passed children properties are the same as in self, we do not actually recompute self's properties (which could be costly if projection mapping is required). Instead, we just replace the children and re-use self's properties as-is. To be able to compare two different properties -- ExecutionPlan::properties(...) signature is modified and now returns `&Arc<PlanProperties>`. If `children` properties are the same in `with_new_children` -- we clone our properties arc and then a parent plan will consider our properties as unchanged, doing the same. - Return `&Arc<PlanProperties>` from `ExecutionPlan::properties(...)` instead of a reference. - Implement `with_new_children` fast-path if there is no children properties changes for all major plans. Note: currently, `reset_plan_states` does not allow to re-use plan in general: it is not supported for dynamic filters and recursive queries features, as in this case state reset should update pointers in the children plans. Closes apache#19796
askalt
force-pushed
the
askalt/with_new_children_fast_path
branch
from
90fa5ba to
3346b77
Compare
alamb
approved these changes
Feb 13, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @askalt -- I think this is a great change
While it is a (small) API change I don't see much way around it and it does make query planning significantly faster in some cases.
I took the liberty of merging up from main to resolve some conflicts with this PR as well as updating the upgrading guide and fixing a few bugs I found (metrics not being reset). Can you please look at the latest few commits to make sure they look reasonable?
I think we should leave this one open for a few more days to see if anyone has comments, but otherwise I think it is ready go from my perspective
Thank you for your patience
| /// making an independent instance of the execution plan to re-execute it, avoiding | ||
| /// re-planning stage. | ||
| fn bench_reset_plan_states(c: &mut Criterion) { | ||
| env_logger::init(); |
Contributor
There was a problem hiding this comment.
I wonder if this is an intended change
| name: String, | ||
| plan: FFI_ExecutionPlan, | ||
| properties: PlanProperties, | ||
| properties: Arc<PlanProperties>, |
Contributor
There was a problem hiding this comment.
I agree with your analysis -- since the actual extern "C" signature hasn't change, this change should be backwards compatibile.
Thank you
Contributor
|
run benchmark sql_planner |
Contributor
|
run benchmark plan_reuse |
This comment was marked as outdated.
This comment was marked as outdated.
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
Benchmark script failed with exit code 101. Last 10 lines of output: Click to expand |
Contributor
|
run benchmark sql_planner |
|
🤖 |
Contributor
|
|
|
🤖: Benchmark completed Details
|
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.
PlanPropertiesredundently #19796This patch aims to implement a fast-path for the ExecutionPlan::with_new_children function for some plans, moving closer to a physical plan re-use implementation and improving planning performance. If the passed children properties are the same as in self, we do not actually recompute self's properties (which could be costly if projection mapping is required). Instead, we just replace the children and re-use self's properties as-is.
To be able to compare two different properties -- ExecutionPlan::properties(...) signature is modified and now returns
&Arc<PlanProperties>. Ifchildrenproperties are the same inwith_new_children-- we clone our properties arc and then a parent plan will consider our properties as unchanged, doing the same.&Arc<PlanProperties>fromExecutionPlan::properties(...)instead of a reference.with_new_childrenfast-path if there is no children properties changes for allmajor plans.
Note: currently,
reset_plan_statesdoes not allow to re-use plan in general: it is notsupported for dynamic filters and recursive queries features, as in this case state reset
should update pointers in the children plans.