peter-toth commented on code in PR #13128:
URL: https://github.com/apache/datafusion/pull/13128#discussion_r1818577627
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1681,9 +1679,15 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for
Simplifier<'a, S> {
}
}
-fn has_common_conjunction(lhs: &Expr, rhs: &Expr) -> bool {
+fn has_common_conjunction(lhs: &Expr, rhs: &Expr) -> Result<bool,
DataFusionError> {
let lhs: HashSet<&Expr> = iter_conjunction(lhs).collect();
- iter_conjunction(rhs).any(|e| lhs.contains(&e))
+ iter_conjunction(rhs).try_fold(false, |acc, e| {
Review Comment:
This still looks a bit ugly. I mean, we just look for the first non-volatile
expression and don't want to iterate over all expressions.
Actually, I think I made a mistake when I added `is_volatile()` with return
type `Result<bool>`:
https://github.com/apache/datafusion/blob/e22d23113f549a90483bbf161add84eba08510fd/datafusion/expr/src/expr.rs#L1581-L1583
As the closure we provide to `exists()` can't return an error it would be
safe to unwrap the result and return only `bool`.
@alamb, do you think we can fix `Expr::is_volatile()`, it would be an API
breaking change though, to make it simpler and so we can keep using `any()`
here?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
