You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/02/18 00:45:33 UTC
[GitHub] [spark] maropu commented on a change in pull request #31318: [SPARK-34222][SQL] Enhance boolean simplification rule
maropu commented on a change in pull request #31318:
URL: https://github.com/apache/spark/pull/31318#discussion_r578043404
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -352,10 +352,7 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper {
val lhs = splitDisjunctivePredicates(left)
Review comment:
Could you update the comment about how to handle this case?
https://github.com/apache/spark/pull/31318/files#diff-d43484d56a4d9991066b5c00d12ec2465c75131e055fc02ee7fb6dfd45b5006fR347-R351
##########
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
Review comment:
nit: could you add an example case here like the comment above (` // (a || b || c || ...) && (a || b) => (a || b)`)?
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,56 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with
'a === 'b || 'b > 3 && 'a > 3 && 'a < 5)
}
+ test("SPARK-34222: (((a && b) && a && (a && c))) => a && b && c") {
+
+ checkCondition((('a > 1 && 'b > 2) && 'a > 1 &&('a > 1 && 'c > 3)),
+ ('a > 1 && ('b > 2 && 'c > 3)))
+
+ checkCondition((('a > 1 && 'b >2) && ('a > 4 && 'b > 5) && ('a > 1 && 'c > 3)),
+ ('a > 1 && 'b > 2 && 'c > 3 && 'a > 4 && 'b > 5))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && 'c >1))),
+ ('a > 1 && 'b > 3 && 'c >1))
+
+ checkCondition(
+ (('a > 1 || 'b > 3) && (('a > 1 || 'b > 3) && 'd > 0 && (('a > 1 || 'b > 3) && 'c >1))),
+ (('a > 1 || 'b > 3) && 'd > 0 && 'c >1))
+
+ checkCondition(
+ ('a > 1 && 'b > 2 && 'a > 1 && 'c > 3),
+ ('a > 1 && 'b > 2 && 'c > 3))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && 'a > 1 ) || ('a > 1 && 'b > 3 && 'a > 1 && 'c > 1 )
Review comment:
nit: remove the unnecessary space between `'c > 1` and `)`.
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,56 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with
'a === 'b || 'b > 3 && 'a > 3 && 'a < 5)
}
+ test("SPARK-34222: (((a && b) && a && (a && c))) => a && b && c") {
+
+ checkCondition((('a > 1 && 'b > 2) && 'a > 1 &&('a > 1 && 'c > 3)),
+ ('a > 1 && ('b > 2 && 'c > 3)))
+
+ checkCondition((('a > 1 && 'b >2) && ('a > 4 && 'b > 5) && ('a > 1 && 'c > 3)),
+ ('a > 1 && 'b > 2 && 'c > 3 && 'a > 4 && 'b > 5))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && 'c >1))),
+ ('a > 1 && 'b > 3 && 'c >1))
+
+ checkCondition(
+ (('a > 1 || 'b > 3) && (('a > 1 || 'b > 3) && 'd > 0 && (('a > 1 || 'b > 3) && 'c >1))),
+ (('a > 1 || 'b > 3) && 'd > 0 && 'c >1))
+
+ checkCondition(
+ ('a > 1 && 'b > 2 && 'a > 1 && 'c > 3),
+ ('a > 1 && 'b > 2 && 'c > 3))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && 'a > 1 ) || ('a > 1 && 'b > 3 && 'a > 1 && 'c > 1 )
+ , ('a > 1 && 'b > 3))
+ }
+
+ test("SPARK-34222: (((a || b) || a || (a || c))) => a || b || c") {
+
+ checkCondition((('a > 1 || 'b > 2) || 'a > 1 ||('a > 1 || 'c > 3)),
+ ('a > 1 || 'b > 2 || 'c > 3))
+
+ checkCondition((('a > 1 || 'b >2) || 'a > 4 ||('a > 1 || 'c > 3)),
+ ('a > 1 || 'b > 2 || 'a > 4 || 'c > 3))
+
+ checkCondition(
+ ('a > 1 || 'b > 3 || ( 'a > 1 || 'b > 3 || ('a > 1 || 'b > 3 || 'c > 1)))
+ , ('a > 1 || 'b > 3 || 'c > 1))
+
+ checkCondition(
+ (('a > 1 && 'b > 3) || ( ('a > 1 && 'b > 3) || (('a > 1 && 'b > 3) || 'c > 1)))
+ , (('a > 1 && 'b > 3) || 'c > 1))
+
+ checkCondition(
+ ('a > 1 || 'b > 2 || 'a > 1 || 'c > 3),
+ ('a > 1 || 'b > 2 || 'c > 3))
+
+ checkCondition(
+ ('a > 1 || 'b > 3 || 'a > 1 ) && ('a > 1 || 'b > 3 || 'a > 1 || 'c > 1 )
+ , ('a > 1 || 'b > 3))
Review comment:
ditto
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,56 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with
'a === 'b || 'b > 3 && 'a > 3 && 'a < 5)
}
+ test("SPARK-34222: (((a && b) && a && (a && c))) => a && b && c") {
+
Review comment:
nit: remove the unnecessary blank line.
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,56 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with
'a === 'b || 'b > 3 && 'a > 3 && 'a < 5)
}
+ test("SPARK-34222: (((a && b) && a && (a && c))) => a && b && c") {
+
+ checkCondition((('a > 1 && 'b > 2) && 'a > 1 &&('a > 1 && 'c > 3)),
Review comment:
nit: add a space between `&&` and `(`
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,56 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with
'a === 'b || 'b > 3 && 'a > 3 && 'a < 5)
}
+ test("SPARK-34222: (((a && b) && a && (a && c))) => a && b && c") {
+
+ checkCondition((('a > 1 && 'b > 2) && 'a > 1 &&('a > 1 && 'c > 3)),
+ ('a > 1 && ('b > 2 && 'c > 3)))
+
+ checkCondition((('a > 1 && 'b >2) && ('a > 4 && 'b > 5) && ('a > 1 && 'c > 3)),
+ ('a > 1 && 'b > 2 && 'c > 3 && 'a > 4 && 'b > 5))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && 'c >1))),
+ ('a > 1 && 'b > 3 && 'c >1))
+
+ checkCondition(
+ (('a > 1 || 'b > 3) && (('a > 1 || 'b > 3) && 'd > 0 && (('a > 1 || 'b > 3) && 'c >1))),
+ (('a > 1 || 'b > 3) && 'd > 0 && 'c >1))
+
+ checkCondition(
+ ('a > 1 && 'b > 2 && 'a > 1 && 'c > 3),
+ ('a > 1 && 'b > 2 && 'c > 3))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && 'a > 1 ) || ('a > 1 && 'b > 3 && 'a > 1 && 'c > 1 )
+ , ('a > 1 && 'b > 3))
+ }
+
+ test("SPARK-34222: (((a || b) || a || (a || c))) => a || b || c") {
+
+ checkCondition((('a > 1 || 'b > 2) || 'a > 1 ||('a > 1 || 'c > 3)),
+ ('a > 1 || 'b > 2 || 'c > 3))
+
+ checkCondition((('a > 1 || 'b >2) || 'a > 4 ||('a > 1 || 'c > 3)),
+ ('a > 1 || 'b > 2 || 'a > 4 || 'c > 3))
+
+ checkCondition(
+ ('a > 1 || 'b > 3 || ( 'a > 1 || 'b > 3 || ('a > 1 || 'b > 3 || 'c > 1)))
+ , ('a > 1 || 'b > 3 || 'c > 1))
+
+ checkCondition(
+ (('a > 1 && 'b > 3) || ( ('a > 1 && 'b > 3) || (('a > 1 && 'b > 3) || 'c > 1)))
+ , (('a > 1 && 'b > 3) || 'c > 1))
Review comment:
ditto
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,56 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with
'a === 'b || 'b > 3 && 'a > 3 && 'a < 5)
}
+ test("SPARK-34222: (((a && b) && a && (a && c))) => a && b && c") {
+
+ checkCondition((('a > 1 && 'b > 2) && 'a > 1 &&('a > 1 && 'c > 3)),
+ ('a > 1 && ('b > 2 && 'c > 3)))
+
+ checkCondition((('a > 1 && 'b >2) && ('a > 4 && 'b > 5) && ('a > 1 && 'c > 3)),
+ ('a > 1 && 'b > 2 && 'c > 3 && 'a > 4 && 'b > 5))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && 'c >1))),
+ ('a > 1 && 'b > 3 && 'c >1))
+
+ checkCondition(
+ (('a > 1 || 'b > 3) && (('a > 1 || 'b > 3) && 'd > 0 && (('a > 1 || 'b > 3) && 'c >1))),
+ (('a > 1 || 'b > 3) && 'd > 0 && 'c >1))
+
+ checkCondition(
+ ('a > 1 && 'b > 2 && 'a > 1 && 'c > 3),
+ ('a > 1 && 'b > 2 && 'c > 3))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && 'a > 1 ) || ('a > 1 && 'b > 3 && 'a > 1 && 'c > 1 )
+ , ('a > 1 && 'b > 3))
+ }
+
+ test("SPARK-34222: (((a || b) || a || (a || c))) => a || b || c") {
+
+ checkCondition((('a > 1 || 'b > 2) || 'a > 1 ||('a > 1 || 'c > 3)),
+ ('a > 1 || 'b > 2 || 'c > 3))
+
+ checkCondition((('a > 1 || 'b >2) || 'a > 4 ||('a > 1 || 'c > 3)),
+ ('a > 1 || 'b > 2 || 'a > 4 || 'c > 3))
+
+ checkCondition(
+ ('a > 1 || 'b > 3 || ( 'a > 1 || 'b > 3 || ('a > 1 || 'b > 3 || 'c > 1)))
+ , ('a > 1 || 'b > 3 || 'c > 1))
Review comment:
could you follow the the other format?
```
checkCondition(
('a > 1 || 'b > 3 || ( 'a > 1 || 'b > 3 || ('a > 1 || 'b > 3 || 'c > 1))),
('a > 1 || 'b > 3 || 'c > 1))
```
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,56 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with
'a === 'b || 'b > 3 && 'a > 3 && 'a < 5)
}
+ test("SPARK-34222: (((a && b) && a && (a && c))) => a && b && c") {
+
+ checkCondition((('a > 1 && 'b > 2) && 'a > 1 &&('a > 1 && 'c > 3)),
+ ('a > 1 && ('b > 2 && 'c > 3)))
+
+ checkCondition((('a > 1 && 'b >2) && ('a > 4 && 'b > 5) && ('a > 1 && 'c > 3)),
+ ('a > 1 && 'b > 2 && 'c > 3 && 'a > 4 && 'b > 5))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && ('a > 1 && 'b > 3 && 'c >1))),
+ ('a > 1 && 'b > 3 && 'c >1))
+
+ checkCondition(
+ (('a > 1 || 'b > 3) && (('a > 1 || 'b > 3) && 'd > 0 && (('a > 1 || 'b > 3) && 'c >1))),
+ (('a > 1 || 'b > 3) && 'd > 0 && 'c >1))
+
+ checkCondition(
+ ('a > 1 && 'b > 2 && 'a > 1 && 'c > 3),
+ ('a > 1 && 'b > 2 && 'c > 3))
+
+ checkCondition(
+ ('a > 1 && 'b > 3 && 'a > 1 ) || ('a > 1 && 'b > 3 && 'a > 1 && 'c > 1 )
+ , ('a > 1 && 'b > 3))
+ }
+
+ test("SPARK-34222: (((a || b) || a || (a || c))) => a || b || c") {
+
Review comment:
ditto
##########
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)
+ val distinct = all.distinct
Review comment:
Could you use `ExpressionSet` for canonicalization?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org