Skip to content

Commit

Permalink
Revert "remove derive(Copy) from Operator (apache#11132)" (apache…
Browse files Browse the repository at this point in the history
…#11341)

This reverts commit b468ba7.
  • Loading branch information
alamb authored and findepi committed Jul 16, 2024
1 parent 8f6df68 commit 8095427
Show file tree
Hide file tree
Showing 17 changed files with 50 additions and 70 deletions.
24 changes: 9 additions & 15 deletions datafusion/core/src/physical_optimizer/pruning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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");
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1358,13 +1358,11 @@ fn build_predicate_expression(
.map(|e| {
Arc::new(phys_expr::BinaryExpr::new(
in_list.expr().clone(),
eq_op.clone(),
eq_op,
e.clone(),
)) as _
})
.reduce(|a, b| {
Arc::new(phys_expr::BinaryExpr::new(a, re_op.clone(), b)) as _
})
.reduce(|a, b| Arc::new(phys_expr::BinaryExpr::new(a, re_op, b)) as _)
.unwrap();
return build_predicate_expression(&change_expr, schema, required_columns);
} else {
Expand All @@ -1376,7 +1374,7 @@ fn build_predicate_expression(
if let Some(bin_expr) = expr_any.downcast_ref::<phys_expr::BinaryExpr>() {
(
bin_expr.left().clone(),
bin_expr.op().clone(),
*bin_expr.op(),
bin_expr.right().clone(),
)
} else {
Expand All @@ -1388,19 +1386,15 @@ 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.clone(),
right_expr,
)),
_ => Arc::new(phys_expr::BinaryExpr::new(left_expr, op, right_expr)),
};
return expr;
}
Expand Down
2 changes: 1 addition & 1 deletion datafusion/expr/src/operator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use std::ops;
use std::ops::Not;

/// Operators applied to expressions
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub enum Operator {
/// Expressions are equal
Eq,
Expand Down
4 changes: 2 additions & 2 deletions datafusion/expr/src/type_coercion/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
22 changes: 11 additions & 11 deletions datafusion/expr/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -997,7 +997,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]`
Expand All @@ -1020,19 +1020,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)
}
Expand All @@ -1049,17 +1049,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<'a>(expr: &'a Expr, op: &Operator) -> Vec<&'a Expr> {
pub fn split_binary(expr: &Expr, op: Operator) -> Vec<&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)
}
Expand Down Expand Up @@ -1613,13 +1613,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")]
);
}
Expand All @@ -1629,7 +1629,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]
);
}
Expand Down
8 changes: 4 additions & 4 deletions datafusion/optimizer/src/analyzer/type_coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<_>>>()?;
Expand All @@ -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((
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions datafusion/optimizer/src/simplify_expressions/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.clone())
Expr::BinaryExpr(BinaryExpr { left, op, right }) if *op == search_op => {
expr_contains(left, needle, search_op)
|| expr_contains(right, needle, search_op)
}
_ => expr == needle,
Expand All @@ -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);
Expand All @@ -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.clone(),
*op,
Box::new(right_expr),
))
}
Expand Down
6 changes: 3 additions & 3 deletions datafusion/optimizer/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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<'a>(expr: &'a Expr, op: &Operator) -> Vec<&'a Expr> {
pub fn split_binary(expr: &Expr, op: Operator) -> Vec<&Expr> {
expr_utils::split_binary(expr, op)
}

Expand Down
4 changes: 2 additions & 2 deletions datafusion/physical-expr-common/src/datum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand All @@ -88,7 +88,7 @@ pub fn apply_cmp_for_nested(

/// Compare on nested type List, Struct, and so on
pub fn compare_op_for_nested(
op: &Operator,
op: Operator,
lhs: &dyn Datum,
rhs: &dyn Datum,
) -> Result<BooleanArray> {
Expand Down
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/expressions/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -329,7 +329,7 @@ impl PhysicalExpr for BinaryExpr {
) -> Result<Arc<dyn PhysicalExpr>> {
Ok(Arc::new(BinaryExpr::new(
Arc::clone(&children[0]),
self.op.clone(),
self.op,
Arc::clone(&children[1]),
)))
}
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/intervals/cp_solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/intervals/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ pub fn create_physical_expr(
//
// There should be no coercion during physical
// planning.
binary(lhs, op.clone(), rhs, input_schema)
binary(lhs, *op, rhs, input_schema)
}
Expr::Like(Like {
negated,
Expand Down
8 changes: 4 additions & 4 deletions datafusion/physical-expr/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-plan/src/joins/hash_join.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1238,7 +1238,7 @@ fn eq_dyn_null(
} else {
Operator::Eq
};
return Ok(compare_op_for_nested(&op, &left, &right)?);
return Ok(compare_op_for_nested(op, &left, &right)?);
}
match (left.data_type(), right.data_type()) {
_ if null_equals_null => not_distinct(&left, &right),
Expand Down
6 changes: 1 addition & 5 deletions datafusion/proto/src/logical_plan/from_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,7 @@ pub fn parse_expr(
Ok(operands
.into_iter()
.reduce(|left, right| {
Expr::BinaryExpr(BinaryExpr::new(
Box::new(left),
op.clone(),
Box::new(right),
))
Expr::BinaryExpr(BinaryExpr::new(Box::new(left), op, Box::new(right)))
})
.expect("Binary expression could not be reduced to a single expression."))
}
Expand Down
2 changes: 1 addition & 1 deletion datafusion/substrait/src/logical_plan/consumer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,7 @@ pub async fn from_substrait_rex(
Arc::try_unwrap(expr)
.unwrap_or_else(|arc: Arc<Expr>| (*arc).clone()),
), // Avoid cloning if possible
op: op.clone(),
op,
right: Box::new(arg),
})),
None => Arc::new(arg),
Expand Down
Loading

0 comments on commit 8095427

Please sign in to comment.