This is an automated email from the ASF dual-hosted git repository.
github-bot pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new f32984b2db CI: add `clippy::needless_pass_by_value` rule (#18468)
f32984b2db is described below
commit f32984b2dbf9e5a193c20643ce624167295fbd61
Author: Yongting You <[email protected]>
AuthorDate: Fri Nov 7 18:51:03 2025 +0800
CI: add `clippy::needless_pass_by_value` rule (#18468)
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes #123` indicates that this PR will close issue #123.
-->
An initial attempt towards
https://github.com/apache/datafusion/issues/18467
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
### Rationale for the additional lint rule
`clippy::needless_pass_by_value`
There is a clippy lint rule that is not turned on by the current
strictness level in CI:
https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
Note it has the `Clippy` category `pedantic`, and its description is
`lints which are rather strict or have occasional false positives` from
https://doc.rust-lang.org/nightly/clippy
It seems we have been suffering from the excessive copying issue for
quite some time, and @alamb is on the front line now
https://github.com/apache/datafusion/issues/18413. I think this extra
lint rule is able to help.
### Implementation plan
This PR only enables this rule in `datafusion-common` package, and apply
`#[allow(clippy::needless_pass_by_value)]` for all violations.
If this PR makes sense, we can open a tracking issue and roll out this
check to the remaining workspace packages. At least this can help
prevent new inefficient patterns and identify existing issues that we
can fix gradually.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
---
ci/scripts/rust_clippy.sh | 2 +-
datafusion/common/src/hash_utils.rs | 18 +++++++++---------
datafusion/common/src/lib.rs | 5 +++++
datafusion/common/src/scalar/mod.rs | 11 +++++------
datafusion/common/src/scalar/struct_builder.rs | 1 +
datafusion/expr/src/logical_plan/plan.rs | 4 ++--
6 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/ci/scripts/rust_clippy.sh b/ci/scripts/rust_clippy.sh
index 1557bd56ea..6a00ad8109 100755
--- a/ci/scripts/rust_clippy.sh
+++ b/ci/scripts/rust_clippy.sh
@@ -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
\ No newline at end of file
diff --git a/datafusion/common/src/hash_utils.rs
b/datafusion/common/src/hash_utils.rs
index 4b18351f70..b4488c770d 100644
--- a/datafusion/common/src/hash_utils.rs
+++ b/datafusion/common/src/hash_utils.rs
@@ -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,
@@ -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)?,
diff --git a/datafusion/common/src/lib.rs b/datafusion/common/src/lib.rs
index 76c7b46e32..c8d5a30ee3 100644
--- a/datafusion/common/src/lib.rs
+++ b/datafusion/common/src/lib.rs
@@ -23,6 +23,11 @@
// 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))]
mod column;
mod dfschema;
diff --git a/datafusion/common/src/scalar/mod.rs
b/datafusion/common/src/scalar/mod.rs
index 188a169a3d..f2e18f7de8 100644
--- a/datafusion/common/src/scalar/mod.rs
+++ b/datafusion/common/src/scalar/mod.rs
@@ -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,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}")
}
diff --git a/datafusion/common/src/scalar/struct_builder.rs
b/datafusion/common/src/scalar/struct_builder.rs
index 56daee9045..045b577824 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.
+ #[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();
diff --git a/datafusion/expr/src/logical_plan/plan.rs
b/datafusion/expr/src/logical_plan/plan.rs
index 0f0d81186d..0b89a52509 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<Expr>) -> 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<LogicalPlan>) -> Result<()> {
if !inputs.is_empty() {
return internal_err!("{self:?} should have no inputs, got: {:?}",
inputs);
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
