From b468ba788310d5dcd6ea4549f0d5b0f215ea49c9 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Thu, 27 Jun 2024 19:20:35 +0100 Subject: [PATCH] remove `derive(Copy)` from `Operator` (#11132) * remove derive Copy from Operator * fix formatting and more cloning * fix remaining case * fix docs case * fix operator borrows --- .../core/src/physical_optimizer/pruning.rs | 24 ++++++++++++------- datafusion/expr/src/operator.rs | 2 +- datafusion/expr/src/type_coercion/binary.rs | 4 ++-- datafusion/expr/src/utils.rs | 22 ++++++++--------- .../optimizer/src/analyzer/type_coercion.rs | 8 +++---- .../src/simplify_expressions/utils.rs | 8 +++---- datafusion/optimizer/src/utils.rs | 6 ++--- datafusion/physical-expr-common/src/datum.rs | 4 ++-- .../physical-expr/src/expressions/binary.rs | 4 ++-- .../physical-expr/src/intervals/cp_solver.rs | 2 +- .../physical-expr/src/intervals/utils.rs | 2 +- datafusion/physical-expr/src/planner.rs | 2 +- datafusion/physical-expr/src/utils/mod.rs | 8 +++---- .../proto/src/logical_plan/from_proto.rs | 6 ++++- .../substrait/src/logical_plan/producer.rs | 14 +++++++++-- 15 files changed, 68 insertions(+), 48 deletions(-) diff --git a/datafusion/core/src/physical_optimizer/pruning.rs b/datafusion/core/src/physical_optimizer/pruning.rs index 98aff0d65898..7051dd9978c2 100644 --- a/datafusion/core/src/physical_optimizer/pruning.rs +++ b/datafusion/core/src/physical_optimizer/pruning.rs @@ -987,8 +987,8 @@ impl<'a> PruningExpressionBuilder<'a> { }) } - fn op(&self) -> Operator { - self.op + fn op(&self) -> &Operator { + &self.op } fn scalar_expr(&self) -> &Arc { @@ -1064,7 +1064,7 @@ fn rewrite_expr_to_prunable( scalar_expr: &PhysicalExprRef, schema: DFSchema, ) -> Result<(PhysicalExprRef, Operator, PhysicalExprRef)> { - if !is_compare_op(op) { + if !is_compare_op(&op) { return plan_err!("rewrite_expr_to_prunable only support compare expression"); } @@ -1131,7 +1131,7 @@ fn rewrite_expr_to_prunable( } } -fn is_compare_op(op: Operator) -> bool { +fn is_compare_op(op: &Operator) -> bool { matches!( op, Operator::Eq @@ -1358,11 +1358,13 @@ fn build_predicate_expression( .map(|e| { Arc::new(phys_expr::BinaryExpr::new( in_list.expr().clone(), - eq_op, + eq_op.clone(), e.clone(), )) as _ }) - .reduce(|a, b| Arc::new(phys_expr::BinaryExpr::new(a, re_op, b)) as _) + .reduce(|a, b| { + Arc::new(phys_expr::BinaryExpr::new(a, re_op.clone(), b)) as _ + }) .unwrap(); return build_predicate_expression(&change_expr, schema, required_columns); } else { @@ -1374,7 +1376,7 @@ fn build_predicate_expression( if let Some(bin_expr) = expr_any.downcast_ref::() { ( bin_expr.left().clone(), - *bin_expr.op(), + bin_expr.op().clone(), bin_expr.right().clone(), ) } else { @@ -1386,7 +1388,7 @@ fn build_predicate_expression( let left_expr = build_predicate_expression(&left, schema, required_columns); let right_expr = build_predicate_expression(&right, schema, required_columns); // simplify boolean expression if applicable - let expr = match (&left_expr, op, &right_expr) { + let expr = match (&left_expr, &op, &right_expr) { (left, Operator::And, _) if is_always_true(left) => right_expr, (_, Operator::And, right) if is_always_true(right) => left_expr, (left, Operator::Or, right) @@ -1394,7 +1396,11 @@ fn build_predicate_expression( { unhandled } - _ => Arc::new(phys_expr::BinaryExpr::new(left_expr, op, right_expr)), + _ => Arc::new(phys_expr::BinaryExpr::new( + left_expr, + op.clone(), + right_expr, + )), }; return expr; } diff --git a/datafusion/expr/src/operator.rs b/datafusion/expr/src/operator.rs index a10312e23446..742511822a0f 100644 --- a/datafusion/expr/src/operator.rs +++ b/datafusion/expr/src/operator.rs @@ -25,7 +25,7 @@ use std::ops; use std::ops::Not; /// Operators applied to expressions -#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Hash)] +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)] pub enum Operator { /// Expressions are equal Eq, diff --git a/datafusion/expr/src/type_coercion/binary.rs b/datafusion/expr/src/type_coercion/binary.rs index d83fbfe49bc2..5645a2a4dede 100644 --- a/datafusion/expr/src/type_coercion/binary.rs +++ b/datafusion/expr/src/type_coercion/binary.rs @@ -1152,8 +1152,8 @@ mod tests { ]; for (i, input_type) in input_types.iter().enumerate() { let expect_type = &result_types[i]; - for op in comparison_op_types { - let (lhs, rhs) = get_input_types(&input_decimal, &op, input_type)?; + for op in &comparison_op_types { + let (lhs, rhs) = get_input_types(&input_decimal, op, input_type)?; assert_eq!(expect_type, &lhs); assert_eq!(expect_type, &rhs); } diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 9f585be8d68d..286f05309ea7 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -996,7 +996,7 @@ fn split_conjunction_impl<'a>(expr: &'a Expr, mut exprs: Vec<&'a Expr>) -> Vec<& /// assert_eq!(split_conjunction_owned(expr), split); /// ``` pub fn split_conjunction_owned(expr: Expr) -> Vec { - split_binary_owned(expr, Operator::And) + split_binary_owned(expr, &Operator::And) } /// Splits an owned binary operator tree [`Expr`] such as `A B C` => `[A, B, C]` @@ -1019,19 +1019,19 @@ pub fn split_conjunction_owned(expr: Expr) -> Vec { /// ]; /// /// // use split_binary_owned to split them -/// assert_eq!(split_binary_owned(expr, Operator::Plus), split); +/// assert_eq!(split_binary_owned(expr, &Operator::Plus), split); /// ``` -pub fn split_binary_owned(expr: Expr, op: Operator) -> Vec { +pub fn split_binary_owned(expr: Expr, op: &Operator) -> Vec { split_binary_owned_impl(expr, op, vec![]) } fn split_binary_owned_impl( expr: Expr, - operator: Operator, + operator: &Operator, mut exprs: Vec, ) -> Vec { match expr { - Expr::BinaryExpr(BinaryExpr { right, op, left }) if op == operator => { + Expr::BinaryExpr(BinaryExpr { right, op, left }) if &op == operator => { let exprs = split_binary_owned_impl(*left, operator, exprs); split_binary_owned_impl(*right, operator, exprs) } @@ -1048,17 +1048,17 @@ fn split_binary_owned_impl( /// Splits an binary operator tree [`Expr`] such as `A B C` => `[A, B, C]` /// /// See [`split_binary_owned`] for more details and an example. -pub fn split_binary(expr: &Expr, op: Operator) -> Vec<&Expr> { +pub fn split_binary<'a>(expr: &'a Expr, op: &Operator) -> Vec<&'a Expr> { split_binary_impl(expr, op, vec![]) } fn split_binary_impl<'a>( expr: &'a Expr, - operator: Operator, + operator: &Operator, mut exprs: Vec<&'a Expr>, ) -> Vec<&'a Expr> { match expr { - Expr::BinaryExpr(BinaryExpr { right, op, left }) if *op == operator => { + Expr::BinaryExpr(BinaryExpr { right, op, left }) if op == operator => { let exprs = split_binary_impl(left, operator, exprs); split_binary_impl(right, operator, exprs) } @@ -1612,13 +1612,13 @@ mod tests { #[test] fn test_split_binary_owned() { let expr = col("a"); - assert_eq!(split_binary_owned(expr.clone(), Operator::And), vec![expr]); + assert_eq!(split_binary_owned(expr.clone(), &Operator::And), vec![expr]); } #[test] fn test_split_binary_owned_two() { assert_eq!( - split_binary_owned(col("a").eq(lit(5)).and(col("b")), Operator::And), + split_binary_owned(col("a").eq(lit(5)).and(col("b")), &Operator::And), vec![col("a").eq(lit(5)), col("b")] ); } @@ -1628,7 +1628,7 @@ mod tests { let expr = col("a").eq(lit(5)).or(col("b")); assert_eq!( // expr is connected by OR, but pass in AND - split_binary_owned(expr.clone(), Operator::And), + split_binary_owned(expr.clone(), &Operator::And), vec![expr] ); } diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs index 4fccd4f254c3..51ec8d8af1d3 100644 --- a/datafusion/optimizer/src/analyzer/type_coercion.rs +++ b/datafusion/optimizer/src/analyzer/type_coercion.rs @@ -146,7 +146,7 @@ impl<'a> TypeCoercionRewriter<'a> { .map(|(lhs, rhs)| { // coerce the arguments as though they were a single binary equality // expression - let (lhs, rhs) = self.coerce_binary_op(lhs, Operator::Eq, rhs)?; + let (lhs, rhs) = self.coerce_binary_op(lhs, &Operator::Eq, rhs)?; Ok((lhs, rhs)) }) .collect::>>()?; @@ -157,12 +157,12 @@ impl<'a> TypeCoercionRewriter<'a> { fn coerce_binary_op( &self, left: Expr, - op: Operator, + op: &Operator, right: Expr, ) -> Result<(Expr, Expr)> { let (left_type, right_type) = get_input_types( &left.get_type(self.schema)?, - &op, + op, &right.get_type(self.schema)?, )?; Ok(( @@ -279,7 +279,7 @@ impl<'a> TreeNodeRewriter for TypeCoercionRewriter<'a> { )))) } Expr::BinaryExpr(BinaryExpr { left, op, right }) => { - let (left, right) = self.coerce_binary_op(*left, op, *right)?; + let (left, right) = self.coerce_binary_op(*left, &op, *right)?; Ok(Transformed::yes(Expr::BinaryExpr(BinaryExpr::new( Box::new(left), op, diff --git a/datafusion/optimizer/src/simplify_expressions/utils.rs b/datafusion/optimizer/src/simplify_expressions/utils.rs index 5da727cb5990..ed3fd75f3efd 100644 --- a/datafusion/optimizer/src/simplify_expressions/utils.rs +++ b/datafusion/optimizer/src/simplify_expressions/utils.rs @@ -69,8 +69,8 @@ pub static POWS_OF_TEN: [i128; 38] = [ /// expressions. Such as: (A AND B) AND C pub fn expr_contains(expr: &Expr, needle: &Expr, search_op: Operator) -> bool { match expr { - Expr::BinaryExpr(BinaryExpr { left, op, right }) if *op == search_op => { - expr_contains(left, needle, search_op) + Expr::BinaryExpr(BinaryExpr { left, op, right }) if op == &search_op => { + expr_contains(left, needle, search_op.clone()) || expr_contains(right, needle, search_op) } _ => expr == needle, @@ -88,7 +88,7 @@ pub fn delete_xor_in_complex_expr(expr: &Expr, needle: &Expr, is_left: bool) -> ) -> Expr { match expr { Expr::BinaryExpr(BinaryExpr { left, op, right }) - if *op == Operator::BitwiseXor => + if op == &Operator::BitwiseXor => { let left_expr = recursive_delete_xor_in_expr(left, needle, xor_counter); let right_expr = recursive_delete_xor_in_expr(right, needle, xor_counter); @@ -102,7 +102,7 @@ pub fn delete_xor_in_complex_expr(expr: &Expr, needle: &Expr, is_left: bool) -> Expr::BinaryExpr(BinaryExpr::new( Box::new(left_expr), - *op, + op.clone(), Box::new(right_expr), )) } diff --git a/datafusion/optimizer/src/utils.rs b/datafusion/optimizer/src/utils.rs index 05b1744d90c5..0549845430a6 100644 --- a/datafusion/optimizer/src/utils.rs +++ b/datafusion/optimizer/src/utils.rs @@ -177,13 +177,13 @@ pub fn split_conjunction_owned(expr: Expr) -> Vec { /// ]; /// /// // use split_binary_owned to split them -/// assert_eq!(split_binary_owned(expr, Operator::Plus), split); +/// assert_eq!(split_binary_owned(expr, &Operator::Plus), split); /// ``` #[deprecated( since = "34.0.0", note = "use `datafusion_expr::utils::split_binary_owned` instead" )] -pub fn split_binary_owned(expr: Expr, op: Operator) -> Vec { +pub fn split_binary_owned(expr: Expr, op: &Operator) -> Vec { expr_utils::split_binary_owned(expr, op) } @@ -194,7 +194,7 @@ pub fn split_binary_owned(expr: Expr, op: Operator) -> Vec { since = "34.0.0", note = "use `datafusion_expr::utils::split_binary` instead" )] -pub fn split_binary(expr: &Expr, op: Operator) -> Vec<&Expr> { +pub fn split_binary<'a>(expr: &'a Expr, op: &Operator) -> Vec<&'a Expr> { expr_utils::split_binary(expr, op) } diff --git a/datafusion/physical-expr-common/src/datum.rs b/datafusion/physical-expr-common/src/datum.rs index f4ce0eebc081..fd3f2587e2ff 100644 --- a/datafusion/physical-expr-common/src/datum.rs +++ b/datafusion/physical-expr-common/src/datum.rs @@ -63,7 +63,7 @@ pub fn apply_cmp( /// Applies a binary [`Datum`] comparison kernel `f` to `lhs` and `rhs` for nested type like /// List, FixedSizeList, LargeList, Struct, Union, Map, or a dictionary of a nested type pub fn apply_cmp_for_nested( - op: Operator, + op: &Operator, lhs: &ColumnarValue, rhs: &ColumnarValue, ) -> Result { @@ -88,7 +88,7 @@ pub fn apply_cmp_for_nested( /// Compare on nested type List, Struct, and so on fn compare_op_for_nested( - op: Operator, + op: &Operator, lhs: &dyn Datum, rhs: &dyn Datum, ) -> Result { diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index 3a8f7ee56ace..d19279c20d10 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -269,7 +269,7 @@ impl PhysicalExpr for BinaryExpr { if right_data_type != left_data_type { return internal_err!("type mismatch"); } - return apply_cmp_for_nested(self.op, &lhs, &rhs); + return apply_cmp_for_nested(&self.op, &lhs, &rhs); } match self.op { @@ -329,7 +329,7 @@ impl PhysicalExpr for BinaryExpr { ) -> Result> { Ok(Arc::new(BinaryExpr::new( children[0].clone(), - self.op, + self.op.clone(), children[1].clone(), ))) } diff --git a/datafusion/physical-expr/src/intervals/cp_solver.rs b/datafusion/physical-expr/src/intervals/cp_solver.rs index 5ba628e7ce40..6fbcd461af66 100644 --- a/datafusion/physical-expr/src/intervals/cp_solver.rs +++ b/datafusion/physical-expr/src/intervals/cp_solver.rs @@ -222,7 +222,7 @@ pub fn propagate_arithmetic( left_child: &Interval, right_child: &Interval, ) -> Result> { - let inverse_op = get_inverse_op(*op)?; + let inverse_op = get_inverse_op(op)?; match (left_child.data_type(), right_child.data_type()) { // If we have a child whose type is a time interval (i.e. DataType::Interval), // we need special handling since timestamp differencing results in a diff --git a/datafusion/physical-expr/src/intervals/utils.rs b/datafusion/physical-expr/src/intervals/utils.rs index b426a656fba9..37527802f84d 100644 --- a/datafusion/physical-expr/src/intervals/utils.rs +++ b/datafusion/physical-expr/src/intervals/utils.rs @@ -63,7 +63,7 @@ pub fn check_support(expr: &Arc, schema: &SchemaRef) -> bool { } // This function returns the inverse operator of the given operator. -pub fn get_inverse_op(op: Operator) -> Result { +pub fn get_inverse_op(op: &Operator) -> Result { match op { Operator::Plus => Ok(Operator::Minus), Operator::Minus => Ok(Operator::Plus), diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs index 9e8561eb68c5..29b9069c0456 100644 --- a/datafusion/physical-expr/src/planner.rs +++ b/datafusion/physical-expr/src/planner.rs @@ -196,7 +196,7 @@ pub fn create_physical_expr( // // There should be no coercion during physical // planning. - binary(lhs, *op, rhs, input_schema) + binary(lhs, op.clone(), rhs, input_schema) } Expr::Like(Like { negated, diff --git a/datafusion/physical-expr/src/utils/mod.rs b/datafusion/physical-expr/src/utils/mod.rs index 005d834552f9..492cb02941df 100644 --- a/datafusion/physical-expr/src/utils/mod.rs +++ b/datafusion/physical-expr/src/utils/mod.rs @@ -44,7 +44,7 @@ use petgraph::stable_graph::StableGraph; pub fn split_conjunction( predicate: &Arc, ) -> Vec<&Arc> { - split_impl(Operator::And, predicate, vec![]) + split_impl(&Operator::And, predicate, vec![]) } /// Assume the predicate is in the form of DNF, split the predicate to a Vec of PhysicalExprs. @@ -53,16 +53,16 @@ pub fn split_conjunction( pub fn split_disjunction( predicate: &Arc, ) -> Vec<&Arc> { - split_impl(Operator::Or, predicate, vec![]) + split_impl(&Operator::Or, predicate, vec![]) } fn split_impl<'a>( - operator: Operator, + operator: &Operator, predicate: &'a Arc, mut exprs: Vec<&'a Arc>, ) -> Vec<&'a Arc> { match predicate.as_any().downcast_ref::() { - Some(binary) if binary.op() == &operator => { + Some(binary) if binary.op() == operator => { let exprs = split_impl(operator, binary.left(), exprs); split_impl(operator, binary.right(), exprs) } diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index 4cd30d6fa475..21331a94c18c 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -266,7 +266,11 @@ pub fn parse_expr( Ok(operands .into_iter() .reduce(|left, right| { - Expr::BinaryExpr(BinaryExpr::new(Box::new(left), op, Box::new(right))) + Expr::BinaryExpr(BinaryExpr::new( + Box::new(left), + op.clone(), + Box::new(right), + )) }) .expect("Binary expression could not be reduced to a single expression.")) } diff --git a/datafusion/substrait/src/logical_plan/producer.rs b/datafusion/substrait/src/logical_plan/producer.rs index b59843ac468d..6c00a326291a 100644 --- a/datafusion/substrait/src/logical_plan/producer.rs +++ b/datafusion/substrait/src/logical_plan/producer.rs @@ -664,7 +664,12 @@ fn to_substrait_join_expr( extension_info, )?; // AND with existing expression - exprs.push(make_binary_op_scalar_func(&l, &r, eq_op, extension_info)); + exprs.push(make_binary_op_scalar_func( + &l, + &r, + eq_op.clone(), + extension_info, + )); } let join_expr: Option = exprs.into_iter().reduce(|acc: Expression, e: Expression| { @@ -1154,7 +1159,12 @@ pub fn to_substrait_rex( let l = to_substrait_rex(ctx, left, schema, col_ref_offset, extension_info)?; let r = to_substrait_rex(ctx, right, schema, col_ref_offset, extension_info)?; - Ok(make_binary_op_scalar_func(&l, &r, *op, extension_info)) + Ok(make_binary_op_scalar_func( + &l, + &r, + op.clone(), + extension_info, + )) } Expr::Case(Case { expr,