Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
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
2 changes: 1 addition & 1 deletion ci/scripts/rust_clippy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@
# under the License.

set -ex
cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests,extended_tests -- -D warnings
cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests,extended_tests -- -D warnings
8 changes: 6 additions & 2 deletions datafusion/common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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: <https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value>
needless_pass_by_value = "deny"

[lib]
name = "datafusion_common"
Expand Down
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
18 changes: 9 additions & 9 deletions datafusion/common/src/hash_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ fn hash_array_primitive<T>(
/// with the new hash using `combine_hashes`
#[cfg(not(feature = "force_hash_collisions"))]
fn hash_array<T>(
array: T,
array: &T,
random_state: &RandomState,
hashes_buffer: &mut [u64],
rehash: bool,
Expand Down Expand Up @@ -400,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::<i32>(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::<i64>(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::<i32>(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::<i64>(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)?,
Expand Down
14 changes: 8 additions & 6 deletions datafusion/common/src/scalar/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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| {
Expand Down Expand Up @@ -4772,12 +4772,11 @@ impl fmt::Display for ScalarValue {
}
}

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}")
}
Expand Down Expand Up @@ -5568,6 +5567,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 +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)]
fn round_trip_through_scalar(arr: ArrayRef) {
for i in 0..arr.len() {
// convert Scalar --> Array
Expand Down Expand Up @@ -7569,6 +7570,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