Repository navigation
Avoid unnecessary clones in logical planning and expression simplification #26756
Description
Is your feature request related to a problem or challenge?
Part of #19795 (Improve query planning speed).
While browsing through the analyzer and simplifier code, I found code that deep-clones owned data then discards the original. These are not Arc clones, so each one copies real data (thus costlier).
Exact findings:
exclude_using_columns(datafusion/expr/src/utils.rs:412) builds a set with plan.schema().columns().iter().cloned().collect(). DFSchema::columns() already returns a fresh Vec, so every Column is cloned a second time.TypeCoercion::coerce_dml(datafusion/optimizer/src/analyzer/type_coercion.rs:346) deep-clones the whole MergeIntoOp, including all its clauses, then rebuilds the Box. It also does *value = value.clone().cast_to(..) for each assignment and insert value.- The Between rewrite and as_inlist in datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs use *boxed.clone(), which allocates a new Box only to unwrap it.
Possibly related earlier work: #5157, #9637 and #18503 (this one is closed). This looks like a pattern that will keep recurring.
Describe the solution you'd like
A small, behavior-preserving cleanup that removes these clones:
- Move values instead of cloning them where the original isn't used afterwards (into_iter(), std::mem::take, editing through &mut).
- Use x.as_ref().clone() instead of *x.clone() for boxed values.
I'd do this as one PR, covering the cases above.
Describe alternatives you've considered
- Larger structural fixes such as Remove Arc<LogicalPlan> from LogicalPlan, stop copying LogicalPlans #4628 (remove Arc) are broader and somewhat out of scope here.
- A lint to catch the pattern automatically. I don't know whether a suitable one exists, (could not find one if it already exists).
Additional context
Related open issues that I found: #19795 and #11316
Willingness to contribute: I plan to contribute to this feature independently.
Metadata
Metadata
Assignees
Labels
No labels
Activity
Omkar-Wagholikar commentedon Oct 26, 2026
take
alamb commentedon Oct 26, 2026
Is that really faster?
I agree unwrapping rather than cloning is a good idea ll around
Omkar-Wagholikar commentedon Oct 27, 2026
In theory it would, but the change (if any) is so small for this specific case that I have been unable to benchmark it properly.
Do the other changes look good to you (exclude_using_columns and moving instead of cloning in coerce_dml)?
I can narrow the PR to those as well.
alamb commentedon Oct 27, 2026
In general I think if the code looks better / cleaner (or no worse) we could merge wihtout compelling benchmark results
If it makes things a lot more complicated, then probably not worth it
Omkar-Wagholikar commentedon Oct 27, 2026
Understood, thanks. I was looking for small, relatively simple changes for my first PR in the project, so that fits well. These are patterns I've run into myself when writing Rust, which is why I noticed them.
Omega359 commentedon Oct 27, 2026
#17675 sounds similar and has been worked on for a long time now (I see force pushes to the tracking branch almost every day)
Omkar-Wagholikar commentedon Oct 27, 2026
I see, thanks for pointing that out.
What would you recommend as a course of action?
Omega359 commentedon Oct 27, 2026
I would suggest to do a comparison of your work to theirs and if there is significant overlay or conflict between them work with the author of that PR to align if you can. If there is something in your PR that doesn't overlap (expression simplification maybe - I haven't actually looked at your PR code) perhaps that could be a separate PR.
I find that oftentimes if there are multiple good competing PRs for the same thing neither is chosen. This is even more likely now since reviewer bandwidth is extremely stretched and first time committer PRs are often overlooked.
Omkar-Wagholikar commentedon Oct 28, 2026
Thanks, I went through #17675, it changes LogicalPlanBuilder to accept Arc.
My changes isolated to local to function bodies. The only file we share is type_coercion.rs, and it's different functions (coerce_output and union coercion there, coerce_dml here), so I don't expect a conflict.
I've yet to raise a PR, I thought it best to create an Issue and get confirmation for it first.
Will raise it in ASAP.
Omkar-Wagholikar commentedon Nov 3, 2026
Hi @alamb and @Omega359,
I've raised the PR, please review when you have the time.
refactor: avoid redundant clones in planning and simplification (apac…