Skip to content

chore: Enforce lint rule clippy::needless_pass_by_value to datafusion-physical-expr #18557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 9, 2025
Merged

chore: Enforce lint rule clippy::needless_pass_by_value to datafusion-physical-expr #18557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions datafusion/physical-expr/src/analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ pub fn analyze(
.update_ranges(&mut target_indices_and_boundaries, Interval::CERTAINLY_TRUE)?
{
PropagationResult::Success => {
shrink_boundaries(graph, target_boundaries, target_expr_and_indices)
shrink_boundaries(&graph, target_boundaries, &target_expr_and_indices)
}
PropagationResult::Infeasible => {
// If the propagation result is infeasible, set intervals to None
Expand All @@ -239,9 +239,9 @@ pub fn analyze(
/// Following this, it constructs and returns a new `AnalysisContext` with the
/// updated parameters.
fn shrink_boundaries(
graph: ExprIntervalGraph,
graph: &ExprIntervalGraph,
mut target_boundaries: Vec<ExprBoundaries>,
target_expr_and_indices: Vec<(Arc<dyn PhysicalExpr>, usize)>,
target_expr_and_indices: &[(Arc<dyn PhysicalExpr>, usize)],
) -> Result<AnalysisContext> {
let initial_boundaries = target_boundaries.clone();
target_expr_and_indices.iter().for_each(|(expr, i)| {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/equivalence/properties/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ impl EquivalenceProperties {
right: Arc<dyn PhysicalExpr>,
) -> Result<()> {
// Add equal expressions to the state:
if self.eq_group.add_equal_conditions(Arc::clone(&left), right) {
if self.eq_group.add_equal_conditions(left, right) {
self.update_oeq_cache()?;
}
self.update_oeq_cache()?;
Expand Down
20 changes: 10 additions & 10 deletions datafusion/physical-expr/src/expressions/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,10 +573,10 @@ impl BinaryExpr {
) -> Result<Option<Result<ArrayRef>>> {
use Operator::*;
let scalar_result = match &self.op {
RegexMatch => regex_match_dyn_scalar(array, scalar, false, false),
RegexIMatch => regex_match_dyn_scalar(array, scalar, false, true),
RegexNotMatch => regex_match_dyn_scalar(array, scalar, true, false),
RegexNotIMatch => regex_match_dyn_scalar(array, scalar, true, true),
RegexMatch => regex_match_dyn_scalar(array, &scalar, false, false),
RegexIMatch => regex_match_dyn_scalar(array, &scalar, false, true),
RegexNotMatch => regex_match_dyn_scalar(array, &scalar, true, false),
RegexNotIMatch => regex_match_dyn_scalar(array, &scalar, true, true),
BitwiseAnd => bitwise_and_dyn_scalar(array, scalar),
BitwiseOr => bitwise_or_dyn_scalar(array, scalar),
BitwiseXor => bitwise_xor_dyn_scalar(array, scalar),
Expand Down Expand Up @@ -625,16 +625,16 @@ impl BinaryExpr {
)
}
}
RegexMatch => regex_match_dyn(left, right, false, false),
RegexIMatch => regex_match_dyn(left, right, false, true),
RegexNotMatch => regex_match_dyn(left, right, true, false),
RegexNotIMatch => regex_match_dyn(left, right, true, true),
RegexMatch => regex_match_dyn(&left, &right, false, false),
RegexIMatch => regex_match_dyn(&left, &right, false, true),
RegexNotMatch => regex_match_dyn(&left, &right, true, false),
RegexNotIMatch => regex_match_dyn(&left, &right, true, true),
BitwiseAnd => bitwise_and_dyn(left, right),
BitwiseOr => bitwise_or_dyn(left, right),
BitwiseXor => bitwise_xor_dyn(left, right),
BitwiseShiftRight => bitwise_shift_right_dyn(left, right),
BitwiseShiftLeft => bitwise_shift_left_dyn(left, right),
StringConcat => concat_elements(left, right),
StringConcat => concat_elements(&left, &right),
AtArrow | ArrowAt | Arrow | LongArrow | HashArrow | HashLongArrow | AtAt
| HashMinus | AtQuestion | Question | QuestionAnd | QuestionPipe
| IntegerDivide => {
Expand Down Expand Up @@ -854,7 +854,7 @@ fn pre_selection_scatter(
Ok(ColumnarValue::Array(Arc::new(boolean_result)))
}

fn concat_elements(left: Arc<dyn Array>, right: Arc<dyn Array>) -> Result<ArrayRef> {
fn concat_elements(left: &ArrayRef, right: &ArrayRef) -> Result<ArrayRef> {
Ok(match left.data_type() {
DataType::Utf8 => Arc::new(concat_elements_utf8(
left.as_string::<i32>(),
Expand Down
6 changes: 3 additions & 3 deletions datafusion/physical-expr/src/expressions/binary/kernels.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@ macro_rules! regexp_is_match_flag {
}

pub(crate) fn regex_match_dyn(
left: ArrayRef,
right: ArrayRef,
left: &ArrayRef,
right: &ArrayRef,
not_match: bool,
flag: bool,
) -> Result<ArrayRef> {
Expand Down Expand Up @@ -259,7 +259,7 @@ macro_rules! regexp_is_match_flag_scalar {

pub(crate) fn regex_match_dyn_scalar(
left: &dyn Array,
right: ScalarValue,
right: &ScalarValue,
not_match: bool,
flag: bool,
) -> Option<Result<ArrayRef>> {
Expand Down
14 changes: 7 additions & 7 deletions datafusion/physical-expr/src/expressions/in_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ where
///
/// Note: This is split into a separate function as higher-rank trait bounds currently
/// cause type inference to misbehave
fn make_hash_set<T>(array: T) -> ArrayHashSet
fn make_hash_set<T>(array: &T) -> ArrayHashSet
where
T: ArrayAccessor,
T::Item: IsEqual,
Expand Down Expand Up @@ -183,26 +183,26 @@ where
/// Creates a `Box<dyn Set>` for the given list of `IN` expressions and `batch`
fn make_set(array: &dyn Array) -> Result<Arc<dyn Set>> {
Ok(downcast_primitive_array! {
array => Arc::new(ArraySet::new(array, make_hash_set(array))),
array => Arc::new(ArraySet::new(array, make_hash_set(&array))),
DataType::Boolean => {
let array = as_boolean_array(array)?;
Arc::new(ArraySet::new(array, make_hash_set(array)))
Arc::new(ArraySet::new(array, make_hash_set(&array)))
},
DataType::Utf8 => {
let array = as_string_array(array)?;
Arc::new(ArraySet::new(array, make_hash_set(array)))
Arc::new(ArraySet::new(array, make_hash_set(&array)))
}
DataType::LargeUtf8 => {
let array = as_largestring_array(array);
Arc::new(ArraySet::new(array, make_hash_set(array)))
Arc::new(ArraySet::new(array, make_hash_set(&array)))
}
DataType::Binary => {
let array = as_generic_binary_array::<i32>(array)?;
Arc::new(ArraySet::new(array, make_hash_set(array)))
Arc::new(ArraySet::new(array, make_hash_set(&array)))
}
DataType::LargeBinary => {
let array = as_generic_binary_array::<i64>(array)?;
Arc::new(ArraySet::new(array, make_hash_set(array)))
Arc::new(ArraySet::new(array, make_hash_set(&array)))
}
DataType::Dictionary(_, _) => unreachable!("dictionary should have been flattened"),
d => return not_impl_err!("DataType::{d} not supported in InList")
Expand Down
1 change: 1 addition & 0 deletions datafusion/physical-expr/src/expressions/literal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ impl PhysicalExpr for Literal {
}

/// Create a literal expression
#[allow(clippy::needless_pass_by_value)]
Copy link
Contributor Author

@corasaurus-hex corasaurus-hex Nov 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Changing this to be &T produced the following errors:

❯ ./ci/scripts/rust_clippy.sh 2>&1 | grep '^error\[' -A1 | grep '^  ' | sort | grep -o -E 'datafusion[^:]+' | uniq -c | sort -n | awk '{ print $1, "error(s)\t", $2}
1 error(s)	 datafusion/physical-expr/src/planner.rs
2 error(s)	 datafusion/physical-expr/src/expressions/literal.rs
3 error(s)	 datafusion/physical-expr/src/simplifier/mod.rs
4 error(s)	 datafusion/physical-expr/src/utils/mod.rs
5 error(s)	 datafusion/physical-expr/src/equivalence/class.rs
6 error(s)	 datafusion/physical-expr/src/expressions/dynamic_filters.rs
18 error(s)	 datafusion/physical-expr/src/simplifier/unwrap_cast.rs
22 error(s)	 datafusion/physical-expr/src/expressions/binary.rs
62 error(s)	 datafusion/physical-expr/src/expressions/in_list.rs
79 error(s)	 datafusion/physical-expr/src/expressions/case.rs
82 error(s)	 datafusion/physical-expr/src/expressions/in_list.rs

❯ ./ci/scripts/rust_clippy.sh 2>&1 | grep '^error\[' | sort | uniq -c | sort -n
   1 error[E0599]: the method `collect` exists for struct `Map<Range<i32>, fn(&_) -> Arc<dyn PhysicalExpr> {lit::<_>}>`, but its trait bounds were not satisfied
   1 error[E0631]: type mismatch in function arguments
   4 error[E0277]: the size for values of type `[{integer}]` cannot be known at compilation time
   4 error[E0277]: the trait bound `[{integer}]: datafusion_expr::Literal` is not satisfied
  39 error[E0277]: the size for values of type `str` cannot be known at compilation time
  39 error[E0277]: the trait bound `str: datafusion_expr::Literal` is not satisfied
 196 error[E0308]: mismatched types

It's exposed publicly, too, and likely used in a lot of user code. Definitely not worth modifying.

Copy link
Contributor

@adriangb adriangb Nov 9, 2025

Choose a reason for hiding this comment

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

Thanks for catching this and for pointing it out for reviewers.

pub fn lit<T: datafusion_expr::Literal>(value: T) -> Arc<dyn PhysicalExpr> {
match value.lit() {
Expr::Literal(v, _) => Arc::new(Literal::new(v)),
Expand Down
3 changes: 3 additions & 0 deletions datafusion/physical-expr/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
// 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)]
#![cfg_attr(test, allow(clippy::needless_pass_by_value))]

// Backward compatibility
pub mod aggregate;
Expand Down
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/utils/guarantee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ impl LiteralGuarantee {
// for an `AND` conjunction to be true, all terms individually must be true
.fold(GuaranteeBuilder::new(), |builder, expr| {
if let Some(cel) = ColOpLit::try_new(expr) {
builder.aggregate_conjunct(cel)
builder.aggregate_conjunct(&cel)
} else if let Some(inlist) = expr
.as_any()
.downcast_ref::<crate::expressions::InListExpr>()
Expand Down Expand Up @@ -292,7 +292,7 @@ impl<'a> GuaranteeBuilder<'a> {
/// # Examples
/// * `AND (a = 1)`: `a` is guaranteed to be 1
/// * `AND (a != 1)`: a is guaranteed to not be 1
fn aggregate_conjunct(self, col_op_lit: ColOpLit<'a>) -> Self {
fn aggregate_conjunct(self, col_op_lit: &ColOpLit<'a>) -> Self {
self.aggregate_multi_conjunct(
col_op_lit.col,
col_op_lit.guarantee,
Expand Down