xudong963 commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-datafusion/pull/1376#discussion_r763079098
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -342,7 +342,8 @@ impl SimplifyExpressions {
/// Partially evaluate `Expr`s so constant subtrees are evaluated at plan time.
///
-/// Note it does not handle algebriac rewrites such as `(a and false)` --> `a`
+/// Note it does not handle algebriac rewrites such as `(a and false)`
Review comment:
```suggestion
/// Note it does not handle algebraic rewrites such as `(a or false)`
```
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -785,180 +786,166 @@ mod tests {
use crate::physical_plan::udf::ScalarUDF;
#[test]
- fn test_simplify_or_true() -> Result<()> {
- let expr_a = col("c").or(lit(true));
- let expr_b = lit(true).or(col("c"));
+ fn test_simplify_or_true() {
+ let expr_a = col("c2").or(lit(true));
+ let expr_b = lit(true).or(col("c2"));
let expected = lit(true);
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_or_false() -> Result<()> {
- let expr_a = lit(false).or(col("c"));
- let expr_b = col("c").or(lit(false));
- let expected = col("c");
+ fn test_simplify_or_false() {
+ let expr_a = lit(false).or(col("c2"));
+ let expr_b = col("c2").or(lit(false));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_or_same() -> Result<()> {
- let expr = col("c").or(col("c"));
- let expected = col("c");
+ fn test_simplify_or_same() {
+ let expr = col("c2").or(col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_false() -> Result<()> {
- let expr_a = lit(false).and(col("c"));
- let expr_b = col("c").and(lit(false));
+ fn test_simplify_and_false() {
+ let expr_a = lit(false).and(col("c2"));
+ let expr_b = col("c2").and(lit(false));
let expected = lit(false);
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_same() -> Result<()> {
- let expr = col("c").and(col("c"));
- let expected = col("c");
+ fn test_simplify_and_same() {
+ let expr = col("c2").and(col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_true() -> Result<()> {
- let expr_a = lit(true).and(col("c"));
- let expr_b = col("c").and(lit(true));
- let expected = col("c");
+ fn test_simplify_and_true() {
+ let expr_a = lit(true).and(col("c2"));
+ let expr_b = col("c2").and(lit(true));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_multiply_by_one() -> Result<()> {
- let expr_a = binary_expr(col("c"), Operator::Multiply, lit(1));
- let expr_b = binary_expr(lit(1), Operator::Multiply, col("c"));
- let expected = col("c");
+ fn test_simplify_multiply_by_one() {
Review comment:
Maybe you can also add `test_simlify_multiply_by_zero` -> 0 ？
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -785,180 +786,166 @@ mod tests {
use crate::physical_plan::udf::ScalarUDF;
#[test]
- fn test_simplify_or_true() -> Result<()> {
- let expr_a = col("c").or(lit(true));
- let expr_b = lit(true).or(col("c"));
+ fn test_simplify_or_true() {
+ let expr_a = col("c2").or(lit(true));
+ let expr_b = lit(true).or(col("c2"));
let expected = lit(true);
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_or_false() -> Result<()> {
- let expr_a = lit(false).or(col("c"));
- let expr_b = col("c").or(lit(false));
- let expected = col("c");
+ fn test_simplify_or_false() {
+ let expr_a = lit(false).or(col("c2"));
+ let expr_b = col("c2").or(lit(false));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_or_same() -> Result<()> {
- let expr = col("c").or(col("c"));
- let expected = col("c");
+ fn test_simplify_or_same() {
+ let expr = col("c2").or(col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_false() -> Result<()> {
- let expr_a = lit(false).and(col("c"));
- let expr_b = col("c").and(lit(false));
+ fn test_simplify_and_false() {
+ let expr_a = lit(false).and(col("c2"));
+ let expr_b = col("c2").and(lit(false));
let expected = lit(false);
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_same() -> Result<()> {
- let expr = col("c").and(col("c"));
- let expected = col("c");
+ fn test_simplify_and_same() {
+ let expr = col("c2").and(col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_true() -> Result<()> {
- let expr_a = lit(true).and(col("c"));
- let expr_b = col("c").and(lit(true));
- let expected = col("c");
+ fn test_simplify_and_true() {
+ let expr_a = lit(true).and(col("c2"));
+ let expr_b = col("c2").and(lit(true));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_multiply_by_one() -> Result<()> {
- let expr_a = binary_expr(col("c"), Operator::Multiply, lit(1));
- let expr_b = binary_expr(lit(1), Operator::Multiply, col("c"));
- let expected = col("c");
+ fn test_simplify_multiply_by_one() {
+ let expr_a = binary_expr(col("c2"), Operator::Multiply, lit(1));
+ let expr_b = binary_expr(lit(1), Operator::Multiply, col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_divide_by_one() -> Result<()> {
- let expr = binary_expr(col("c"), Operator::Divide, lit(1));
- let expected = col("c");
+ fn test_simplify_divide_by_one() {
+ let expr = binary_expr(col("c2"), Operator::Divide, lit(1));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_divide_by_same() -> Result<()> {
- let expr = binary_expr(col("c"), Operator::Divide, col("c"));
+ fn test_simplify_divide_by_same() {
+ let expr = binary_expr(col("c2"), Operator::Divide, col("c2"));
let expected = lit(1);
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_simple_and() -> Result<()> {
+ fn test_simplify_simple_and() {
// (c > 5) AND (c > 5)
- let expr = (col("c").gt(lit(5))).and(col("c").gt(lit(5)));
- let expected = col("c").gt(lit(5));
+ let expr = (col("c2").gt(lit(5))).and(col("c2").gt(lit(5)));
+ let expected = col("c2").gt(lit(5));
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_composed_and() -> Result<()> {
+ fn test_simplify_composed_and() {
// ((c > 5) AND (d < 6)) AND (c > 5)
let expr = binary_expr(
- binary_expr(col("c").gt(lit(5)), Operator::And,
col("d").lt(lit(6))),
+ binary_expr(col("c2").gt(lit(5)), Operator::And,
col("d").lt(lit(6))),
Operator::And,
- col("c").gt(lit(5)),
+ col("c2").gt(lit(5)),
);
let expected =
- binary_expr(col("c").gt(lit(5)), Operator::And,
col("d").lt(lit(6)));
+ binary_expr(col("c2").gt(lit(5)), Operator::And,
col("d").lt(lit(6)));
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_negated_and() -> Result<()> {
+ fn test_simplify_negated_and() {
Review comment:
Why can't be simplified to `false`?
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -785,180 +786,166 @@ mod tests {
use crate::physical_plan::udf::ScalarUDF;
#[test]
- fn test_simplify_or_true() -> Result<()> {
- let expr_a = col("c").or(lit(true));
- let expr_b = lit(true).or(col("c"));
+ fn test_simplify_or_true() {
+ let expr_a = col("c2").or(lit(true));
+ let expr_b = lit(true).or(col("c2"));
let expected = lit(true);
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_or_false() -> Result<()> {
- let expr_a = lit(false).or(col("c"));
- let expr_b = col("c").or(lit(false));
- let expected = col("c");
+ fn test_simplify_or_false() {
+ let expr_a = lit(false).or(col("c2"));
+ let expr_b = col("c2").or(lit(false));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_or_same() -> Result<()> {
- let expr = col("c").or(col("c"));
- let expected = col("c");
+ fn test_simplify_or_same() {
+ let expr = col("c2").or(col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_false() -> Result<()> {
- let expr_a = lit(false).and(col("c"));
- let expr_b = col("c").and(lit(false));
+ fn test_simplify_and_false() {
+ let expr_a = lit(false).and(col("c2"));
+ let expr_b = col("c2").and(lit(false));
let expected = lit(false);
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_same() -> Result<()> {
- let expr = col("c").and(col("c"));
- let expected = col("c");
+ fn test_simplify_and_same() {
+ let expr = col("c2").and(col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_true() -> Result<()> {
- let expr_a = lit(true).and(col("c"));
- let expr_b = col("c").and(lit(true));
- let expected = col("c");
+ fn test_simplify_and_true() {
+ let expr_a = lit(true).and(col("c2"));
+ let expr_b = col("c2").and(lit(true));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_multiply_by_one() -> Result<()> {
- let expr_a = binary_expr(col("c"), Operator::Multiply, lit(1));
- let expr_b = binary_expr(lit(1), Operator::Multiply, col("c"));
- let expected = col("c");
+ fn test_simplify_multiply_by_one() {
+ let expr_a = binary_expr(col("c2"), Operator::Multiply, lit(1));
+ let expr_b = binary_expr(lit(1), Operator::Multiply, col("c2"));
+ let expected = col("c2");
assert_eq!(simplify(&expr_a), expected);
assert_eq!(simplify(&expr_b), expected);
- Ok(())
}
#[test]
- fn test_simplify_divide_by_one() -> Result<()> {
- let expr = binary_expr(col("c"), Operator::Divide, lit(1));
- let expected = col("c");
+ fn test_simplify_divide_by_one() {
+ let expr = binary_expr(col("c2"), Operator::Divide, lit(1));
+ let expected = col("c2");
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_divide_by_same() -> Result<()> {
- let expr = binary_expr(col("c"), Operator::Divide, col("c"));
+ fn test_simplify_divide_by_same() {
+ let expr = binary_expr(col("c2"), Operator::Divide, col("c2"));
let expected = lit(1);
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_simple_and() -> Result<()> {
+ fn test_simplify_simple_and() {
// (c > 5) AND (c > 5)
- let expr = (col("c").gt(lit(5))).and(col("c").gt(lit(5)));
- let expected = col("c").gt(lit(5));
+ let expr = (col("c2").gt(lit(5))).and(col("c2").gt(lit(5)));
+ let expected = col("c2").gt(lit(5));
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_composed_and() -> Result<()> {
+ fn test_simplify_composed_and() {
// ((c > 5) AND (d < 6)) AND (c > 5)
let expr = binary_expr(
- binary_expr(col("c").gt(lit(5)), Operator::And,
col("d").lt(lit(6))),
+ binary_expr(col("c2").gt(lit(5)), Operator::And,
col("d").lt(lit(6))),
Operator::And,
- col("c").gt(lit(5)),
+ col("c2").gt(lit(5)),
);
let expected =
- binary_expr(col("c").gt(lit(5)), Operator::And,
col("d").lt(lit(6)));
+ binary_expr(col("c2").gt(lit(5)), Operator::And,
col("d").lt(lit(6)));
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_negated_and() -> Result<()> {
+ fn test_simplify_negated_and() {
// (c > 5) AND !(c > 5) -- can't remove
let expr = binary_expr(
- col("c").gt(lit(5)),
+ col("c2").gt(lit(5)),
Operator::And,
- Expr::not(col("c").gt(lit(5))),
+ Expr::not(col("c2").gt(lit(5))),
);
let expected = expr.clone();
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_or_and() -> Result<()> {
+ fn test_simplify_or_and() {
// (c > 5) OR ((d < 6) AND (c > 5) -- can remove
let expr = binary_expr(
- col("c").gt(lit(5)),
+ col("c2").gt(lit(5)),
Operator::Or,
- binary_expr(col("d").lt(lit(6)), Operator::And,
col("c").gt(lit(5))),
+ binary_expr(col("d").lt(lit(6)), Operator::And,
col("c2").gt(lit(5))),
);
- let expected = col("c").gt(lit(5));
+ let expected = col("c2").gt(lit(5));
assert_eq!(simplify(&expr), expected);
- Ok(())
}
#[test]
- fn test_simplify_and_and_false() -> Result<()> {
- let expr =
- binary_expr(lit(ScalarValue::Boolean(None)), Operator::And,
lit(false));
+ fn test_simplify_and_and_false() {
Review comment:
Maybe it should be `test_simplify_null_and_false`?
--
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]
