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 14:15:01 UTC
[GitHub] [arrow-datafusion] capkurmagati commented on a change in pull request #1376: Make tests for `simplify` and `Simplifer` consistent
capkurmagati commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-datafusion/pull/1376#discussion_r763041673
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -1191,212 +1178,171 @@ mod tests {
test_evaluate_with_start_time(input_expr, expected_expr, &Utc::now())
}
- #[test]
- fn simplify_expr_not_not() -> Result<()> {
- let schema = expr_test_schema();
- let mut rewriter = Simplifier {
- schemas: vec![&schema],
- };
+ // ------------------------------
+ // ----- Simplifier tests -------
+ // ------------------------------
- assert_eq!(
- (col("c2").not().not().not()).rewrite(&mut rewriter)?,
- col("c2").not(),
- );
+ // TODO rename to simplify
+ fn do_simplify(expr: Expr) -> Expr {
+ let schema = expr_test_schema();
+ let mut rewriter = Simplifier::new(vec![&schema]);
+ expr.rewrite(&mut rewriter).expect("expected to simplify")
+ }
- Ok(())
+ fn expr_test_schema() -> DFSchemaRef {
+ Arc::new(
+ DFSchema::new(vec![
+ DFField::new(None, "c1", DataType::Utf8, true),
+ DFField::new(None, "c2", DataType::Boolean, true),
+ ])
+ .unwrap(),
+ )
}
#[test]
- fn simplify_expr_null_comparison() -> Result<()> {
- let schema = expr_test_schema();
- let mut rewriter = Simplifier {
- schemas: vec![&schema],
- };
+ fn simplify_expr_not_not() {
+ assert_eq!(do_simplify(col("c2").not().not().not()), col("c2").not(),);
+ }
+ #[test]
+ fn simplify_expr_null_comparison() {
// x = null is always null
assert_eq!(
- (lit(true).eq(lit(ScalarValue::Boolean(None)))).rewrite(&mut rewriter)?,
+ do_simplify(lit(true).eq(lit(ScalarValue::Boolean(None)))),
lit(ScalarValue::Boolean(None)),
);
// null != null is always null
assert_eq!(
- (lit(ScalarValue::Boolean(None)).not_eq(lit(ScalarValue::Boolean(None))))
- .rewrite(&mut rewriter)?,
+ do_simplify(
+ lit(ScalarValue::Boolean(None)).not_eq(lit(ScalarValue::Boolean(None)))
+ ),
lit(ScalarValue::Boolean(None)),
);
// x != null is always null
assert_eq!(
- (col("c2").not_eq(lit(ScalarValue::Boolean(None)))).rewrite(&mut rewriter)?,
+ do_simplify(col("c2").not_eq(lit(ScalarValue::Boolean(None)))),
lit(ScalarValue::Boolean(None)),
);
// null = x is always null
assert_eq!(
- (lit(ScalarValue::Boolean(None)).eq(col("c2"))).rewrite(&mut rewriter)?,
+ do_simplify(lit(ScalarValue::Boolean(None)).eq(col("c2"))),
lit(ScalarValue::Boolean(None)),
);
-
- Ok(())
}
#[test]
- fn simplify_expr_eq() -> Result<()> {
+ fn simplify_expr_eq() {
let schema = expr_test_schema();
- let mut rewriter = Simplifier {
- schemas: vec![&schema],
- };
-
- assert_eq!(col("c2").get_type(&schema)?, DataType::Boolean);
+ assert_eq!(col("c2").get_type(&schema).unwrap(), DataType::Boolean);
// true = ture -> true
- assert_eq!((lit(true).eq(lit(true))).rewrite(&mut rewriter)?, lit(true),);
+ assert_eq!(do_simplify(lit(true).eq(lit(true))), lit(true));
// true = false -> false
- assert_eq!(
- (lit(true).eq(lit(false))).rewrite(&mut rewriter)?,
- lit(false),
- );
+ assert_eq!(do_simplify(lit(true).eq(lit(false))), lit(false),);
// c2 = true -> c2
- assert_eq!((col("c2").eq(lit(true))).rewrite(&mut rewriter)?, col("c2"),);
+ assert_eq!(do_simplify(col("c2").eq(lit(true))), col("c2"));
// c2 = false => !c2
- assert_eq!(
- (col("c2").eq(lit(false))).rewrite(&mut rewriter)?,
- col("c2").not(),
- );
-
- Ok(())
+ assert_eq!(do_simplify(col("c2").eq(lit(false))), col("c2").not(),);
}
#[test]
- fn simplify_expr_eq_skip_nonboolean_type() -> Result<()> {
+ fn simplify_expr_eq_skip_nonboolean_type() {
let schema = expr_test_schema();
- let mut rewriter = Simplifier {
- schemas: vec![&schema],
- };
- // When one of the operand is not of boolean type, folding the other boolean constant will
- // change return type of expression to non-boolean.
+ // When one of the operand is not of boolean type, folding the
+ // other boolean constant will change return type of
+ // expression to non-boolean.
//
// Make sure c1 column to be used in tests is not boolean type
- assert_eq!(col("c1").get_type(&schema)?, DataType::Utf8);
+ assert_eq!(col("c1").get_type(&schema).unwrap(), DataType::Utf8);
Review comment:
Shall we add a test for nonboolean type for logical plan? Something like
```rust
#[test]
fn test_simplity_skip_nonboolean_type() {
let table_scan = test_table_scan();
let plan = LogicalPlanBuilder::from(table_scan)
.filter(col("d").eq(lit(false)).not())
.unwrap()
.project(vec![col("a")])
.unwrap()
.build()
.unwrap();
let expected = "\
Projection: #test.a\
\n Filter: NOT #test.d = Boolean(false)\
\n TableScan: test projection=None";
assert_optimized_plan_eq(&plan, expected);
}
```
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -1191,212 +1178,171 @@ mod tests {
test_evaluate_with_start_time(input_expr, expected_expr, &Utc::now())
}
- #[test]
- fn simplify_expr_not_not() -> Result<()> {
- let schema = expr_test_schema();
- let mut rewriter = Simplifier {
- schemas: vec![&schema],
- };
+ // ------------------------------
+ // ----- Simplifier tests -------
+ // ------------------------------
- assert_eq!(
- (col("c2").not().not().not()).rewrite(&mut rewriter)?,
- col("c2").not(),
- );
+ // TODO rename to simplify
+ fn do_simplify(expr: Expr) -> Expr {
Review comment:
👍 and looks ready to rename?
##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -1406,62 +1352,40 @@ mod tests {
else_expr: Some(Box::new(col("c2"))),
}
);
+ }
- Ok(())
+ fn lit_true() -> Expr {
Review comment:
I wonder you intended to use `lit_true()` and `lit(true)` in different situations.
--
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