Skip to content

Commit e833ca4

Browse files
committed
Refactors deeply nested FP style code
1 parent e7884bc commit e833ca4

File tree

2 files changed

+32
-30
lines changed

2 files changed

+32
-30
lines changed

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper {
314314
// a && a => a
315315
case (l, r) if l fastEquals r => l
316316
// (a || b) && (a || c) => a || (b && c)
317-
case (_, _) =>
317+
case _ =>
318318
// 1. Split left and right to get the disjunctive predicates,
319319
// i.e. lhsSet = (a, b), rhsSet = (a, c)
320320
// 2. Find the common predict between lhsSet and rhsSet, i.e. common = (a)
@@ -323,19 +323,20 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper {
323323
val lhsSet = splitDisjunctivePredicates(left).toSet
324324
val rhsSet = splitDisjunctivePredicates(right).toSet
325325
val common = lhsSet.intersect(rhsSet)
326-
val ldiff = lhsSet.diff(common)
327-
val rdiff = rhsSet.diff(common)
328-
if (ldiff.size == 0 || rdiff.size == 0) {
329-
// a && (a || b) => a
330-
common.reduce(Or)
326+
if (common.isEmpty) {
327+
// No common factors, return the original predicate
328+
and
331329
} else {
332-
// (a || b || c || ...) && (a || b || d || ...) && (a || b || e || ...) ... =>
333-
// (a || b) || ((c || ...) && (f || ...) && (e || ...) && ...)
334-
(ldiff.reduceOption(Or) ++ rdiff.reduceOption(Or))
335-
.reduceOption(And)
336-
.map(_ :: common.toList)
337-
.getOrElse(common.toList)
338-
.reduce(Or)
330+
val ldiff = lhsSet.diff(common)
331+
val rdiff = rhsSet.diff(common)
332+
if (ldiff.isEmpty || rdiff.isEmpty) {
333+
// (a || b || c || ...) && (a || b) => (a || b)
334+
common.reduce(Or)
335+
} else {
336+
// (a || b || c || ...) && (a || b || d || ...) =>
337+
// ((c || ...) && (d || ...)) || a || b
338+
(And(ldiff.reduce(Or), rdiff.reduce(Or)) :: common.toList).reduce(Or)
339+
}
339340
}
340341
} // end of And(left, right)
341342

@@ -351,7 +352,7 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper {
351352
// a || a => a
352353
case (l, r) if l fastEquals r => l
353354
// (a && b) || (a && c) => a && (b || c)
354-
case (_, _) =>
355+
case _ =>
355356
// 1. Split left and right to get the conjunctive predicates,
356357
// i.e. lhsSet = (a, b), rhsSet = (a, c)
357358
// 2. Find the common predict between lhsSet and rhsSet, i.e. common = (a)
@@ -360,19 +361,20 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper {
360361
val lhsSet = splitConjunctivePredicates(left).toSet
361362
val rhsSet = splitConjunctivePredicates(right).toSet
362363
val common = lhsSet.intersect(rhsSet)
363-
val ldiff = lhsSet.diff(common)
364-
val rdiff = rhsSet.diff(common)
365-
if ( ldiff.size == 0 || rdiff.size == 0) {
366-
// a || (b && a) => a
367-
common.reduce(And)
364+
if (common.isEmpty) {
365+
// No common factors, return the original predicate
366+
or
368367
} else {
369-
// (a && b && c && ...) || (a && b && d && ...) || (a && b && e && ...) ... =>
370-
// a && b && ((c && ...) || (d && ...) || (e && ...) || ...)
371-
(ldiff.reduceOption(And) ++ rdiff.reduceOption(And))
372-
.reduceOption(Or)
373-
.map(_ :: common.toList)
374-
.getOrElse(common.toList)
375-
.reduce(And)
368+
val ldiff = lhsSet.diff(common)
369+
val rdiff = rhsSet.diff(common)
370+
if (ldiff.isEmpty || rdiff.isEmpty) {
371+
// (a && b) || (a && b && c && ...) => a && b
372+
common.reduce(And)
373+
} else {
374+
// (a && b && c && ...) || (a && b && d && ...) =>
375+
// ((c && ...) || (d && ...)) && a && b
376+
(Or(ldiff.reduce(And), rdiff.reduce(And)) :: common.toList).reduce(And)
377+
}
376378
}
377379
} // end of Or(left, right)
378380

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,8 @@ class BooleanSimplificationSuite extends PlanTest {
7272
(((('b > 3) && ('c > 2)) ||
7373
(('c < 1) && ('a === 5))) ||
7474
(('b < 5) && ('a > 1))) && ('a === 'b)
75-
checkCondition(input, expected)
7675

76+
checkCondition(input, expected)
7777
}
7878

7979
test("(a || b || c || ...) && (a || b || d || ...) && (a || b || e || ...) ...") {
@@ -85,8 +85,8 @@ class BooleanSimplificationSuite extends PlanTest {
8585

8686
checkCondition(('a < 2 || 'b > 3) && ('a < 2 || 'c > 5), ('b > 3 && 'c > 5) || 'a < 2)
8787

88-
var input: Expression = ('a === 'b || 'b > 3) && ('a === 'b || 'a > 3) && ('a === 'b || 'a < 5)
89-
var expected: Expression = ('b > 3 && 'a > 3 && 'a < 5) || 'a === 'b
90-
checkCondition(input, expected)
88+
checkCondition(
89+
('a === 'b || 'b > 3) && ('a === 'b || 'a > 3) && ('a === 'b || 'a < 5),
90+
('b > 3 && 'a > 3 && 'a < 5) || 'a === 'b)
9191
}
9292
}

0 commit comments

Comments
 (0)