Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Mar 17, 2025

Which issue does this PR close?

Rationale for this change

Some of the newer DataFusion crates don't have the clippy::clone_on_ref_ptr lint enabled. This lint is present to make it clear when clone() is very cheap (because it is on an Arc rather than some structure that requires deep copying

What changes are included in this PR?

  1. Enable the lint in other crates
  2. Update a few places to get it to pass

Here is an example of the lint enabled in the core crate

#![cfg_attr(not(test), deny(clippy::clone_on_ref_ptr))]

Are these changes tested?

By CI

Are there any user-facing changes?

No

@alamb alamb added development-process Related to development process of DataFusion and removed development-process Related to development process of DataFusion labels Mar 17, 2025
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate catalog Related to the catalog crate datasource Changes to the datasource crate labels Mar 17, 2025
state.aggregate_functions().clone(),
state.window_functions().clone(),
state.runtime_env().clone(),
Arc::clone(state.runtime_env()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this shows what the lint disallows -- when it is an Arc it must use Arc::clone rather than .clone() so it is easier to see when reading the code that no copies are made

Copy link
Contributor

Choose a reason for hiding this comment

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

I always wondered what the motivation of the lint is 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point -- I'll add some comments explaining it

@alamb alamb marked this pull request as ready for review March 17, 2025 20:34
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @alamb not sure why CI failed though

@alamb
Copy link
Contributor Author

alamb commented Mar 18, 2025

lgtm thanks @alamb not sure why CI failed though

Thanks @comphead -- I am not sure either -- I'll figure t out

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate common Related to common crate execution Related to the execution crate proto Related to proto crate functions Changes to functions implementation ffi Changes to the ffi crate labels Mar 18, 2025
@berkaysynnada berkaysynnada merged commit d2a395c into apache:main Mar 18, 2025
27 checks passed
@berkaysynnada
Copy link
Contributor

Thank you @alamb and @comphead

@alamb alamb deleted the alamb/add_arc_clone_lint branch March 18, 2025 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

catalog Related to the catalog crate common Related to common crate core Core DataFusion crate datasource Changes to the datasource crate execution Related to the execution crate ffi Changes to the ffi crate functions Changes to functions implementation logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates proto Related to proto crate sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants