alamb commented on code in PR #10358:
URL: https://github.com/apache/datafusion/pull/10358#discussion_r1588472598
##########
datafusion/core/tests/simplification.rs:
##########
@@ -658,3 +658,18 @@ fn test_simplify_concat() {
let expected = concat(vec![col("c0"), lit("hello rust"), col("c1")]);
test_simplify(expr, expected)
}
+
+#[test]
+fn test_simplify_iterations() {
Review Comment:
does this test fail without the change?
Maybe we could add the test from the ticket too:
https://github.com/apache/datafusion/issues/1160#issuecomment-952905624
@devinjdangelo 's deeply nested expressions usecase would also be
interesting to test:
https://github.com/apache/datafusion/issues/1160#issuecomment-1986906627
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -107,6 +110,7 @@ impl<S: SimplifyInfo> ExprSimplifier<S> {
info,
guarantees: vec![],
canonicalize: true,
+ max_simplifier_iterations: DEFAULT_MAX_SIMPLIFIER_ITERATIONS,
Review Comment:
Perhaps we could have an API to update it to mirror
```rust
pub fn with_canonicalize(mut self, canonicalize: bool) -> Self {
self.canonicalize = canonicalize;
self
}
```
Something like
```rust
pub fn with_max_simplifier_iterations(mut self,
max_simplifier_iterations: usize) -> Self {
self.max_simplifier_iterations = max_simplifier_iterations;
self
}
```
Perhaps
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -181,24 +185,30 @@ 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()?
+ let mut i = 0;
+ loop {
Review Comment:
I think you can write this loop more concisely using the
`Transformed::transform_data` like this (BTW this is confusing, I made a PR
jsut today to try and clarify it here
https://github.com/apache/datafusion/pull/10355)
```rust
let mut i = 0;
loop {
let result = expr.rewrite(&mut const_evaluator)?
.transform_data(|expr| expr.rewrite(&mut simplifier))?
.transform_data(|expr| expr.rewrite(&mut
guarantee_rewriter))?
// shorten inlist should be started after other inlist rules
are applied
.transform_data(|expr| expr.rewrite(&mut
shorten_in_list_simplifier))?;
i += 1;
if !result.transformed || i >= self.max_simplifier_iterations {
return Ok(result.data);
}
}
}
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -181,24 +185,30 @@ 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()?
+ let mut i = 0;
+ loop {
+ let result = expr.rewrite(&mut const_evaluator)?;
+ let mut transformed = result.transformed;
+ expr = result.data;
+
+ let result = expr.rewrite(&mut simplifier)?;
+ transformed |= result.transformed;
+ expr = result.data;
+
+ let result = expr.rewrite(&mut guarantee_rewriter)?;
+ transformed |= result.transformed;
+ expr = result.data;
+
// shorten inlist should be started after other inlist rules are
applied
- .rewrite(&mut shorten_in_list_simplifier)
- .data()
+ let result = expr.rewrite(&mut shorten_in_list_simplifier)?;
+ transformed |= result.transformed;
+ expr = result.data;
+
+ i += 1;
+ if !transformed || i >= self.max_simplifier_iterations {
+ return Ok(expr);
Review Comment:
Maybe we could add a function like `simplify_inner` that did and then use
that in tests
```rust
pub fn simplify(&self, mut expr: Expr) -> Result<Expr> {
self.simplify_inner(expr).0
}
/// Returns the simplified expr and the number of iterations required.
fn simplify_inner(&self, mut expr: Expr) -> Result<(usize, Expr)> {
```
--
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
