Skip to content

refactor: avoid redundant clones in planning and simplification - #27029

Merged
jayzhan211 merged 2 commits into
apache:mainfrom
Omkar-Wagholikar:refactor/avoid-redundant-clones
Nov 3, 2026
Merged

jayzhan211 merged 2 commits into
apache:mainfrom
Omkar-Wagholikar:refactor/avoid-redundant-clones

Conversation

@Omkar-Wagholikar

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

I found some parts of the code that deep-clone owned data then immediately discard the original.
Each of these copies does real work.
This PR moves the values instead where the original isn't used afterwards. The code is no more complicated than before. As discussed on the issue, the aim is cleaner code. I haven't measured a speedup (sql_planner shows no difference outside noise).

What changes are included in this PR?

  • exclude_using_columns (datafusion/expr/src/utils.rs): DFSchema::columns() already returns an owned Vec, so it is now collected with into_iter() and each Column is no longer cloned a second time.
  • TypeCoercion::coerce_dml (datafusion/optimizer/src/analyzer/type_coercion.rs): the MergeIntoOp is edited in place through &mut dml.op instead of being deep-cloned and rebuilt. std::mem::take replaces value.clone() before cast_to.
  • Between rewrite and as_inlist (datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs): boxed.as_ref().clone() replaces *boxed.clone(), which allocates a Box only to unwrap it.

I have also checked this against #17675 (LogicalPlanBuilder taking Arc) and found no overlap. type_coercion.rs is the only shared file and even then the touched functions are different.

What is the testing strategy for this PR?

No new tests, since the behavior is unchanged and existing tests cover these paths:

  • cargo test -p datafusion-expr -p datafusion-optimizer, including the unit tests for the simplifier and type coercion.
  • The merge_into.slt sqllogictest, which exercises coerce_dml.
  • The full sqllogictest suite and core_integration (1,426 tests) also pass locally.

Are there any user-facing changes?

No. This is a behavior-preserving refactor with no public API changes.

@github-actions github-actions Bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Nov 3, 2026

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayzhan211
jayzhan211 enabled auto-merge November 3, 2026 12:12
@jayzhan211
jayzhan211 added this pull request to the merge queue Nov 3, 2026
Merged via the queue into apache:main with commit 8b88a86 Nov 3, 2026
44 checks passed
@alamb

alamb commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions optimizer Optimizer rules v57.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid unnecessary clones in logical planning and expression simplification

3 participants