Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ci/scripts/rust_clippy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,7 @@

set -ex
cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests,extended_tests -- -D warnings

# Update packages incrementally for stricter Clippy checks
# TODO: add tracking issue for the remaining workspace packages like `datafusion-catalog`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be better to add at the module level (rather than in the CI script) so that it would also be flagged locally when people ran clippy.

Similar to this:

#![deny(clippy::clone_on_ref_ptr)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the CI script, and update the datafusion/common/cargo.toml instead in 2736b39, since we can update the whole datafusion-common crate at once.

When updating a larger crate, I think it's possible to configure like this to update module by module.

cargo clippy -p datafusion-common --all-targets --features avro,pyarrow,parquet_encryption,sql -- -D warnings -W clippy::needless_pass_by_value
1 change: 1 addition & 0 deletions datafusion/common/src/column.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ mod tests {
use arrow::datatypes::{DataType, SchemaBuilder};
use std::sync::Arc;

#[allow(clippy::needless_pass_by_value)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer using

Suggested change
#[allow(clippy::needless_pass_by_value)]
#[expect(clippy::needless_pass_by_value)]

Which will error if the lint is no longer needed

allow will sit there silently even when the the lint is fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in dac1004

fn create_qualified_schema(qualifier: &str, names: Vec<&str>) -> Result<DFSchema> {
Copy link
Contributor

Choose a reason for hiding this comment

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

we could change this to impl IntoIterator<Item = &str> for example

Suggested change
fn create_qualified_schema(qualifier: &str, names: Vec<&str>) -> Result<DFSchema> {
fn create_qualified_schema(qualifier: &str, names: impl IntoIterator<Item = &str>) -> Result<DFSchema> {

let mut schema_builder = SchemaBuilder::new();
schema_builder.extend(
Expand Down
1 change: 1 addition & 0 deletions datafusion/common/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,7 @@ mod test {
Err(ArrowError::SchemaError("bar".to_string()).into())
}

#[allow(clippy::needless_pass_by_value)]
fn do_root_test(e: DataFusionError, exp: DataFusionError) {
let e = e.find_root();

Expand Down
1 change: 1 addition & 0 deletions datafusion/common/src/file_options/parquet_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ mod tests {
}
}

#[allow(clippy::needless_pass_by_value)]
fn extract_column_options(
props: &WriterProperties,
col: ColumnPath,
Expand Down
1 change: 1 addition & 0 deletions datafusion/common/src/hash_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ fn hash_array_primitive<T>(
/// If `rehash==true` this combines the previous hash value in the buffer
/// with the new hash using `combine_hashes`
#[cfg(not(feature = "force_hash_collisions"))]
#[allow(clippy::needless_pass_by_value)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? What is clippy's alternate suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I am wondering if this has found a good potential for optimization...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The arg array can safely be taken by reference.

However, I believe all current call sites already pass it by moving, so they’re fine.

That said, there might be optimization opportunities elsewhere — this needless_pass_by_value warning could indicate that some callers are cloning unnecessarily just to make the rust compiler happy.

fn hash_array<T>(
array: T,
random_state: &RandomState,
Expand Down
4 changes: 4 additions & 0 deletions datafusion/common/src/scalar/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4772,6 +4772,7 @@ impl fmt::Display for ScalarValue {
}
}

#[allow(clippy::needless_pass_by_value)]
fn fmt_list(arr: ArrayRef, f: &mut fmt::Formatter) -> fmt::Result {
// ScalarValue List, LargeList, FixedSizeList should always have a single element
assert_eq!(arr.len(), 1);
Expand Down Expand Up @@ -5568,6 +5569,7 @@ mod tests {
}

// Verifies that ScalarValue has the same behavior with compute kernel when it overflows.
#[allow(clippy::needless_pass_by_value)]
fn check_scalar_add_overflow<T>(left: ScalarValue, right: ScalarValue)
where
T: ArrowNumericType,
Expand Down Expand Up @@ -7097,6 +7099,7 @@ mod tests {
/// 1. convert to a `ScalarValue`
/// 2. Convert `ScalarValue` back to an `ArrayRef`
/// 3. Compare the original array (sliced) and new array for equality
#[allow(clippy::needless_pass_by_value)]
fn round_trip_through_scalar(arr: ArrayRef) {
for i in 0..arr.len() {
// convert Scalar --> Array
Expand Down Expand Up @@ -7569,6 +7572,7 @@ mod tests {
}

// mimics how casting work on scalar values by `casting` `scalar` to `desired_type`
#[allow(clippy::needless_pass_by_value)]
fn check_scalar_cast(scalar: ScalarValue, desired_type: DataType) {
// convert from scalar --> Array to call cast
let scalar_array = scalar.to_array().expect("Failed to convert to array");
Expand Down
1 change: 1 addition & 0 deletions datafusion/common/src/scalar/struct_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ impl ScalarStructBuilder {
}

/// Add the specified field and `ScalarValue` to the struct.
#[allow(clippy::needless_pass_by_value)]
pub fn with_scalar(self, field: impl IntoFieldRef, value: ScalarValue) -> Self {
// valid scalar value should not fail
let array = value.to_array().unwrap();
Expand Down