diff --git a/datafusion/physical-expr/src/analysis.rs b/datafusion/physical-expr/src/analysis.rs index 1d59dab8fd6d..f34dfb4ae1b4 100644 --- a/datafusion/physical-expr/src/analysis.rs +++ b/datafusion/physical-expr/src/analysis.rs @@ -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 @@ -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, - target_expr_and_indices: Vec<(Arc, usize)>, + target_expr_and_indices: &[(Arc, usize)], ) -> Result { let initial_boundaries = target_boundaries.clone(); target_expr_and_indices.iter().for_each(|(expr, i)| { diff --git a/datafusion/physical-expr/src/equivalence/properties/mod.rs b/datafusion/physical-expr/src/equivalence/properties/mod.rs index 4d919d623bf9..c13618feb8aa 100644 --- a/datafusion/physical-expr/src/equivalence/properties/mod.rs +++ b/datafusion/physical-expr/src/equivalence/properties/mod.rs @@ -380,7 +380,7 @@ impl EquivalenceProperties { right: Arc, ) -> 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()?; diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index b09d57f02d58..f3a71cbea480 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -573,10 +573,10 @@ impl BinaryExpr { ) -> Result>> { 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), @@ -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 => { @@ -854,7 +854,7 @@ fn pre_selection_scatter( Ok(ColumnarValue::Array(Arc::new(boolean_result))) } -fn concat_elements(left: Arc, right: Arc) -> Result { +fn concat_elements(left: &ArrayRef, right: &ArrayRef) -> Result { Ok(match left.data_type() { DataType::Utf8 => Arc::new(concat_elements_utf8( left.as_string::(), diff --git a/datafusion/physical-expr/src/expressions/binary/kernels.rs b/datafusion/physical-expr/src/expressions/binary/kernels.rs index 6c96975ed644..ad44b0021203 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels.rs @@ -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 { @@ -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> { diff --git a/datafusion/physical-expr/src/expressions/in_list.rs b/datafusion/physical-expr/src/expressions/in_list.rs index fa91635d9bfd..eeac986beec0 100644 --- a/datafusion/physical-expr/src/expressions/in_list.rs +++ b/datafusion/physical-expr/src/expressions/in_list.rs @@ -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(array: T) -> ArrayHashSet +fn make_hash_set(array: &T) -> ArrayHashSet where T: ArrayAccessor, T::Item: IsEqual, @@ -183,26 +183,26 @@ where /// Creates a `Box` for the given list of `IN` expressions and `batch` fn make_set(array: &dyn Array) -> Result> { 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::(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::(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") diff --git a/datafusion/physical-expr/src/expressions/literal.rs b/datafusion/physical-expr/src/expressions/literal.rs index 94e91d43a1c4..359bfcefdbb5 100644 --- a/datafusion/physical-expr/src/expressions/literal.rs +++ b/datafusion/physical-expr/src/expressions/literal.rs @@ -137,6 +137,7 @@ impl PhysicalExpr for Literal { } /// Create a literal expression +#[allow(clippy::needless_pass_by_value)] pub fn lit(value: T) -> Arc { match value.lit() { Expr::Literal(v, _) => Arc::new(Literal::new(v)), diff --git a/datafusion/physical-expr/src/lib.rs b/datafusion/physical-expr/src/lib.rs index aa8c9e50fd71..f59582f40506 100644 --- a/datafusion/physical-expr/src/lib.rs +++ b/datafusion/physical-expr/src/lib.rs @@ -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; diff --git a/datafusion/physical-expr/src/utils/guarantee.rs b/datafusion/physical-expr/src/utils/guarantee.rs index 8a57cc7b7c15..d63a9590c3f6 100644 --- a/datafusion/physical-expr/src/utils/guarantee.rs +++ b/datafusion/physical-expr/src/utils/guarantee.rs @@ -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::() @@ -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,