Reduce cloning in LogicalPlanBuilder by joroKr21 · Pull Request #17675 · apache/datafusion

joroKr21 · 2025-09-19T17:16:46Z

Which issue does this PR close?

Related to Remove Arc<LogicalPlan> from LogicalPlan, stop copying LogicalPlans #4628 but goes in the other direction, using Arc<LogicalPlan> in LogicalPlanBuilder

Rationale for this change

Since currently we do have Arc<LogicalPlan> in the tree it's a bit weird that the plan builder forces us to unwrap and/or clone these plans in many cases and prevents node sharing when using the builder APIs.

What changes are included in this PR?

Migrate function arguments from LogicalPlan to impl Into<Arc<LogicalPlan>>
~~Migrate function arguments from &Schema to impl Into<SchemaRef>~~
Add a new LogicalPlanBuilder::build_arc function
Update usages (mostly in tests)

Are these changes tested?

Relying on existing tests, this is mostly a compile-time change.

Are there any user-facing changes?

Yes, a lot functions in LogicalPlanBuilder have changed signatures.

functions that accepted an owned LogicalPlan will continue to work as-is but now also accept Arc<LogicalPlan>
~~functions that accepted &Schema and cloned internally will break user code, so this is up for debate~~
add a new LogicalPlanBuilder::build_arc function

alamb · 2025-09-26T16:54:41Z

I will try and review this carefully over the next day or two

findepi

partial review, < datafusion/expr/src/logical_plan/builder.rs

findepi · 2025-09-26T17:13:38Z

+if matches!(plan.as_ref(), LogicalPlan::Projection(_)) {
 // special case Projection to avoid adding multiple projections
- LogicalPlan::Projection(Projection { expr, input, .. }) => {
- let new_exprs = coerce_exprs_for_schema(expr, input.schema(), schema)?;
- let projection = Projection::try_new(new_exprs, input)?;
- Ok(LogicalPlan::Projection(projection))
- }
- _ => {
- let exprs: Vec<Expr> = plan.schema().iter().map(Expr::from).collect();
- let new_exprs = coerce_exprs_for_schema(exprs, plan.schema(), schema)?;
- let add_project = new_exprs.iter().any(|expr| expr.try_as_col().is_none());
- if add_project {
- let projection = Projection::try_new(new_exprs, Arc::new(plan))?;
- Ok(LogicalPlan::Projection(projection))
- } else {
- Ok(plan)
- }
+ let LogicalPlan::Projection(Projection { expr, input, .. }) =
+ Arc::unwrap_or_clone(plan)
+ else {
+ unreachable!()
+ };


The matching here became more complicated. Is this worth it?

It's not that much more complicated - but yes, it is a bit annoying that you can't first match and then unwrap or clone - I guess we could always clone the inner Projection node instead of trying to unwrap - wdyt?

findepi · 2025-09-26T17:14:01Z

 self,
 name: String,
- recursive_term: LogicalPlan,
+ recursive_term: impl Into<Arc<LogicalPlan>>,


would just Arc<LogicalPlan> work?

It would work but it would make it more restrictive on the caller side. with impl Into<Arc<LogicalPlan>> they can pass in either LogicalPlan or Arc<LogicalPlan>. I guess there's also an argument to be made that if we force callers to change maybe they would discover some redundant cloning but I feel like it's unnecessary breakage.

findepi · 2025-09-26T18:36:21Z

 /// Create a [CopyTo] for copying the contents of this builder to the specified file(s)
 pub fn copy_to(
- input: LogicalPlan,
+ input: impl Into<Arc<LogicalPlan>>,


can it be just Arc<LogicalPlan>?

alamb

Thank you @joroKr21 -- This seems like a reasonable change and hopefully it will help performance

I think the changes to avoid Schema clones will help , though I just double checked and a Schema is mostly an empty HashMap and an Arc (Fields) so clone'ing isn't that expensive.

Also, I didn't find many places in the code by inspection where we saved a deep clone of a LogicalPlan -- did I miss any that you know of?

I would like to see two things before approving:

Benchmarks

If we are going to change the APIs around and disrupt downstream users, I think they should get something from it, in this case better planning performance.

Unfortunately, the planning benchmarks are currently broken but @pepijnve and I are working on that: #17801

An entry in the upgrade guide

Let's add an entr in the upgrade guide explaining what needs to be changed on upgrade, to help downstream users:

https://github.com/apache/datafusion/blob/main/docs/source/library-user-guide/upgrading.md#datafusion-5100

alamb · 2025-09-26T20:55:29Z

 expr,
-file_schema.clone(),
-table_schema.clone(),
+Arc::clone(&file_schema),


I do love all these Arc::clone as it makes it clear we aren't cloning schema a bunch

alamb · 2025-09-26T21:02:55Z

- )
- })
- .collect::<Result<Vec<_>>>()?;
- curr_plan.with_new_exprs(curr_plan.expressions(), new_inputs)


here is one example of avoiding a clone, I think

alamb · 2025-09-26T21:04:08Z

 }

-pub fn table_source(table_schema: &Schema) -> Arc<dyn TableSource> {
- // TODO should we take SchemaRef and avoid cloning?


🎉 yes we should!

@alamb should I understand that you prefer to keep this change although it's breaking? Or should I rather revert it?
I think the changes to avoid Schema clones will help , though I just double checked and a Schema is mostly an empty HashMap and an Arc (Fields) so clone'ing isn't that expensive.
That's a good point, I didn't look into it. I'm open to reverting the changes in the logical plan builder from SchemaRef back to &Schema to reduce breaking changes but I wouldn't change logical2physical.

I reverted that part, it's not a big win

alamb · 2025-09-26T21:05:29Z


-/// Convert a logical expression to a physical expression (without any simplification, etc)
-pub fn logical2physical(expr: &Expr, schema: &Schema) -> Arc<dyn PhysicalExpr> {
- // TODO this makes a deep copy of the Schema. Should take SchemaRef instead and avoid deep copy


joroKr21 · 2025-09-28T09:23:54Z

I think the changes to avoid Schema clones will help , though I just double checked and a Schema is mostly an empty HashMap and an Arc (Fields) so clone'ing isn't that expensive.

That's a good point, I didn't look into it. I'm open to reverting the changes in the logical plan builder from SchemaRef back to &Schema to reduce breaking changes but I wouldn't change logical2physical.

Also, I didn't find many places in the code by inspection where we saved a deep clone of a LogicalPlan -- did I miss any that you know of?

I don't think there's any deep cloning - after all the LogicalPlan children are also Arcs. But I also don't see much of a downside if we use impl Into<Arc<LogicalPlan>> since it allows using either one.

If we are going to change the APIs around and disrupt downstream users, I think they should get something from it, in this case better planning performance.

I guess that really depends on how much we use the logical plan builder internally.

alamb · 2025-09-28T12:57:13Z

I think the changes to avoid Schema clones will help , though I just double checked and a Schema is mostly an empty HashMap and an Arc (Fields) so clone'ing isn't that expensive.
That's a good point, I didn't look into it. I'm open to reverting the changes in the logical plan builder from SchemaRef back to &Schema to reduce breaking changes but I wouldn't change logical2physical.
Also, I didn't find many places in the code by inspection where we saved a deep clone of a LogicalPlan -- did I miss any that you know of?
I don't think there's any deep cloning - after all the LogicalPlan children are also Arcs. But I also don't see much of a downside if we use impl Into<Arc<LogicalPlan>> since it allows using either one.

I agree this change seems non disruptive for downstream users and is a good plan

If we are going to change the APIs around and disrupt downstream users, I think they should get something from it, in this case better planning performance.
I guess that really depends on how much we use the logical plan builder internally.

I think it is used frequently for planning and Dataframe APIs:
https://github.com/apache/datafusion/blob/62e6d5e259d32f590b6d61bad6b08ece7b3416b9/datafusion/core/src/dataframe/mod.rs#L392-L391

Omega359 · 2025-10-28T19:15:06Z

Can we get a run of the planning benchmark compared to main to see if this has any affect on performance?

codecov-commenter · 2026-07-23T14:14:11Z

Codecov Report

❌ Patch coverage is 76.77419% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.22%. Comparing base (c1b39bd) to head (c3f0903).

Files with missing lines	Patch %	Lines
datafusion/expr/src/logical_plan/builder.rs	83.49%	6 Missing and 11 partials ⚠️
...atafusion/optimizer/src/scalar_subquery_to_join.rs	60.00%	0 Missing and 6 partials ⚠️
datafusion/expr/src/expr_rewriter/mod.rs	70.58%	1 Missing and 4 partials ⚠️
...tafusion/optimizer/src/optimize_projections/mod.rs	50.00%	0 Missing and 3 partials ⚠️
datafusion/optimizer/src/analyzer/type_coercion.rs	0.00%	0 Missing and 2 partials ⚠️
datafusion/optimizer/src/push_down_filter.rs	0.00%	0 Missing and 2 partials ⚠️
datafusion/optimizer/src/unions_to_filter.rs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #17675 +/- ##
==========================================
- Coverage 81.22% 81.22% -0.01% 
==========================================
Files 1111 1111 Lines 389991 389985 -6 Branches 389991 389985 -6 ==========================================
- Hits 316783 316762 -21 - Misses 54590 54602 +12 - Partials 18618 18621 +3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Migrate function arguments from `LogicalPlan` to `impl Into<Arc<LogicalPlan>>` - Update usages (mostly in tests)

github-actionsBot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate labels Sep 19, 2025

joroKr21 force-pushed the arc-builder branch 5 times, most recently from a9fa7e5 to 21dd5e4Compare September 19, 2025 19:10

joroKr21 marked this pull request as ready for review September 19, 2025 19:31

joroKr21 force-pushed the arc-builder branch 2 times, most recently from 9ef878c to fc12cefCompare September 21, 2025 13:29

joroKr21 mentioned this pull request Sep 26, 2025
Avoid redundant Schema clones #17643
Merged

joroKr21 force-pushed the arc-builder branch 2 times, most recently from 45e5601 to 9c1be02Compare September 26, 2025 13:37

github-actionsBot added physical-expr Changes to the physical-expr crates datasource Changes to the datasource crate labels Sep 26, 2025

findepi reviewed Sep 26, 2025
View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Sep 26, 2025

alamb reviewed Sep 26, 2025
View reviewed changes

joroKr21 force-pushed the arc-builder branch 2 times, most recently from 2c28b02 to d0b38d1Compare October 20, 2025 21:40

github-actionsBot removed the sql SQL Planner label Oct 20, 2025

joroKr21 force-pushed the arc-builder branch 3 times, most recently from 7fce6ef to e77063dCompare October 21, 2025 13:24

joroKr21 force-pushed the arc-builder branch 2 times, most recently from 22d451f to 2aaeb76Compare May 21, 2026 21:21

joroKr21 force-pushed the arc-builder branch 2 times, most recently from 318c916 to bb8b2ddCompare May 29, 2026 11:29

joroKr21 force-pushed the arc-builder branch from bb8b2dd to 72ccfdaCompare June 10, 2026 16:32

joroKr21 force-pushed the arc-builder branch from 72ccfda to d785565Compare June 19, 2026 16:21

joroKr21 force-pushed the arc-builder branch from d785565 to f37cc43Compare July 1, 2026 07:15

joroKr21 force-pushed the arc-builder branch from f37cc43 to d9c17daCompare July 12, 2026 14:54

joroKr21 force-pushed the arc-builder branch from d9c17da to d006305Compare July 23, 2026 13:44

joroKr21 force-pushed the arc-builder branch 13 times, most recently from 7750d90 to aab565fCompare July 30, 2026 11:57

joroKr21 force-pushed the arc-builder branch 6 times, most recently from fcf18a0 to 75bcfd1Compare August 3, 2026 18:55

Reduce cloning in LogicalPlanBuilder
c3f0903
- Migrate function arguments from `LogicalPlan` to `impl Into<Arc<LogicalPlan>>` - Update usages (mostly in tests)

Uh oh!

Conversation

joroKr21 commented Sep 19, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Sep 26, 2025

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Benchmarks

An entry in the upgrade guide

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joroKr21 commented Sep 28, 2025

Uh oh!

alamb commented Sep 28, 2025

Uh oh!

Omega359 commented Oct 28, 2025

Uh oh!

codecov-commenter commented Jul 23, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joroKr21 commented Sep 19, 2025•
edited
Loading

codecov-commenter commented Jul 23, 2026•
edited
Loading