capkurmagati commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-datafusion/pull/1376#discussion_r763984743
##########
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:
> I guess I was thinking the plan tests are for ensuring that simplify
is being called correctly on the expressions in the plan, rather than testing
the various Expr corner cases of the simplification logic itself
Thanks for the explanation. Makes sense to me.
--
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]
