remove derive(Copy) from Operator (apache#11132)

samuelcolvin · findepi · commit 4098365378e0 · 2024-07-16T13:26:36.000+02:00
* remove derive Copy from Operator

* fix formatting and more cloning

* fix remaining case

* fix docs case

* fix operator borrows
diff --git 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<dyn PhysicalExpr> {
@@ -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::<phys_expr::BinaryExpr>() {
             (
                 bin_expr.left().clone(),
-                *bin_expr.op(),
+                bin_expr.op().clone(),
                 bin_expr.right().clone(),
             )
         } else {
@@ -1386,15 +1388,19 @@ 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)
                 if is_always_true(left) || is_always_true(right) =>
             {
                 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
@@ -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
@@ -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
@@ -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<Expr> {
-    split_binary_owned(expr, Operator::And)
+    split_binary_owned(expr, &Operator::And)
 }
 
 /// Splits an owned binary operator tree [`Expr`] such as `A <OP> B <OP> C` => `[A, B, C]`
@@ -1019,19 +1019,19 @@ pub fn split_conjunction_owned(expr: Expr) -> Vec<Expr> {
 /// ];
 ///
 /// // 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<Expr> {
+pub fn split_binary_owned(expr: Expr, op: &Operator) -> Vec<Expr> {
     split_binary_owned_impl(expr, op, vec![])
 }
 
 fn split_binary_owned_impl(
     expr: Expr,
-    operator: Operator,
+    operator: &Operator,
     mut exprs: Vec<Expr>,
 ) -> Vec<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_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 <OP> B <OP> 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
@@ -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::<Result<Vec<_>>>()?;
@@ -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
@@ -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
@@ -177,13 +177,13 @@ pub fn split_conjunction_owned(expr: Expr) -> Vec<Expr> {
 /// ];
 ///
 /// // 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<Expr> {
+pub fn split_binary_owned(expr: Expr, op: &Operator) -> Vec<Expr> {
     expr_utils::split_binary_owned(expr, op)
 }
 
@@ -194,7 +194,7 @@ pub fn split_binary_owned(expr: Expr, op: Operator) -> Vec<Expr> {
     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
@@ -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<ColumnarValue> {
@@ -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<BooleanArray> {
diff --git 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<Arc<dyn PhysicalExpr>> {
         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
@@ -222,7 +222,7 @@ pub fn propagate_arithmetic(
     left_child: &Interval,
     right_child: &Interval,
 ) -> Result<Option<(Interval, Interval)>> {
-    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
@@ -63,7 +63,7 @@ pub fn check_support(expr: &Arc<dyn PhysicalExpr>, schema: &SchemaRef) -> bool {
 }
 
 // This function returns the inverse operator of the given operator.
-pub fn get_inverse_op(op: Operator) -> Result<Operator> {
+pub fn get_inverse_op(op: &Operator) -> Result<Operator> {
     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
@@ -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
@@ -44,7 +44,7 @@ use petgraph::stable_graph::StableGraph;
 pub fn split_conjunction(
     predicate: &Arc<dyn PhysicalExpr>,
 ) -> Vec<&Arc<dyn PhysicalExpr>> {
-    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<dyn PhysicalExpr>,
 ) -> Vec<&Arc<dyn PhysicalExpr>> {
-    split_impl(Operator::Or, predicate, vec![])
+    split_impl(&Operator::Or, predicate, vec![])
 }
 
 fn split_impl<'a>(
-    operator: Operator,
+    operator: &Operator,
     predicate: &'a Arc<dyn PhysicalExpr>,
     mut exprs: Vec<&'a Arc<dyn PhysicalExpr>>,
 ) -> Vec<&'a Arc<dyn PhysicalExpr>> {
     match predicate.as_any().downcast_ref::<BinaryExpr>() {
-        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
@@ -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
@@ -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<Expression> =
         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,

Original file line number	Diff line number	Diff line change
`@@ -269,7 +269,7 @@ impl PhysicalExpr for BinaryExpr {`
`269`	`269`	`if right_data_type != left_data_type {`
`270`	`270`	`return internal_err!("type mismatch");`
`271`	`271`	`}`
`272`		`- return apply_cmp_for_nested(self.op, &lhs, &rhs);`
	`272`	`+ return apply_cmp_for_nested(&self.op, &lhs, &rhs);`
`273`	`273`	`}`
`274`	`274`
`275`	`275`	`match self.op {`
`@@ -329,7 +329,7 @@ impl PhysicalExpr for BinaryExpr {`
`329`	`329`	`) -> Result<Arc<dyn PhysicalExpr>> {`
`330`	`330`	`Ok(Arc::new(BinaryExpr::new(`
`331`	`331`	`children[0].clone(),`
`332`		`- self.op,`
	`332`	`+ self.op.clone(),`
`333`	`333`	`children[1].clone(),`
`334`	`334`	`)))`
`335`	`335`	`}`