From 665c453bc4b1fd5a0867c069e587548e4de3ca7e Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Mon, 3 Nov 2025 17:54:07 +0800 Subject: [PATCH 1/6] add `clippy::needless_pass_by_value` to datafusion-common --- ci/scripts/rust_clippy.sh | 4 ++++ datafusion/common/src/column.rs | 1 + datafusion/common/src/error.rs | 1 + datafusion/common/src/file_options/parquet_writer.rs | 1 + datafusion/common/src/hash_utils.rs | 1 + datafusion/common/src/scalar/mod.rs | 4 ++++ datafusion/common/src/scalar/struct_builder.rs | 1 + 7 files changed, 13 insertions(+) diff --git a/ci/scripts/rust_clippy.sh b/ci/scripts/rust_clippy.sh index 1557bd56eab4..a9bf69698a1f 100755 --- a/ci/scripts/rust_clippy.sh +++ b/ci/scripts/rust_clippy.sh @@ -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` +cargo clippy -p datafusion-common --all-targets --features avro,pyarrow,parquet_encryption,sql -- -D warnings -W clippy::needless_pass_by_value diff --git a/datafusion/common/src/column.rs b/datafusion/common/src/column.rs index c7f0b5a4f488..e05f1986fd74 100644 --- a/datafusion/common/src/column.rs +++ b/datafusion/common/src/column.rs @@ -382,6 +382,7 @@ mod tests { use arrow::datatypes::{DataType, SchemaBuilder}; use std::sync::Arc; + #[allow(clippy::needless_pass_by_value)] fn create_qualified_schema(qualifier: &str, names: Vec<&str>) -> Result { let mut schema_builder = SchemaBuilder::new(); schema_builder.extend( diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index fde52944d049..0b9f231206e8 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -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(); diff --git a/datafusion/common/src/file_options/parquet_writer.rs b/datafusion/common/src/file_options/parquet_writer.rs index 564929c61bab..f7d6039a85dd 100644 --- a/datafusion/common/src/file_options/parquet_writer.rs +++ b/datafusion/common/src/file_options/parquet_writer.rs @@ -478,6 +478,7 @@ mod tests { } } + #[allow(clippy::needless_pass_by_value)] fn extract_column_options( props: &WriterProperties, col: ColumnPath, diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index 4b18351f708b..6effcf537d06 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -140,6 +140,7 @@ fn hash_array_primitive( /// 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)] fn hash_array( array: T, random_state: &RandomState, diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index 188a169a3dd2..d730ecc4b109 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -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); @@ -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(left: ScalarValue, right: ScalarValue) where T: ArrowNumericType, @@ -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 @@ -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"); diff --git a/datafusion/common/src/scalar/struct_builder.rs b/datafusion/common/src/scalar/struct_builder.rs index 56daee904514..75b8af3ab025 100644 --- a/datafusion/common/src/scalar/struct_builder.rs +++ b/datafusion/common/src/scalar/struct_builder.rs @@ -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(); From 2736b392ab7e8f1742a4ef882e5e39c8b8cf239a Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Tue, 4 Nov 2025 17:45:21 +0800 Subject: [PATCH 2/6] review: fix clippy error instead of suppressing --- ci/scripts/rust_clippy.sh | 6 +----- datafusion/common/Cargo.toml | 8 ++++++-- datafusion/common/src/hash_utils.rs | 19 +++++++++---------- datafusion/common/src/scalar/mod.rs | 18 ++++++++---------- .../common/src/scalar/struct_builder.rs | 5 ++--- .../tests/cases/roundtrip_logical_plan.rs | 8 ++++---- .../src/logical_plan/consumer/expr/literal.rs | 10 +++++++--- .../src/logical_plan/producer/expr/literal.rs | 6 +++--- 8 files changed, 40 insertions(+), 40 deletions(-) diff --git a/ci/scripts/rust_clippy.sh b/ci/scripts/rust_clippy.sh index a9bf69698a1f..6a00ad810956 100755 --- a/ci/scripts/rust_clippy.sh +++ b/ci/scripts/rust_clippy.sh @@ -18,8 +18,4 @@ # under the License. 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` -cargo clippy -p datafusion-common --all-targets --features avro,pyarrow,parquet_encryption,sql -- -D warnings -W clippy::needless_pass_by_value +cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests,extended_tests -- -D warnings \ No newline at end of file diff --git a/datafusion/common/Cargo.toml b/datafusion/common/Cargo.toml index abeb4e66a269..7625c1c02a6b 100644 --- a/datafusion/common/Cargo.toml +++ b/datafusion/common/Cargo.toml @@ -31,8 +31,12 @@ rust-version = { workspace = true } [package.metadata.docs.rs] all-features = true -[lints] -workspace = true +[lints] # Overrides / extends workspace.lints +[lints.clippy] +# Avoid unnecessary clones for performance, okay to ignore for tests or intentional +# moves. +# Reference: +needless_pass_by_value = "deny" [lib] name = "datafusion_common" diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index 6effcf537d06..b4488c770d8d 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -140,9 +140,8 @@ fn hash_array_primitive( /// 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)] fn hash_array( - array: T, + array: &T, random_state: &RandomState, hashes_buffer: &mut [u64], rehash: bool, @@ -401,16 +400,16 @@ pub fn create_hashes<'a>( downcast_primitive_array! { array => hash_array_primitive(array, random_state, hashes_buffer, rehash), DataType::Null => hash_null(random_state, hashes_buffer, rehash), - DataType::Boolean => hash_array(as_boolean_array(array)?, random_state, hashes_buffer, rehash), - DataType::Utf8 => hash_array(as_string_array(array)?, random_state, hashes_buffer, rehash), - DataType::Utf8View => hash_array(as_string_view_array(array)?, random_state, hashes_buffer, rehash), - DataType::LargeUtf8 => hash_array(as_largestring_array(array), random_state, hashes_buffer, rehash), - DataType::Binary => hash_array(as_generic_binary_array::(array)?, random_state, hashes_buffer, rehash), - DataType::BinaryView => hash_array(as_binary_view_array(array)?, random_state, hashes_buffer, rehash), - DataType::LargeBinary => hash_array(as_generic_binary_array::(array)?, random_state, hashes_buffer, rehash), + DataType::Boolean => hash_array(&as_boolean_array(array)?, random_state, hashes_buffer, rehash), + DataType::Utf8 => hash_array(&as_string_array(array)?, random_state, hashes_buffer, rehash), + DataType::Utf8View => hash_array(&as_string_view_array(array)?, random_state, hashes_buffer, rehash), + DataType::LargeUtf8 => hash_array(&as_largestring_array(array), random_state, hashes_buffer, rehash), + DataType::Binary => hash_array(&as_generic_binary_array::(array)?, random_state, hashes_buffer, rehash), + DataType::BinaryView => hash_array(&as_binary_view_array(array)?, random_state, hashes_buffer, rehash), + DataType::LargeBinary => hash_array(&as_generic_binary_array::(array)?, random_state, hashes_buffer, rehash), DataType::FixedSizeBinary(_) => { let array: &FixedSizeBinaryArray = array.as_any().downcast_ref().unwrap(); - hash_array(array, random_state, hashes_buffer, rehash) + hash_array(&array, random_state, hashes_buffer, rehash) } DataType::Dictionary(_, _) => downcast_dictionary_array! { array => hash_dictionary(array, random_state, hashes_buffer, rehash)?, diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index d730ecc4b109..b52a02465425 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -4436,7 +4436,7 @@ impl From> for ScalarValue { value .into_iter() .fold(ScalarStructBuilder::new(), |builder, (name, value)| { - builder.with_name_and_scalar(name, value) + builder.with_name_and_scalar(name, &value) }) .build() .unwrap() @@ -4648,9 +4648,9 @@ impl fmt::Display for ScalarValue { } None => write!(f, "NULL")?, }, - ScalarValue::List(arr) => fmt_list(arr.to_owned() as ArrayRef, f)?, - ScalarValue::LargeList(arr) => fmt_list(arr.to_owned() as ArrayRef, f)?, - ScalarValue::FixedSizeList(arr) => fmt_list(arr.to_owned() as ArrayRef, f)?, + ScalarValue::List(arr) => fmt_list(arr.as_ref(), f)?, + ScalarValue::LargeList(arr) => fmt_list(arr.as_ref(), f)?, + ScalarValue::FixedSizeList(arr) => fmt_list(arr.as_ref(), f)?, ScalarValue::Date32(e) => format_option!( f, e.map(|v| { @@ -4772,13 +4772,11 @@ impl fmt::Display for ScalarValue { } } -#[allow(clippy::needless_pass_by_value)] -fn fmt_list(arr: ArrayRef, f: &mut fmt::Formatter) -> fmt::Result { +fn fmt_list(arr: &dyn Array, f: &mut fmt::Formatter) -> fmt::Result { // ScalarValue List, LargeList, FixedSizeList should always have a single element assert_eq!(arr.len(), 1); let options = FormatOptions::default().with_display_error(true); - let formatter = - ArrayFormatter::try_new(arr.as_ref() as &dyn Array, &options).unwrap(); + let formatter = ArrayFormatter::try_new(arr, &options).unwrap(); let value_formatter = formatter.value(0); write!(f, "{value_formatter}") } @@ -8238,8 +8236,8 @@ mod tests { let field_b = Field::new("b", DataType::Utf8, true); let s = ScalarStructBuilder::new() - .with_scalar(field_a, ScalarValue::from(1i32)) - .with_scalar(field_b, ScalarValue::Utf8(None)) + .with_scalar(field_a, &ScalarValue::from(1i32)) + .with_scalar(field_b, &ScalarValue::Utf8(None)) .build() .unwrap(); diff --git a/datafusion/common/src/scalar/struct_builder.rs b/datafusion/common/src/scalar/struct_builder.rs index 75b8af3ab025..026540c342e0 100644 --- a/datafusion/common/src/scalar/struct_builder.rs +++ b/datafusion/common/src/scalar/struct_builder.rs @@ -83,8 +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 { + pub fn with_scalar(self, field: impl IntoFieldRef, value: &ScalarValue) -> Self { // valid scalar value should not fail let array = value.to_array().unwrap(); self.with_array(field, array) @@ -92,7 +91,7 @@ impl ScalarStructBuilder { /// Add a field with the specified name and value to the struct. /// the field is created with the specified data type and as non nullable - pub fn with_name_and_scalar(self, name: &str, value: ScalarValue) -> Self { + pub fn with_name_and_scalar(self, name: &str, value: &ScalarValue) -> Self { let field = Field::new(name, value.data_type(), false); self.with_scalar(field, value) } diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs index bfd693e6a0f8..cdc167a3acfc 100644 --- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs @@ -1533,22 +1533,22 @@ fn round_trip_scalar_values_and_data_types() { ScalarStructBuilder::new() .with_scalar( Field::new("a", DataType::Int32, true), - ScalarValue::from(23i32), + &ScalarValue::from(23i32), ) .with_scalar( Field::new("b", DataType::Boolean, false), - ScalarValue::from(false), + &ScalarValue::from(false), ) .build() .unwrap(), ScalarStructBuilder::new() .with_scalar( Field::new("a", DataType::Int32, true), - ScalarValue::from(23i32), + &ScalarValue::from(23i32), ) .with_scalar( Field::new("b", DataType::Boolean, false), - ScalarValue::from(false), + &ScalarValue::from(false), ) .build() .unwrap(), diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs index eb3d345dc001..1a53096719e7 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs @@ -304,10 +304,13 @@ pub(crate) fn from_substrait_literal( &mut entry_name_idx, )?; ScalarStructBuilder::new() - .with_scalar(Field::new("key", key_sv.data_type(), false), key_sv) + .with_scalar( + Field::new("key", key_sv.data_type(), false), + &key_sv, + ) .with_scalar( Field::new("value", value_sv.data_type(), true), - value_sv, + &value_sv, ) .build() }) @@ -367,7 +370,8 @@ pub(crate) fn from_substrait_literal( let sv = from_substrait_literal(consumer, field, dfs_names, name_idx)?; // We assume everything to be nullable, since Arrow's strict about things matching // and it's hard to match otherwise. - builder = builder.with_scalar(Field::new(name, sv.data_type(), true), sv); + builder = + builder.with_scalar(Field::new(name, sv.data_type(), true), &sv); } builder.build()? } diff --git a/datafusion/substrait/src/logical_plan/producer/expr/literal.rs b/datafusion/substrait/src/logical_plan/producer/expr/literal.rs index 1bb24168e57a..c1bf694693f7 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/literal.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/literal.rs @@ -530,9 +530,9 @@ mod tests { let c2 = Field::new("c2", DataType::Utf8, true); round_trip_literal( ScalarStructBuilder::new() - .with_scalar(c0.to_owned(), ScalarValue::Boolean(Some(true))) - .with_scalar(c1.to_owned(), ScalarValue::Int32(Some(1))) - .with_scalar(c2.to_owned(), ScalarValue::Utf8(None)) + .with_scalar(c0.to_owned(), &ScalarValue::Boolean(Some(true))) + .with_scalar(c1.to_owned(), &ScalarValue::Int32(Some(1))) + .with_scalar(c2.to_owned(), &ScalarValue::Utf8(None)) .build()?, )?; round_trip_literal(ScalarStructBuilder::new_null(vec![c0, c1, c2]))?; From f2811e93087afbb331e44038247aa11b67cbd81e Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Tue, 4 Nov 2025 18:00:10 +0800 Subject: [PATCH 3/6] revert public interface changes --- datafusion/common/src/scalar/mod.rs | 6 +++--- datafusion/common/src/scalar/struct_builder.rs | 5 +++-- datafusion/proto/tests/cases/roundtrip_logical_plan.rs | 8 ++++---- .../src/logical_plan/consumer/expr/literal.rs | 10 +++------- .../src/logical_plan/producer/expr/literal.rs | 6 +++--- 5 files changed, 16 insertions(+), 19 deletions(-) diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index b52a02465425..87a82a70267d 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -4436,7 +4436,7 @@ impl From> for ScalarValue { value .into_iter() .fold(ScalarStructBuilder::new(), |builder, (name, value)| { - builder.with_name_and_scalar(name, &value) + builder.with_name_and_scalar(name, value) }) .build() .unwrap() @@ -8236,8 +8236,8 @@ mod tests { let field_b = Field::new("b", DataType::Utf8, true); let s = ScalarStructBuilder::new() - .with_scalar(field_a, &ScalarValue::from(1i32)) - .with_scalar(field_b, &ScalarValue::Utf8(None)) + .with_scalar(field_a, ScalarValue::from(1i32)) + .with_scalar(field_b, ScalarValue::Utf8(None)) .build() .unwrap(); diff --git a/datafusion/common/src/scalar/struct_builder.rs b/datafusion/common/src/scalar/struct_builder.rs index 026540c342e0..75b8af3ab025 100644 --- a/datafusion/common/src/scalar/struct_builder.rs +++ b/datafusion/common/src/scalar/struct_builder.rs @@ -83,7 +83,8 @@ impl ScalarStructBuilder { } /// Add the specified field and `ScalarValue` to the struct. - pub fn with_scalar(self, field: impl IntoFieldRef, value: &ScalarValue) -> Self { + #[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(); self.with_array(field, array) @@ -91,7 +92,7 @@ impl ScalarStructBuilder { /// Add a field with the specified name and value to the struct. /// the field is created with the specified data type and as non nullable - pub fn with_name_and_scalar(self, name: &str, value: &ScalarValue) -> Self { + pub fn with_name_and_scalar(self, name: &str, value: ScalarValue) -> Self { let field = Field::new(name, value.data_type(), false); self.with_scalar(field, value) } diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs index cdc167a3acfc..bfd693e6a0f8 100644 --- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs @@ -1533,22 +1533,22 @@ fn round_trip_scalar_values_and_data_types() { ScalarStructBuilder::new() .with_scalar( Field::new("a", DataType::Int32, true), - &ScalarValue::from(23i32), + ScalarValue::from(23i32), ) .with_scalar( Field::new("b", DataType::Boolean, false), - &ScalarValue::from(false), + ScalarValue::from(false), ) .build() .unwrap(), ScalarStructBuilder::new() .with_scalar( Field::new("a", DataType::Int32, true), - &ScalarValue::from(23i32), + ScalarValue::from(23i32), ) .with_scalar( Field::new("b", DataType::Boolean, false), - &ScalarValue::from(false), + ScalarValue::from(false), ) .build() .unwrap(), diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs index 1a53096719e7..eb3d345dc001 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs @@ -304,13 +304,10 @@ pub(crate) fn from_substrait_literal( &mut entry_name_idx, )?; ScalarStructBuilder::new() - .with_scalar( - Field::new("key", key_sv.data_type(), false), - &key_sv, - ) + .with_scalar(Field::new("key", key_sv.data_type(), false), key_sv) .with_scalar( Field::new("value", value_sv.data_type(), true), - &value_sv, + value_sv, ) .build() }) @@ -370,8 +367,7 @@ pub(crate) fn from_substrait_literal( let sv = from_substrait_literal(consumer, field, dfs_names, name_idx)?; // We assume everything to be nullable, since Arrow's strict about things matching // and it's hard to match otherwise. - builder = - builder.with_scalar(Field::new(name, sv.data_type(), true), &sv); + builder = builder.with_scalar(Field::new(name, sv.data_type(), true), sv); } builder.build()? } diff --git a/datafusion/substrait/src/logical_plan/producer/expr/literal.rs b/datafusion/substrait/src/logical_plan/producer/expr/literal.rs index c1bf694693f7..1bb24168e57a 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/literal.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/literal.rs @@ -530,9 +530,9 @@ mod tests { let c2 = Field::new("c2", DataType::Utf8, true); round_trip_literal( ScalarStructBuilder::new() - .with_scalar(c0.to_owned(), &ScalarValue::Boolean(Some(true))) - .with_scalar(c1.to_owned(), &ScalarValue::Int32(Some(1))) - .with_scalar(c2.to_owned(), &ScalarValue::Utf8(None)) + .with_scalar(c0.to_owned(), ScalarValue::Boolean(Some(true))) + .with_scalar(c1.to_owned(), ScalarValue::Int32(Some(1))) + .with_scalar(c2.to_owned(), ScalarValue::Utf8(None)) .build()?, )?; round_trip_literal(ScalarStructBuilder::new_null(vec![c0, c1, c2]))?; From dac10047c91d64cdc101230774ee6ccc498bf214 Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Thu, 6 Nov 2025 08:55:09 +0800 Subject: [PATCH 4/6] review: use expect + move existing rule from lib.rs -> clippytoml --- datafusion/common/Cargo.toml | 3 +++ datafusion/common/src/column.rs | 2 +- datafusion/common/src/error.rs | 2 +- datafusion/common/src/file_options/parquet_writer.rs | 2 +- datafusion/common/src/lib.rs | 3 --- datafusion/common/src/scalar/mod.rs | 6 +++--- datafusion/common/src/scalar/struct_builder.rs | 2 +- datafusion/expr/src/logical_plan/plan.rs | 4 ++-- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/datafusion/common/Cargo.toml b/datafusion/common/Cargo.toml index 7625c1c02a6b..91519857b346 100644 --- a/datafusion/common/Cargo.toml +++ b/datafusion/common/Cargo.toml @@ -37,6 +37,9 @@ all-features = true # moves. # Reference: needless_pass_by_value = "deny" +# Make sure fast / cheap clones on Arc are explicit: +# https://github.com/apache/datafusion/issues/11143 +clone_on_ref_ptr = "deny" [lib] name = "datafusion_common" diff --git a/datafusion/common/src/column.rs b/datafusion/common/src/column.rs index e05f1986fd74..b1bca436ae6d 100644 --- a/datafusion/common/src/column.rs +++ b/datafusion/common/src/column.rs @@ -382,7 +382,7 @@ mod tests { use arrow::datatypes::{DataType, SchemaBuilder}; use std::sync::Arc; - #[allow(clippy::needless_pass_by_value)] + #[expect(clippy::needless_pass_by_value)] fn create_qualified_schema(qualifier: &str, names: Vec<&str>) -> Result { let mut schema_builder = SchemaBuilder::new(); schema_builder.extend( diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index 0b9f231206e8..63c13c24a8be 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -1185,7 +1185,7 @@ mod test { Err(ArrowError::SchemaError("bar".to_string()).into()) } - #[allow(clippy::needless_pass_by_value)] + #[expect(clippy::needless_pass_by_value)] fn do_root_test(e: DataFusionError, exp: DataFusionError) { let e = e.find_root(); diff --git a/datafusion/common/src/file_options/parquet_writer.rs b/datafusion/common/src/file_options/parquet_writer.rs index f7d6039a85dd..ce12d679a5a0 100644 --- a/datafusion/common/src/file_options/parquet_writer.rs +++ b/datafusion/common/src/file_options/parquet_writer.rs @@ -478,7 +478,7 @@ mod tests { } } - #[allow(clippy::needless_pass_by_value)] + #[expect(clippy::needless_pass_by_value)] fn extract_column_options( props: &WriterProperties, col: ColumnPath, diff --git a/datafusion/common/src/lib.rs b/datafusion/common/src/lib.rs index 76c7b46e3273..9cad2783f09b 100644 --- a/datafusion/common/src/lib.rs +++ b/datafusion/common/src/lib.rs @@ -20,9 +20,6 @@ html_favicon_url = "https://raw.githubusercontent.com/apache/datafusion/19fe44cf2f30cbdd63d4a4f52c74055163c6cc38/docs/logos/standalone_logo/logo_original.svg" )] #![cfg_attr(docsrs, feature(doc_cfg))] -// Make sure fast / cheap clones on Arc are explicit: -// https://github.com/apache/datafusion/issues/11143 -#![deny(clippy::clone_on_ref_ptr)] mod column; mod dfschema; diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index 87a82a70267d..39c518488e07 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -5567,7 +5567,7 @@ mod tests { } // Verifies that ScalarValue has the same behavior with compute kernel when it overflows. - #[allow(clippy::needless_pass_by_value)] + #[expect(clippy::needless_pass_by_value)] fn check_scalar_add_overflow(left: ScalarValue, right: ScalarValue) where T: ArrowNumericType, @@ -7097,7 +7097,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)] + #[expect(clippy::needless_pass_by_value)] fn round_trip_through_scalar(arr: ArrayRef) { for i in 0..arr.len() { // convert Scalar --> Array @@ -7570,7 +7570,7 @@ mod tests { } // mimics how casting work on scalar values by `casting` `scalar` to `desired_type` - #[allow(clippy::needless_pass_by_value)] + #[expect(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"); diff --git a/datafusion/common/src/scalar/struct_builder.rs b/datafusion/common/src/scalar/struct_builder.rs index 75b8af3ab025..3ff1b152f787 100644 --- a/datafusion/common/src/scalar/struct_builder.rs +++ b/datafusion/common/src/scalar/struct_builder.rs @@ -83,7 +83,7 @@ impl ScalarStructBuilder { } /// Add the specified field and `ScalarValue` to the struct. - #[allow(clippy::needless_pass_by_value)] + #[expect(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(); diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index 0f0d81186d68..0b89a5250902 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -1156,7 +1156,7 @@ impl LogicalPlan { /// Helper for [Self::with_new_exprs] to use when no expressions are expected. #[inline] - #[allow(clippy::needless_pass_by_value)] // expr is moved intentionally to ensure it's not used again + #[expect(clippy::needless_pass_by_value)] // expr is moved intentionally to ensure it's not used again fn assert_no_expressions(&self, expr: Vec) -> Result<()> { if !expr.is_empty() { return internal_err!("{self:?} should have no exprs, got {:?}", expr); @@ -1166,7 +1166,7 @@ impl LogicalPlan { /// Helper for [Self::with_new_exprs] to use when no inputs are expected. #[inline] - #[allow(clippy::needless_pass_by_value)] // inputs is moved intentionally to ensure it's not used again + #[expect(clippy::needless_pass_by_value)] // inputs is moved intentionally to ensure it's not used again fn assert_no_inputs(&self, inputs: Vec) -> Result<()> { if !inputs.is_empty() { return internal_err!("{self:?} should have no inputs, got: {:?}", inputs); From c66bee82704451ddfc34b96f46e8e8ee8eedd418 Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Thu, 6 Nov 2025 10:13:00 +0800 Subject: [PATCH 5/6] disable for test modules using `cfg_attr` --- datafusion/common/src/column.rs | 1 - datafusion/common/src/error.rs | 1 - datafusion/common/src/file_options/parquet_writer.rs | 1 - datafusion/common/src/lib.rs | 3 +++ datafusion/common/src/scalar/mod.rs | 3 --- datafusion/common/src/scalar/struct_builder.rs | 2 +- 6 files changed, 4 insertions(+), 7 deletions(-) diff --git a/datafusion/common/src/column.rs b/datafusion/common/src/column.rs index b1bca436ae6d..c7f0b5a4f488 100644 --- a/datafusion/common/src/column.rs +++ b/datafusion/common/src/column.rs @@ -382,7 +382,6 @@ mod tests { use arrow::datatypes::{DataType, SchemaBuilder}; use std::sync::Arc; - #[expect(clippy::needless_pass_by_value)] fn create_qualified_schema(qualifier: &str, names: Vec<&str>) -> Result { let mut schema_builder = SchemaBuilder::new(); schema_builder.extend( diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index 63c13c24a8be..fde52944d049 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -1185,7 +1185,6 @@ mod test { Err(ArrowError::SchemaError("bar".to_string()).into()) } - #[expect(clippy::needless_pass_by_value)] fn do_root_test(e: DataFusionError, exp: DataFusionError) { let e = e.find_root(); diff --git a/datafusion/common/src/file_options/parquet_writer.rs b/datafusion/common/src/file_options/parquet_writer.rs index ce12d679a5a0..564929c61bab 100644 --- a/datafusion/common/src/file_options/parquet_writer.rs +++ b/datafusion/common/src/file_options/parquet_writer.rs @@ -478,7 +478,6 @@ mod tests { } } - #[expect(clippy::needless_pass_by_value)] fn extract_column_options( props: &WriterProperties, col: ColumnPath, diff --git a/datafusion/common/src/lib.rs b/datafusion/common/src/lib.rs index 9cad2783f09b..ce888165081b 100644 --- a/datafusion/common/src/lib.rs +++ b/datafusion/common/src/lib.rs @@ -20,6 +20,9 @@ html_favicon_url = "https://raw.githubusercontent.com/apache/datafusion/19fe44cf2f30cbdd63d4a4f52c74055163c6cc38/docs/logos/standalone_logo/logo_original.svg" )] #![cfg_attr(docsrs, feature(doc_cfg))] +// This lint rule is enforced in `../Cargo.toml`, but it's okay to skip them in tests +// See details in https://github.com/apache/datafusion/issues/18503 +#![cfg_attr(test, allow(clippy::needless_pass_by_value))] mod column; mod dfschema; diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index 39c518488e07..f2e18f7de8f5 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -5567,7 +5567,6 @@ mod tests { } // Verifies that ScalarValue has the same behavior with compute kernel when it overflows. - #[expect(clippy::needless_pass_by_value)] fn check_scalar_add_overflow(left: ScalarValue, right: ScalarValue) where T: ArrowNumericType, @@ -7097,7 +7096,6 @@ 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 - #[expect(clippy::needless_pass_by_value)] fn round_trip_through_scalar(arr: ArrayRef) { for i in 0..arr.len() { // convert Scalar --> Array @@ -7570,7 +7568,6 @@ mod tests { } // mimics how casting work on scalar values by `casting` `scalar` to `desired_type` - #[expect(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"); diff --git a/datafusion/common/src/scalar/struct_builder.rs b/datafusion/common/src/scalar/struct_builder.rs index 3ff1b152f787..045b5778243d 100644 --- a/datafusion/common/src/scalar/struct_builder.rs +++ b/datafusion/common/src/scalar/struct_builder.rs @@ -83,7 +83,7 @@ impl ScalarStructBuilder { } /// Add the specified field and `ScalarValue` to the struct. - #[expect(clippy::needless_pass_by_value)] + #[expect(clippy::needless_pass_by_value)] // Skip for public API's compatibility pub fn with_scalar(self, field: impl IntoFieldRef, value: ScalarValue) -> Self { // valid scalar value should not fail let array = value.to_array().unwrap(); From f6c873ca2d9dc2c03b26211e378221c8c166b8c1 Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Fri, 7 Nov 2025 14:39:03 +0800 Subject: [PATCH 6/6] fix global lint inheritence --- datafusion/common/Cargo.toml | 11 ++--------- datafusion/common/src/lib.rs | 5 +++++ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/datafusion/common/Cargo.toml b/datafusion/common/Cargo.toml index 91519857b346..abeb4e66a269 100644 --- a/datafusion/common/Cargo.toml +++ b/datafusion/common/Cargo.toml @@ -31,15 +31,8 @@ rust-version = { workspace = true } [package.metadata.docs.rs] all-features = true -[lints] # Overrides / extends workspace.lints -[lints.clippy] -# Avoid unnecessary clones for performance, okay to ignore for tests or intentional -# moves. -# Reference: -needless_pass_by_value = "deny" -# Make sure fast / cheap clones on Arc are explicit: -# https://github.com/apache/datafusion/issues/11143 -clone_on_ref_ptr = "deny" +[lints] +workspace = true [lib] name = "datafusion_common" diff --git a/datafusion/common/src/lib.rs b/datafusion/common/src/lib.rs index ce888165081b..c8d5a30ee3e0 100644 --- a/datafusion/common/src/lib.rs +++ b/datafusion/common/src/lib.rs @@ -20,6 +20,11 @@ html_favicon_url = "https://raw.githubusercontent.com/apache/datafusion/19fe44cf2f30cbdd63d4a4f52c74055163c6cc38/docs/logos/standalone_logo/logo_original.svg" )] #![cfg_attr(docsrs, feature(doc_cfg))] +// Make sure fast / cheap clones on Arc are explicit: +// https://github.com/apache/datafusion/issues/11143 +#![deny(clippy::clone_on_ref_ptr)] +// https://github.com/apache/datafusion/issues/18503 +#![deny(clippy::needless_pass_by_value)] // This lint rule is enforced in `../Cargo.toml`, but it's okay to skip them in tests // See details in https://github.com/apache/datafusion/issues/18503 #![cfg_attr(test, allow(clippy::needless_pass_by_value))]