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

Enable clone_on_ref_ptrclippy lint on some crates #11157

Closed
wants to merge 8 commits into from

Conversation

lewiszlw
Copy link
Member

@lewiszlw lewiszlw commented Jun 28, 2024

Which issue does this PR close?

part of #11143.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions labels Jun 28, 2024
@lewiszlw lewiszlw marked this pull request as draft June 28, 2024 04:01
@github-actions github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels Jun 28, 2024
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) substrait labels Jul 2, 2024
@lewiszlw lewiszlw marked this pull request as ready for review July 2, 2024 06:01
@lewiszlw lewiszlw changed the title Enable clone_on_ref_ptr lint Enable clone_on_ref_ptr lint on some crates Jul 2, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @lewiszlw This is epic 🤖 🦾

I loaded all the diffs and skimmed them and they look good to me ❤️

I think this change makes the code more verbose, but explicit about where clones are doing a deep copy and where they are not

Also since this does not change any public APIs I think it is 👌 -- thank you for doing this

This might cause issues with people who have forked the code, but I don't see any way around that (other than of course having them contribute their changes back up stream)

cc @ozankabak

datafusion/common-runtime/src/lib.rs Show resolved Hide resolved
@@ -14,6 +14,7 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
#![deny(clippy::clone_on_ref_ptr)]
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise here

@alamb alamb changed the title Enable clone_on_ref_ptr lint on some crates Enable clone_on_ref_ptrclippy lint on some crates Jul 2, 2024
@ozankabak
Copy link
Contributor

ozankabak commented Jul 2, 2024

This is a great change. However, instead of doing this in such a big PR, can we open an epic and do it in multiple small PRs one crate at a time? If possible, doing that will be much more friendly to fork owners.

@alamb
Copy link
Contributor

alamb commented Jul 3, 2024

I think this one is broken into several smaller PRs like

#11238
#11240
#11241

So closing this PR to try and keep the review queue down. Please let me know if that wasn't correct @lewiszlw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants