Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: consistently apply clippy::clone_on_ref_ptr in all crates #15284

Merged
merged 4 commits into from
Mar 18, 2025

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
@@ -145,7 +145,7 @@ impl From<&dyn Session> for TaskContext {
state.scalar_functions().clone(),
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