Skip to content
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.

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

remove derive(Copy) from Operator #11132

Merged
merged 5 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 15 additions & 9 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,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 {
Expand All @@ -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 {
Expand All @@ -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;
}
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, Copy, Clone, PartialEq, Eq, PartialOrd, Hash)]
#[derive(Debug, 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 @@ -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]`
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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")]
);
}
Expand All @@ -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]
);
}
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)
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,
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,
op.clone(),
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(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)
}

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
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(
children[0].clone(),
self.op,
self.op.clone(),
children[1].clone(),
)))
}
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 @@ -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,
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
6 changes: 5 additions & 1 deletion datafusion/proto/src/logical_plan/from_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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."))
}
Expand Down
14 changes: 12 additions & 2 deletions datafusion/substrait/src/logical_plan/producer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down Expand Up @@ -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,
Expand Down