c21 commented on a change in pull request #31318:
URL: https://github.com/apache/spark/pull/31318#discussion_r580103906
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -366,22 +367,34 @@ object BooleanSimplification extends Rule[LogicalPlan]
with PredicateHelper {
// ((c || ...) && (d || ...)) || a || b
(common :+ And(ldiff.reduce(Or), rdiff.reduce(Or))).reduce(Or)
}
+ } else {
+ // No common factors from disjunctive predicates, reduce common
factor from conjunction
+ val all = splitConjunctivePredicates(left) ++
splitConjunctivePredicates(right)
+ val distinct = ExpressionSet(all)
+ if (all.size == distinct.size) {
+ // No common factors, return the original predicate
+ and
+ } else {
+ // (((a && b) && a && (a && c))) => a && b && c
+ distinct.reduce(And)
+ }
}
// Common factor elimination for disjunction
case or @ (left Or right) =>
// 1. Split left and right to get the conjunctive predicates,
- // i.e. lhs = (a, b), rhs = (a, c)
+ // i.e. lhs = (a, b), rhs = (a, c)
Review comment:
nit: similarly to above, could we change to `lhs = (a && b), rhs = (a &&
c)`?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -366,22 +367,34 @@ object BooleanSimplification extends Rule[LogicalPlan]
with PredicateHelper {
// ((c || ...) && (d || ...)) || a || b
Review comment:
nit: given we are here now, it seems that expression order is not right.
Shall we update to
```
a || b || ((c || ...) && (d || ...))
```
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -366,6 +363,16 @@ object BooleanSimplification extends Rule[LogicalPlan]
with PredicateHelper {
// ((c || ...) && (d || ...)) || a || b
(common :+ And(ldiff.reduce(Or), rdiff.reduce(Or))).reduce(Or)
}
+ } else {
+ // No common factors from disjunctive predicates, reduce common
factor from conjunction
+ val all = splitConjunctivePredicates(left) ++
splitConjunctivePredicates(right)
Review comment:
It seems that we are improving to handle conjunctive and disjunctive
cases separately. I am wondering could we do a better job to handle the
expression with mix of `&&` and `||`?
e.g.
```
(a || b) && (a && b)
```
to
```
a && b
```
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -366,22 +367,34 @@ object BooleanSimplification extends Rule[LogicalPlan]
with PredicateHelper {
// ((c || ...) && (d || ...)) || a || b
(common :+ And(ldiff.reduce(Or), rdiff.reduce(Or))).reduce(Or)
}
+ } else {
+ // No common factors from disjunctive predicates, reduce common
factor from conjunction
+ val all = splitConjunctivePredicates(left) ++
splitConjunctivePredicates(right)
+ val distinct = ExpressionSet(all)
+ if (all.size == distinct.size) {
+ // No common factors, return the original predicate
+ and
+ } else {
+ // (((a && b) && a && (a && c))) => a && b && c
Review comment:
nit: could we remove unnecessary parentheses to help better read of
comment?
```
(a && b) && a && (a && c) => a && b && c
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
