Wrap immutable plan parts into Arc (make creating ExecutionPlans less costly)
#19893
+509
−290
Conversation
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
github-actions
bot
added
physical-expr
askalt
force-pushed
the
askalt/arc_immutable
branch
from
b9caf1d to
bb97763
Compare
Contributor
Author
|
run benchmark reset_plan_states |
|
🤖 Hi @askalt, thanks for the request (#19893 (comment)). |
Contributor
Author
|
@xudong963 could you please |
Member
|
run benchmark reset_plan_states |
|
🤖 |
|
🤖: Benchmark completed Details
|
askalt
force-pushed
the
askalt/arc_immutable
branch
2 times, most recently
from
70a396a to
3614687
Compare
github-actions
bot
added
datasource
askalt
force-pushed
the
askalt/arc_immutable
branch
from
3614687 to
bb97763
Compare
github-actions
bot
added
optimizer
askalt
force-pushed
the
askalt/arc_immutable
branch
from
bb97763 to
f2d829a
Compare
martin-g
reviewed
Jan 21, 2026
askalt
force-pushed
the
askalt/arc_immutable
branch
from
20b3042 to
e6fed67
Compare
Contributor
Author
|
I decided to simplify |
askalt
force-pushed
the
askalt/arc_immutable
branch
3 times, most recently
from
1e27378 to
2c0746c
Compare
alamb
reviewed
Jan 28, 2026
Contributor
There was a problem hiding this comment.
I am sorry this is taking so long, but I a very worried about API churn and it takes me a non trivial amount of time to review the potential implications of this change
I have two major points:
- I really think it is important to not force downstream users to change how they call
project_schemaas it is a very common operation and there is no technical reason we can't keep their code the same (though it is harder in Datafusion). I made a PR to do so (and backout many of the changes currently required in this PR): askalt#5 as suggested by @crepererum - As I mentioned previously
OptionProjectionRefseems overly complicated to me to add to the public API -- and because theOptionis wrapped, it means that the normal functions forOption(like match and .and_then, etc) don't work. I think it would be simpler, and follow the rest of the code more closely if it wereOption<ProjectionRef>and the special projection application code is put into ProjectionRef
Thank you for your work on this @askalt
Contributor
Author
Ok, I will do it. |
askalt
force-pushed
the
askalt/arc_immutable
branch
from
2c0746c to
6053a7c
Compare
github-actions
bot
removed
catalog
askalt
force-pushed
the
askalt/arc_immutable
branch
from
6053a7c to
5d11e60
Compare
github-actions
bot
removed
the
documentation
Contributor
Author
@alamb, done as suggested, please check
|
alamb
approved these changes
Feb 3, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @askalt - this looks really nice to me now. I think the code is faster and easier to work with 🏆 (HashJoinExecBuilder, NestedLoopJoinExecBuilder are especially nice)
I'll kick off a final benchmark run but I think this should be good to merge tomorrow
| hash_join.null_aware, | ||
| )?))) | ||
| Ok(Some(Arc::new( | ||
| HashJoinExecBuilder::from(hash_join) |
| @@ -1370,9 +1377,9 @@ impl ExecutionPlan for AggregateExec { | |||
| ) -> Result<Arc<dyn ExecutionPlan>> { | |||
| let mut me = AggregateExec::try_new_with_schema( | |||
Contributor
There was a problem hiding this comment.
As a follow on PR, I think we can probably avoid a lot of redundant work here -- try_new_with_schema does many things, including re-compute plan properties
| } | ||
| } | ||
|
|
||
| /// Helps to build [`HashJoinExec`]. |
Contributor
|
run benchmark sql_planner |
1 similar comment
Contributor
|
run benchmark sql_planner |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
- Closes apache#19852 Improve performance of query planning and plan state re-set by making node clone cheap. - Store projection as `Option<Arc<[usize]>>` instead of `Option<Vec<usize>>` in `FilterExec`, `HashJoinExec`, `NestedLoopJoinExec`. - Store exprs as `Arc<[ProjectionExpr]>` instead of Vec in `ProjectionExprs`. - Store arced aggregation, filter, group by expressions within `AggregateExec`.
askalt
force-pushed
the
askalt/arc_immutable
branch
from
5d11e60 to
a4bbb71
Compare
alamb
changed the title
ExecutionPlans less costly)
Contributor
2010YOUY01
reviewed
Feb 10, 2026
| can_project(&self.schema(), projection.as_deref())?; | ||
| let projection = | ||
| combine_projections(projection.as_ref(), self.projection.as_ref())?; | ||
| HashJoinExecBuilder::from(self) |
Contributor
There was a problem hiding this comment.
I find this builder pattern a bit risky. Currently, HashJoinExecBuilder::from(self) clones some fields from self while implicitly resetting others (for example, the dynamic filter).
This behavior may be surprising for certain transformations. It might be safer to make such partial resets explicit, so callers are clearly aware of which fields are preserved and which are dropped.
Would it be possible to remove these builders and require all cloning to be explicit instead? Since immutable fields are already wrapped in Arc, planning performance should not be affected. The main downside I see is increased verbosity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 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?
Arc#19852Rationale for this change
Improve performance of query planning and plan state re-set by making node clone cheap.
What changes are included in this PR?
Option<Arc<[usize]>>instead ofOption<Vec<usize>>inFilterExec,HashJoinExec,NestedLoopJoinExec.Arc<[ProjectionExpr]>instead of Vec inProjectionExprs.AggregateExec.