maropu commented on a change in pull request #31318:
URL: https://github.com/apache/spark/pull/31318#discussion_r585142435
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -345,53 +345,77 @@ object BooleanSimplification extends Rule[LogicalPlan]
with PredicateHelper {
// Common factor elimination for conjunction
case and @ (left And right) =>
// 1. Split left and right to get the disjunctive predicates,
- // i.e. lhs = (a, b), rhs = (a, c)
+ // i.e. lhs = (a || b), rhs = (a || c)
// 2. Find the common predict between lhsSet and rhsSet, i.e. common =
(a)
// 3. Remove common predict from lhsSet and rhsSet, i.e. ldiff = (b),
rdiff = (c)
- // 4. Apply the formula, get the optimized predicate: common || (ldiff
&& rdiff)
+ // 4. If common is non-empty, apply the formula to get the optimized
predicate:
+ // common || (ldiff && rdiff)
+ // 5. Else if common is empty, split left and right to get the
conjunctive predicates.
+ // for example lhs = (a && b), rhs = (a && c) => all = (a, b, a,
c), distinct = (a, b, c)
+ // optimized predicate: (a && b && c)
val lhs = splitDisjunctivePredicates(left)
val rhs = splitDisjunctivePredicates(right)
val common = lhs.filter(e => rhs.exists(e.semanticEquals))
- if (common.isEmpty) {
- // No common factors, return the original predicate
- and
- } else {
+ if (!common.isEmpty) {
val ldiff = lhs.filterNot(e => common.exists(e.semanticEquals))
val rdiff = rhs.filterNot(e => common.exists(e.semanticEquals))
if (ldiff.isEmpty || rdiff.isEmpty) {
// (a || b || c || ...) && (a || b) => (a || b)
common.reduce(Or)
} else {
// (a || b || c || ...) && (a || b || d || ...) =>
- // ((c || ...) && (d || ...)) || a || b
+ // a || b || ((c || ...) && (d || ...))
(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
+ buildBalancedPredicate(distinct.toSeq, 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)
// 2. Find the common predict between lhsSet and rhsSet, i.e. common =
(a)
// 3. Remove common predict from lhsSet and rhsSet, i.e. ldiff = (b),
rdiff = (c)
- // 4. Apply the formula, get the optimized predicate: common && (ldiff
|| rdiff)
+ // 4. If common is non-empty, apply the formula to get the optimized
predicate:
+ // common && (ldiff || rdiff)
+ // 5. Else if common is empty, split left and right to get the
conjunctive predicates.
+ // for example lhs = (a || b), rhs = (a || c) => all = (a, b, a, c),
distinct = (a, b, c)
+ // optimized predicate: (a || b || c)
val lhs = splitConjunctivePredicates(left)
val rhs = splitConjunctivePredicates(right)
val common = lhs.filter(e => rhs.exists(e.semanticEquals))
- if (common.isEmpty) {
- // No common factors, return the original predicate
- or
- } else {
+ if (!common.isEmpty) {
Review comment:
ditto
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -345,53 +345,77 @@ object BooleanSimplification extends Rule[LogicalPlan]
with PredicateHelper {
// Common factor elimination for conjunction
case and @ (left And right) =>
// 1. Split left and right to get the disjunctive predicates,
- // i.e. lhs = (a, b), rhs = (a, c)
+ // i.e. lhs = (a || b), rhs = (a || c)
// 2. Find the common predict between lhsSet and rhsSet, i.e. common =
(a)
// 3. Remove common predict from lhsSet and rhsSet, i.e. ldiff = (b),
rdiff = (c)
- // 4. Apply the formula, get the optimized predicate: common || (ldiff
&& rdiff)
+ // 4. If common is non-empty, apply the formula to get the optimized
predicate:
+ // common || (ldiff && rdiff)
+ // 5. Else if common is empty, split left and right to get the
conjunctive predicates.
+ // for example lhs = (a && b), rhs = (a && c) => all = (a, b, a,
c), distinct = (a, b, c)
+ // optimized predicate: (a && b && c)
val lhs = splitDisjunctivePredicates(left)
val rhs = splitDisjunctivePredicates(right)
val common = lhs.filter(e => rhs.exists(e.semanticEquals))
- if (common.isEmpty) {
- // No common factors, return the original predicate
- and
- } else {
+ if (!common.isEmpty) {
Review comment:
nit: `common.nonEmpty`
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,54 @@ 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 || 'b > 5) ||('a > 1 || 'c >
3),
Review comment:
ditto: `'b >2`
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,54 @@ 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)),
Review comment:
ditto: `'c >1`
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,54 @@ 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 || 'b > 5) ||('a > 1 || 'c >
3),
+ ('a > 1 || 'b > 2) || ('a > 4 || 'b > 5) || '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 ),
Review comment:
nit: remove a space: `'c > 1 )` and `'a > 1 )`
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,54 @@ 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),
Review comment:
nit: add a space: `'b >2`
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,54 @@ 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)
Review comment:
ditto: `'c >1`
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala
##########
@@ -126,6 +126,54 @@ 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)),
Review comment:
ditto: `'c >1`
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
