Clippy lints by LouisGariepy · Pull Request #6464 · apache/datafusion

LouisGariepy · 2023-05-27T02:40:37Z

Which issue does this PR close?

Closes #207.

Rationale for this change

This PR fixes most of the remaining clippy allows remaining in the code base.

Clippy allows sometimes silence actually useful lints. Even if the lint was not useful at some point, it's easy to forget about them even after the code has been fixed.

For these reasons it's good practice to clean them up once in a while.

What changes are included in this PR?

The bulk of the changes is simply removing allows the were not needed at all.

Some changes are slightly more involved, but should still be pretty localized and self-explanatory.

Are these changes tested?

This PR does not (and should not) introduce new features or modify existing features. Thus, existing test coverage should be sufficient.

I added one test to for the code I added for data pointer equality (related to the removal of vtable address comparison lints

Are there any user-facing changes?

No(*) (see the Notes section below)

Notes

All of the committed fixes are fairly straightforward and uncontroversial.

Some fixes would be breaking(*) or require maintainer input to get right. I documented these cases with TODO(clippy) and gave ample details and potential solutions. I would appreciate if the reviewer could pay special attention to these so we can decide which ones should be fixed as part of this PR, which ones should be fixed as part of a separate PR, and which ones should not be fixed at all.

The remaining allows are all too many arguments lints whose fixes are not straightforward and might require some more in-depth redesign. I'm proposing to leave them as is for now and fix them on a case-by-case basis as solutions become more evident. I did not mark these with TODO(clippy) as I can't suggest solutions for them without some more careful examination. I will try to get to them in due time, in one or more future PRs.

LouisGariepy · 2023-05-27T02:50:46Z

Oh one thing I was unsure about:

I put the DataPtr trait in datafusion_common::utils. I'm not sure that's the right place for this utility. Please let me know 😃 .

LouisGariepy · 2023-05-27T06:24:55Z

Also, bear in mind that the line count includes several added detailed comments. I'm fairly certain that overall this PR removes lines of code or is very close to neutral.

alamb · 2023-05-28T12:26:50Z

Thank you @LouisGariepy -- I plan to review this carefully tomorrow or Tuesday if no one beats me to it

This fixes the doc build error in CI and seems more appropriate.

alamb

Thank you very much @LouisGariepy - I went through this PR carefully and I think it is a really nice cleanup and thus plan to merge it in.

I had some comments that might improve the code more, but I think this PR as it is is a great step forward and we can address the other items as follow on PRs.

Again, thank you for helping keep the codebase cleaner 🦾

alamb · 2023-05-29T11:29:07Z

+    // TODO(clippy): The clippy `allow` could be removed by renaming this method to `next_batch`.
+    // This would also make the intent of the method clearer.
+    //
+    // Another option could be to rework `AvroArrowArrayReader::next_batch` so it returns an


I think this second option would be my preference

alamb · 2023-05-29T11:29:56Z


        // "YES if the column is possibly nullable, NO if it is known not nullable. "
-        let nullable_str = if is_nullable { "YES" } else { "NO" };
+        let nullable_str = if field.is_nullable() { "YES" } else { "NO" };


this is a nice cleanup

alamb · 2023-05-29T11:30:18Z

 //! This module contains interfaces and default implementations
 //! of table namespacing concepts, including catalogs and schemas.

+// TODO(clippy): Having a `datasource::datasource` module path is unclear and ambiguous.


alamb · 2023-05-29T11:31:31Z

        {
-            let left_changed = Arc::ptr_eq(left, &right_exec);
-            let right_changed = Arc::ptr_eq(right, &left_exec);
+            let left_changed = Arc::data_ptr_eq(left, &right_exec);


I like the fact that you have moved the (potentially dubious) pointer comparisons into a trait that has much better documentation about rationale. 🙏 thank you

alamb · 2023-05-29T11:32:25Z

-                baseline_metrics,
-                context,
-                partition,
+                self, context, partition,


this certainly looks much nicer 😍

alamb · 2023-05-29T11:33:07Z

    };
    use std::sync::Arc;

+    struct PrimitiveTypeField<'a> {


alamb · 2023-05-29T11:33:56Z

    use std::sync::Arc;

+    struct PrimitiveTypeField<'a> {
+        name: &'a str,


I don't think it is important but given that name seems to always be a constant string, you could do something like

Suggested change

name: &'a str,

name: &'static str,

And avoid having to have the named lifetime 'a on this struct

alamb · 2023-05-29T11:34:23Z

-            None,
-        )]);
+        let schema_descr = get_test_schema_descr(vec![PrimitiveTypeField {
+            name: "c1",


I am a big fan of explicit field names as a way to better document what is going on here.

alamb · 2023-05-29T11:36:31Z

+            PrimitiveTypeField {
+                name: "c1",
+                physical_ty: PhysicalType::INT32,
+                logical_ty: None,
+                precision: None,
+                scale: None,
+                byte_len: None,
+            },


When I was reading this, I was thinking these tests might be even more readable if they had a builder style, something like

PrimtiveTypeField::new("c1", PhysicalType::INT32)

And then for tests that need other optional fields set:

PrimtiveTypeField::new("c1", PhysicalType::INT32) .with_logical_type(LogicalType::Decimal { scale: 0, precision: 9, });

alamb · 2023-05-29T11:37:35Z

+    // TODO(clippy): This should probably be renamed to use a `with_*` prefix. Something
+    // like `with_template`, or `with_exprs_and_inputs`.
+    //
+    // Also, I think `ExtensionPlanNode` has been renamed to `UserDefinedLogicalNode`


LouisGariepy · 2023-05-29T22:39:10Z

@alamb Thank you for the review 😃. I implemented your suggestions and fixed a typo that slipped in.

Just to be sure, regarding the TODO(clippy) comments, would you prefer those to be fixed in this PR, or in a separate one?

alamb · 2023-05-30T10:07:46Z

Just to be sure, regarding the TODO(clippy) comments, would you prefer those to be fixed in this PR, or in a separate one?

I think a separate one is better -- I don't see any reason to hold up the changes in this PR for technically unrelated other changes.

THanks again @LouisGariepy

alamb · 2023-05-30T10:08:08Z

-            physical_ty: PhysicalType::INT32,
-            logical_ty: Some(LogicalType::Decimal {
+
+        let field = PrimitiveTypeField::new("c1", PhysicalType::INT32)


LouisGariepy added 5 commits May 26, 2023 20:02

Remove unneeded clippy lints.

45f4fbc

Variable name fix for consistency.

92ace3b

Document DataPtr trait.

0559994

Minor documentation adjustment.

57962d1

Add test case.

3ffb970

github-actions Bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates substrait Changes to the substrait crate labels May 27, 2023

Downgrade doc comment to impl comment.

20c5ebd

This fixes the doc build error in CI and seems more appropriate.

alamb approved these changes May 29, 2023

View reviewed changes

LouisGariepy added 3 commits May 29, 2023 17:42

Fix typo.

f742b87

Implement builder.

35a7f8f

Replace generic lifetime with 'static lifetime.

c5df95e

alamb approved these changes May 30, 2023

View reviewed changes

alamb merged commit 0d9c542 into apache:main May 30, 2023

LouisGariepy mentioned this pull request Jun 3, 2023

Fix avro_to_arrow::reader::Reader::next clippy allow (should_implement_trait). #6537

Closed

Conversation

LouisGariepy commented May 27, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Notes

Uh oh!

LouisGariepy commented May 27, 2023

Uh oh!

LouisGariepy commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented May 28, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LouisGariepy commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented May 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LouisGariepy commented May 27, 2023 •

edited

Loading

LouisGariepy commented May 29, 2023 •

edited

Loading