alamb commented on a change in pull request #1401:
URL: https://github.com/apache/arrow-datafusion/pull/1401#discussion_r775022437
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -1311,44 +1197,25 @@ mod tests {
assert_eq!(col("c1").get_type(&schema).unwrap(), DataType::Utf8);
assert_eq!(
- do_simplify(col("c1").not_eq(lit(true))),
- col("c1").not_eq(lit(true)),
- );
-
- assert_eq!(
Review comment:
this was removed for the same reason as the tests above - they don't
make sense to compare a string column to a boolean constant (it will get a run
time error)
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -930,16 +826,16 @@ mod tests {
let expr_plus = binary_expr(null.clone(), Operator::Divide,
null.clone());
let expr_eq = null;
- assert_eq!(simplify(&expr_plus), expr_eq);
+ assert_eq!(simplify(expr_plus), expr_eq);
}
#[test]
- fn test_simplify_do_not_simplify_arithmetic_expr() {
Review comment:
not sure why this test was here -- now that constant propagation is
implemented, these cases do indeed work
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -553,213 +388,274 @@ impl<'a> Simplifier<'a> {
false
}
+}
- fn boolean_folding_for_or(
- const_bool: &Option<bool>,
- bool_expr: Box<Expr>,
- left_right_order: bool,
- ) -> Expr {
- // See if we can fold 'const_bool OR bool_expr' to a constant boolean
- match const_bool {
- // TRUE or expr (including NULL) = TRUE
- Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
- // FALSE or expr (including NULL) = expr
- Some(false) => *bool_expr,
- None => match *bool_expr {
- // NULL or TRUE = TRUE
- Expr::Literal(ScalarValue::Boolean(Some(true))) => {
- Expr::Literal(ScalarValue::Boolean(Some(true)))
+fn boolean_folding_for_or(
+ const_bool: Option<bool>,
+ bool_expr: Expr,
+ left_right_order: bool,
+) -> Expr {
+ use Expr::*;
+ use ScalarValue::Boolean;
+
+ // See if we can fold 'const_bool OR bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE or expr (including NULL) = TRUE
+ Some(true) => lit(true),
+ // FALSE or expr (including NULL) = expr
+ Some(false) => bool_expr,
+ None => match bool_expr {
+ // NULL or TRUE = TRUE
+ Literal(Boolean(Some(true))) => lit(true),
+ // NULL or FALSE = NULL
+ Literal(Boolean(Some(false))) => lit_null(),
+ // NULL or NULL = NULL
+ Literal(Boolean(None)) => lit_null(),
+ // NULL or expr can be either NULL or TRUE
+ // So let us not rewrite it
+ _ => {
+ let mut left = Literal(Boolean(const_bool));
+ let mut right = bool_expr;
+ if !left_right_order {
+ std::mem::swap(&mut left, &mut right);
}
- // NULL or FALSE = NULL
- Expr::Literal(ScalarValue::Boolean(Some(false))) => {
- Expr::Literal(ScalarValue::Boolean(None))
- }
- // NULL or NULL = NULL
- Expr::Literal(ScalarValue::Boolean(None)) => {
- Expr::Literal(ScalarValue::Boolean(None))
- }
- // NULL or expr can be either NULL or TRUE
- // So let us not rewrite it
- _ => {
- let mut left =
-
Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
- let mut right = bool_expr;
- if !left_right_order {
- std::mem::swap(&mut left, &mut right);
- }
- Expr::BinaryExpr {
- left,
- op: Operator::Or,
- right,
- }
+ BinaryExpr {
+ left: Box::new(left),
+ op: Operator::Or,
+ right: Box::new(right),
}
- },
- }
+ }
+ },
}
+}
- fn boolean_folding_for_and(
- const_bool: &Option<bool>,
- bool_expr: Box<Expr>,
- left_right_order: bool,
- ) -> Expr {
- // See if we can fold 'const_bool AND bool_expr' to a constant boolean
- match const_bool {
- // TRUE and expr (including NULL) = expr
- Some(true) => *bool_expr,
- // FALSE and expr (including NULL) = FALSE
- Some(false) => Expr::Literal(ScalarValue::Boolean(Some(false))),
- None => match *bool_expr {
- // NULL and TRUE = NULL
- Expr::Literal(ScalarValue::Boolean(Some(true))) => {
- Expr::Literal(ScalarValue::Boolean(None))
+fn boolean_folding_for_and(
+ const_bool: Option<bool>,
+ bool_expr: Expr,
+ left_right_order: bool,
+) -> Expr {
+ use Expr::*;
+ use ScalarValue::Boolean;
+
+ // See if we can fold 'const_bool AND bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE and expr (including NULL) = expr
+ Some(true) => bool_expr,
+ // FALSE and expr (including NULL) = FALSE
+ Some(false) => lit(false),
+ None => match bool_expr {
+ // NULL and TRUE = NULL
+ Literal(Boolean(Some(true))) => lit_null(),
+ // NULL and FALSE = FALSE
+ Literal(Boolean(Some(false))) => lit(false),
+ // NULL and NULL = NULL
+ Literal(Boolean(None)) => lit_null(),
+ // NULL and expr can either be NULL or FALSE
+ // So let us not rewrite it
+ _ => {
+ let mut left = Literal(Boolean(const_bool));
+ let mut right = bool_expr;
+ if !left_right_order {
+ std::mem::swap(&mut left, &mut right);
}
- // NULL and FALSE = FALSE
- Expr::Literal(ScalarValue::Boolean(Some(false))) => {
- Expr::Literal(ScalarValue::Boolean(Some(false)))
- }
- // NULL and NULL = NULL
- Expr::Literal(ScalarValue::Boolean(None)) => {
- Expr::Literal(ScalarValue::Boolean(None))
- }
- // NULL and expr can either be NULL or FALSE
- // So let us not rewrite it
- _ => {
- let mut left =
-
Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
- let mut right = bool_expr;
- if !left_right_order {
- std::mem::swap(&mut left, &mut right);
- }
- Expr::BinaryExpr {
- left,
- op: Operator::And,
- right,
- }
+ BinaryExpr {
+ left: Box::new(left),
+ op: Operator::And,
+ right: Box::new(right),
}
- },
- }
+ }
+ },
}
}
impl<'a> ExprRewriter for Simplifier<'a> {
/// rewrite the expression simplifying any constant expressions
fn mutate(&mut self, expr: Expr) -> Result<Expr> {
+ use Expr::*;
+ use Operator::{And, Divide, Eq, Multiply, NotEq, Or};
+
let new_expr = match expr {
- Expr::BinaryExpr { left, op, right } => match op {
- Operator::Eq => match (left.as_ref(), right.as_ref()) {
- (
- Expr::Literal(ScalarValue::Boolean(l)),
- Expr::Literal(ScalarValue::Boolean(r)),
- ) => match (l, r) {
- (Some(l), Some(r)) => {
- Expr::Literal(ScalarValue::Boolean(Some(l == r)))
- }
- _ => Expr::Literal(ScalarValue::Boolean(None)),
- },
- (Expr::Literal(ScalarValue::Boolean(b)), _)
- if self.is_boolean_type(&right) =>
- {
- match b {
- Some(true) => *right,
- Some(false) => Expr::Not(right),
- None => Expr::Literal(ScalarValue::Boolean(None)),
- }
- }
- (_, Expr::Literal(ScalarValue::Boolean(b)))
- if self.is_boolean_type(&left) =>
- {
- match b {
- Some(true) => *left,
- Some(false) => Expr::Not(left),
- None => Expr::Literal(ScalarValue::Boolean(None)),
- }
- }
- _ => Expr::BinaryExpr {
- left,
- op: Operator::Eq,
- right,
- },
- },
- Operator::NotEq => match (left.as_ref(), right.as_ref()) {
- (
- Expr::Literal(ScalarValue::Boolean(l)),
- Expr::Literal(ScalarValue::Boolean(r)),
- ) => match (l, r) {
- (Some(l), Some(r)) => {
- Expr::Literal(ScalarValue::Boolean(Some(l != r)))
- }
- _ => Expr::Literal(ScalarValue::Boolean(None)),
- },
- (Expr::Literal(ScalarValue::Boolean(b)), _)
- if self.is_boolean_type(&right) =>
- {
- match b {
- Some(true) => Expr::Not(right),
- Some(false) => *right,
- None => Expr::Literal(ScalarValue::Boolean(None)),
- }
- }
- (_, Expr::Literal(ScalarValue::Boolean(b)))
- if self.is_boolean_type(&left) =>
- {
- match b {
- Some(true) => Expr::Not(left),
- Some(false) => *left,
- None => Expr::Literal(ScalarValue::Boolean(None)),
- }
- }
- _ => Expr::BinaryExpr {
- left,
- op: Operator::NotEq,
- right,
- },
- },
- Operator::Or => match (left.as_ref(), right.as_ref()) {
- (Expr::Literal(ScalarValue::Boolean(b)), _)
- if self.is_boolean_type(&right) =>
- {
- Self::boolean_folding_for_or(b, right, true)
- }
- (_, Expr::Literal(ScalarValue::Boolean(b)))
- if self.is_boolean_type(&left) =>
- {
- Self::boolean_folding_for_or(b, left, false)
- }
- _ => Expr::BinaryExpr {
- left,
- op: Operator::Or,
- right,
- },
- },
- Operator::And => match (left.as_ref(), right.as_ref()) {
- (Expr::Literal(ScalarValue::Boolean(b)), _)
- if self.is_boolean_type(&right) =>
- {
- Self::boolean_folding_for_and(b, right, true)
- }
- (_, Expr::Literal(ScalarValue::Boolean(b)))
- if self.is_boolean_type(&left) =>
- {
- Self::boolean_folding_for_and(b, left, false)
- }
- _ => Expr::BinaryExpr {
- left,
- op: Operator::And,
- right,
- },
- },
- _ => Expr::BinaryExpr { left, op, right },
- },
- // Not(Not(expr)) --> expr
- Expr::Not(inner) => {
- if let Expr::Not(negated_inner) = *inner {
- *negated_inner
- } else {
- Expr::Not(inner)
+ //
Review comment:
Now the simplification rules are in one standardized match statement
which I find easier to understand what is going on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
