You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/12/06 15:32:56 UTC
[GitHub] [arrow-datafusion] xudong963 commented on a change in pull request #1376: Make tests for `simplify` and `Simplifer` consistent
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org