Wrap immutable plan parts into Arc #19893

askalt · 2026-01-20T08:05:53Z

Which issue does this PR close?

Closes Wrap more immutable plan parts into Arc #19852

Rationale 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?

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 · 2026-01-20T08:22:30Z

run benchmark reset_plan_states

alamb-ghbot · 2026-01-20T08:22:34Z

🤖 Hi @askalt, thanks for the request (#19893 (comment)). scrape_comments.py only responds to whitelisted users. Allowed users: Dandandan, Omega359, adriangb, alamb, comphead, gabotechs, geoffreyclaude, klion26, rluvaton, xudong963, zhuqi-lucas.

askalt · 2026-01-20T08:24:31Z

@xudong963 could you please run benchmark reset_plan_states?

xudong963 · 2026-01-20T09:32:43Z

run benchmark reset_plan_states

alamb-ghbot · 2026-01-20T09:43:39Z

🤖 ./gh_compare_branch_bench.sh compare_branch_bench.sh Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing askalt/arc_immutable (bb97763) to d90d074 diff
BENCH_NAME=reset_plan_states
BENCH_COMMAND=cargo bench --features=parquet --bench reset_plan_states
BENCH_FILTER=
BENCH_BRANCH_NAME=askalt_arc_immutable
Results will be posted here when complete

alamb-ghbot · 2026-01-20T10:10:59Z

🤖: Benchmark completed

Details

group     askalt_arc_immutable                   main
-----     --------------------                   ----
query1    1.00     32.2±1.09ms        ? ?/sec    1.01     32.4±0.31ms        ? ?/sec
query2    1.00      9.2±0.08ms        ? ?/sec    1.03      9.5±0.11ms        ? ?/sec
query3    1.00     12.2±0.25ms        ? ?/sec    1.15     14.1±0.08ms        ? ?/sec

datafusion/physical-plan/src/projection.rs

datafusion/physical-plan/src/filter.rs

- 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`.

crepererum · 2026-01-22T16:38:55Z

I understand that we wanna avoid breaking changes and hence are kinda liberal with impl Into<...>/impl AsRef<...> parameters. However these methods are generic and will be compiled on the call-site, i.e. whoever calls these methods has to recompile them. Furthermore calling these methods with different types duplicates these methods in LLVM and likely also in the resulting binary. This means: longer compile times and binary size bloat.

Hence I would favor if we could avoid these impl parameters.

askalt · 2026-01-22T16:56:10Z

I understand that we wanna avoid breaking changes and hence are kinda liberal with impl Into<...>/impl AsRef<...> parameters. However these methods are generic and will be compiled on the call-site, i.e. whoever calls these methods has to recompile them. Furthermore calling these methods with different types duplicates these methods in LLVM and likely also in the resulting binary. This means: longer compile times and binary size bloat.

Hence I would favor if we could avoid these impl parameters.

Actually, I also think that using a generic here isn't justified, since &Vec<u8> can always be converted to a slice. I believe the API should be as simple as possible. Let's get @alamb opinion.

alamb · 2026-01-23T21:15:35Z

I understand that we wanna avoid breaking changes and hence are kinda liberal with impl Into<...>/impl AsRef<...> parameters. However these methods are generic and will be compiled on the call-site, i.e. whoever calls these methods has to recompile them. Furthermore calling these methods with different types duplicates these methods in LLVM and likely also in the resulting binary. This means: longer compile times and binary size bloat.
Hence I would favor if we could avoid these impl parameters.

Actually, I also think that using a generic here isn't justified, since &Vec<u8> can always be converted to a slice. I believe the API should be as simple as possible. Let's get @alamb opinion.

Indeed my concern is the impact on (all) downstream users during the upgrade. I believe the initial PR from @askalt was substantially larger because it required changing a bunch of callsites.

If we can avoid the use of impl and not require many changes (I understand some may be impossible to avoid) that sounds good to me

alamb

Thank you for this PR @askalt and for the review @crepererum and @martin-g

I think the performance results are quite compelling and this is quite close.

I agree it would be find to avoid the AsRef impls, if you are willing to do so @askalt . I also think it would be fine to leave this as is.

In any event, I think we should add an example to project_schema that shows how to call it with Option<&Vec<usize>> (what is passed by TableProvider::scan)

Before we merge this PR I think we

should also add a note to the upgrade guide with a note about this change (I can help with this, but I didn't want to push changes to this PR before we have consensus approach)
Clean up the API for OptionProjectionRef (move it, and make Option<ProjectionRef>)

alamb · 2026-01-23T21:54:21Z

datafusion/common/src/utils/mod.rs

 pub fn project_schema(
    schema: &SchemaRef,
-    projection: Option<&Vec<usize>>,
+    projection: Option<&impl AsRef<[usize]>>,


Per @crepererum 's suggestion, I tried out what it would look like and came up with

Change project_schema to use Option<&[usize]> askalt/datafusion#3

It does seem to be reasonable

The original signature is Option<&Vec<..>> I think to align with TableProvider::scan (which also shouldn't have a owned Vec, but that I think is a historic accident)

alamb · 2026-01-23T21:58:16Z

datafusion/physical-expr/src/projection.rs

 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct ProjectionExprs {
-    exprs: Vec<ProjectionExpr>,
+    exprs: Arc<[ProjectionExpr]>,


I think it is worth commenting here the rationale for using Arc<[...]>, namely that it makes this structure inexpensive to copy as happens during PhysicalPlanning

alamb · 2026-01-23T21:59:51Z

datafusion/physical-plan/src/aggregates/mod.rs

    mode: AggregateMode,
    /// Group by expressions
-    group_by: PhysicalGroupBy,
+    group_by: Arc<PhysicalGroupBy>,


likewise here I think it would be good to note they are Arc to make clone/plan rewriting faster

alamb · 2026-01-23T22:05:57Z

datafusion/physical-plan/src/projection.rs

+    /// Returns an internal error if existing projection contains index that is
+    /// greater than len of the passed `projection`.
+    ///
+    pub fn apply_projection<'a>(


I see this is basically refactored out of NestedLoops join (and maybe elsewhere)

alamb · 2026-01-23T22:10:54Z

datafusion/physical-plan/src/projection.rs

    }
 }

+/// Describes an option immutable reference counted shared projection.


I see why you need something like this (as it changes the signture of TableProvider) but this seems inconsistent with the other representations of projections in DataFusion/Arrow I am familiar with, which are represented with an Option rather than having the Option internally

I recommend:

Move this into datafusion/physical-expr/src/projection.rs (not in physical plan) so it is near ProjectionExprs

Move the Option outside (so the signature is Option<ProjectionRef> rather than OptionProjectionRef)

I also recommend documenting why this structure exists (because it is cheap to clone)

Something like

/// This structure represents projecting a set of columns by index. /// It uses an `Arc` internally to make it cheap to clone

github-actions bot added physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate common Related to common crate proto Related to proto crate physical-plan Changes to the physical-plan crate labels Jan 20, 2026

askalt force-pushed the askalt/arc_immutable branch from b9caf1d to bb97763 Compare January 20, 2026 08:17

askalt mentioned this pull request Jan 20, 2026

Cache PlanProperties, add fast-path for with_new_children #19792

Open

askalt mentioned this pull request Jan 20, 2026

Avoid changing project_schema signature askalt/datafusion#2

Closed

askalt force-pushed the askalt/arc_immutable branch 2 times, most recently from 70a396a to 3614687 Compare January 21, 2026 10:04

github-actions bot added datasource Changes to the datasource crate and removed optimizer Optimizer rules core Core DataFusion crate common Related to common crate proto Related to proto crate labels Jan 21, 2026

askalt force-pushed the askalt/arc_immutable branch from 3614687 to bb97763 Compare January 21, 2026 10:08

github-actions bot added optimizer Optimizer rules core Core DataFusion crate common Related to common crate proto Related to proto crate and removed datasource Changes to the datasource crate labels Jan 21, 2026

askalt force-pushed the askalt/arc_immutable branch from bb97763 to f2d829a Compare January 21, 2026 10:11

martin-g reviewed Jan 21, 2026

View reviewed changes

datafusion/physical-plan/src/projection.rs Show resolved Hide resolved

datafusion/physical-plan/src/filter.rs Outdated Show resolved Hide resolved

askalt force-pushed the askalt/arc_immutable branch from f2d829a to 050e6bb Compare January 21, 2026 12:39

alamb mentioned this pull request Jan 23, 2026

Change project_schema to use Option<&[usize]> askalt/datafusion#3

Open

alamb added the api change Changes the API exposed to users of the crate label Jan 23, 2026

alamb reviewed Jan 23, 2026

View reviewed changes

Wrap immutable plan parts into Arc #19893

Are you sure you want to change the base?

Wrap immutable plan parts into Arc #19893

Conversation

askalt commented Jan 20, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Uh oh!

askalt commented Jan 20, 2026

Uh oh!

alamb-ghbot commented Jan 20, 2026

Uh oh!

askalt commented Jan 20, 2026

Uh oh!

xudong963 commented Jan 20, 2026

Uh oh!

alamb-ghbot commented Jan 20, 2026

Uh oh!

alamb-ghbot commented Jan 20, 2026

Uh oh!

Uh oh!

Uh oh!

crepererum commented Jan 22, 2026

Uh oh!

askalt commented Jan 22, 2026

Uh oh!

alamb commented Jan 23, 2026

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alamb left a comment •

edited

Loading