alamb commented on code in PR #10358:
URL: https://github.com/apache/datafusion/pull/10358#discussion_r1590280001
##########
datafusion/core/tests/simplification.rs:
##########
@@ -658,3 +681,11 @@ fn test_simplify_concat() {
let expected = concat(vec![col("c0"), lit("hello rust"), col("c1")]);
test_simplify(expr, expected)
}
+#[test]
+fn test_simplify_cycles() {
+ // cast(now() as int64) < cast(to_timestamp(0) as int64) + i64::MAX
+ let expr = cast(now(), DataType::Int64)
+ .lt(cast(to_timestamp(vec![lit(0)]), DataType::Int64) + lit(i64::MAX));
+ let expected = lit(true);
+ test_simplify_with_cycle_count(expr, expected, 3);
Review Comment:
nice
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -3575,4 +3604,48 @@ mod tests {
assert_eq!(simplify(expr), expected);
}
+
+ #[test]
+ fn test_simplify_iterations() {
+ // TRUE
+ let expr = lit(true);
+ let expected = lit(true);
+ let (expr, num_iter) = simplify_count(expr);
+ assert_eq!(expr, expected);
+ assert_eq!(num_iter, 1);
+
+ // (true != NULL) OR (5 > 10)
+ let expr = lit(true).not_eq(lit_bool_null()).or(lit(5).gt(lit(10)));
+ let expected = lit_bool_null();
+ let (expr, num_iter) = simplify_count(expr);
+ assert_eq!(expr, expected);
+ assert_eq!(num_iter, 2);
+
+ // cast(now() as int64) < cast(to_timestamp(0) as int64) + i64::MAX
+ let expr = cast(now(), DataType::Int64)
+ .lt(cast(to_timestamp(vec![lit(0)]), DataType::Int64) +
lit(i64::MAX));
+ let expected = lit(true);
+ let (expr, num_iter) = simplify_count(expr);
+ assert_eq!(expr, expected);
+ assert_eq!(num_iter, 3);
+
+ // NOTE: this currently does not simplify
+ // (((c4 - 10) + 10) *100) / 100
Review Comment:
I agree the simplifier needs to be made more sophisticated to handle some of
these cases It might be cool to add a ticket explaining the case
Maybe it is something like `(<expr> + CONST) + CONST)` --> `<expr> + (CONST
+ CONST)` (basically apply associative rules
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -323,6 +337,63 @@ impl<S: SimplifyInfo> ExprSimplifier<S> {
self.canonicalize = canonicalize;
self
}
+
+ /// Specifies the maximum number of simplification cycles to run.
Review Comment:
😍
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -181,24 +196,23 @@ impl<S: SimplifyInfo> ExprSimplifier<S> {
expr = expr.rewrite(&mut Canonicalizer::new()).data()?
}
- // TODO iterate until no changes are made during rewrite
- // (evaluating constants can enable new simplifications and
- // simplifications can enable new constant evaluation)
- // https://github.com/apache/datafusion/issues/1160
- expr.rewrite(&mut const_evaluator)
- .data()?
- .rewrite(&mut simplifier)
- .data()?
- .rewrite(&mut guarantee_rewriter)
- .data()?
- // run both passes twice to try an minimize simplifications that
we missed
- .rewrite(&mut const_evaluator)
- .data()?
- .rewrite(&mut simplifier)
- .data()?
- // shorten inlist should be started after other inlist rules are
applied
- .rewrite(&mut shorten_in_list_simplifier)
- .data()
+ let mut num_cycles = 0;
Review Comment:
I think it is important to provide context in comments when it might not be
obvious *why* code is doing something. In this caseL
```suggestion
// Evaluating constants can enable new simplifications and
// simplifications can enable new constant evaluation
// see `Self::with_max_cycles`
let mut num_cycles = 0;
```
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
