Skip to content

[SPARK-33861][SQL] Simplify conditional in predicate #30865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wangyum wants to merge 8 commits into from
Closed

[SPARK-33861][SQL] Simplify conditional in predicate #30865

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f786f18
Simplify conditional in predicate
wangyum Dec 20, 2020
45fd7a5
Merge remote-tracking branch 'upstream/master' into SPARK-33861
wangyum Dec 22, 2020
448faf0
fix
wangyum Dec 22, 2020
94551ea
Merge remote-tracking branch 'upstream/master' into SPARK-33861
wangyum Dec 23, 2020
c90227a
fix
wangyum Dec 23, 2020
cccbaf1
fix
wangyum Dec 24, 2020
8bd9ef9
further optimize
wangyum Dec 24, 2020
878beb0
fix
wangyum Dec 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
further optimize
  • Loading branch information
@wangyum
wangyum committed Dec 24, 2020
commit 8bd9ef9e3136e67a49dbbfe8574725dfe9f38c94
18 changes: 11 additions & 7 deletions .../main/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalsInPredicate.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ import org.apache.spark.sql.types.BooleanType
* - CASE WHEN cond THEN trueVal ELSE null END => AND(cond, trueVal)
* - CASE WHEN cond THEN trueVal ELSE true END => OR(NOT(cond), trueVal)
* - CASE WHEN cond THEN false ELSE elseVal END => AND(NOT(cond), elseVal)
* - CASE WHEN cond THEN false END => AND(NOT(cond), false)
* - CASE WHEN cond THEN false END => false
* - CASE WHEN cond THEN true ELSE elseVal END => OR(cond, elseVal)
* - CASE WHEN cond THEN true END => OR(cond, false)
* - CASE WHEN cond THEN true END => cond
*/
object SimplifyConditionalsInPredicate extends Rule[LogicalPlan] {

Expand All @@ -64,14 +64,18 @@ object SimplifyConditionalsInPredicate extends Rule[LogicalPlan] {
And(cond, trueValue)
case CaseWhen(Seq((cond, trueValue)), Some(TrueLiteral)) =>
Or(Not(cond), trueValue)
case CaseWhen(Seq((cond, FalseLiteral)), elseValue) =>
And(Not(cond), elseValue.getOrElse(FalseLiteral))
case CaseWhen(Seq((cond, TrueLiteral)), elseValue) =>
Or(cond, elseValue.getOrElse(FalseLiteral))
case CaseWhen(Seq((_, FalseLiteral)), Some(FalseLiteral) | None) =>
Copy link
Contributor

@cloud-fan cloud-fan Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This skips evaluating the condition, and we should make sure the condition is deterministic. @wangyum can you fix it?

Copy link
Member Author

@wangyum wangyum Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will fix it later.

Copy link
Member

@HyukjinKwon HyukjinKwon Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also it matters if cond throws an exception. Should probably think about NoThrow too.

Copy link
Member Author

@wangyum wangyum Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make these condition deterministic?

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

Lines 295 to 298 in 26d8df3

case FalseLiteral And _ => FalseLiteral
case _ And FalseLiteral => FalseLiteral
case TrueLiteral Or _ => TrueLiteral
case _ Or TrueLiteral => TrueLiteral

Copy link
Member Author

@wangyum wangyum Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#31067

FalseLiteral
case CaseWhen(Seq((cond, FalseLiteral)), Some(elseValue)) =>
And(Not(cond), elseValue)
case CaseWhen(Seq((cond, TrueLiteral)), Some(FalseLiteral) | None) =>
cond
case CaseWhen(Seq((cond, TrueLiteral)), Some(elseValue)) =>
Or(cond, elseValue)
case e if e.dataType == BooleanType => e
Copy link
Contributor

@cloud-fan cloud-fan Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add an assert. The analyzer should guarantee it.

Copy link
Contributor

@cloud-fan cloud-fan Dec 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean something like

case e =>
  assert(e.dataType != BooleanType, ...)
  e

Copy link
Contributor

@tanelk tanelk Dec 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the added assert can never be triggered.
I think @cloud-fan meant also to remove the case e if e.dataType == BooleanType => e

case e =>
assert(e.dataType != BooleanType,
"Expected a Boolean type expression in simplifyConditional, " +
"Expected a Boolean type expression in SimplifyConditionalsInPredicate, " +
s"but got the type `${e.dataType.catalogString}` in `${e.sql}`.")
e
}
Expand Down
32 changes: 20 additions & 12 deletions .../scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalsInPredicateSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
LocalRelation('i.int, 'b.boolean, 'a.array(IntegerType), 'm.map(IntegerType, IntegerType))
private val anotherTestRelation = LocalRelation('d.int)

test("if(cond, trueVal, false) => And(cond, trueVal)") {
test("IF(cond, trueVal, false) => AND(cond, trueVal)") {
val originalCond = If(
UnresolvedAttribute("i") > Literal(10),
UnresolvedAttribute("b"),
Expand All @@ -59,7 +59,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}

test("if(cond, trueVal, true) => or(not(cond), trueVal)") {
test("IF(cond, trueVal, true) => OR(NOT(cond), trueVal)") {
val originalCond = If(
UnresolvedAttribute("i") > Literal(10),
UnresolvedAttribute("b"),
Expand All @@ -74,7 +74,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}

test("if(cond, false, falseVal) => and(not(cond), falseVal)") {
test("IF(cond, false, falseVal) => AND(NOT(cond), elseVal)") {
val originalCond = If(
UnresolvedAttribute("i") > Literal(10),
FalseLiteral,
Expand All @@ -89,7 +89,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}

test("if(cond, true, falseVal) => or(cond, falseVal)") {
test("IF(cond, true, falseVal) => OR(cond, elseVal)") {
val originalCond = If(
UnresolvedAttribute("i") > Literal(10),
TrueLiteral,
Expand All @@ -104,8 +104,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}


test("case when cond then trueVal else false end => And(cond, trueVal)") {
test("CASE WHEN cond THEN trueVal ELSE false END => AND(cond, trueVal)") {
Seq(Some(FalseLiteral), None, Some(Literal(null, BooleanType))).foreach { elseExp =>
val originalCond = CaseWhen(
Seq((UnresolvedAttribute("i") > Literal(10), UnresolvedAttribute("b"))),
Expand All @@ -121,7 +120,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
}
}

test("case when cond then trueVal else true end => or(not(cond), trueVal)") {
test("CASE WHEN cond THEN trueVal ELSE true END => OR(NOT(cond), trueVal)") {
val originalCond = CaseWhen(
Seq((UnresolvedAttribute("i") > Literal(10), UnresolvedAttribute("b"))),
TrueLiteral)
Expand All @@ -135,8 +134,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}

test("case when cond then false else elseValue end => and(not(cond), elseValue)") {
Seq()
test("CASE WHEN cond THEN false ELSE elseVal END => AND(NOT(cond), elseVal)") {
val originalCond = CaseWhen(
Seq((UnresolvedAttribute("i") > Literal(10), FalseLiteral)),
UnresolvedAttribute("b"))
Expand All @@ -150,7 +148,17 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}

test("case when cond then true else elseValue end => or(cond, elseValue)") {
test("CASE WHEN cond THEN false END => false") {
val originalCond = CaseWhen(
Seq((UnresolvedAttribute("i") > Literal(10), FalseLiteral)))
testFilter(originalCond, expectedCond = FalseLiteral)
testJoin(originalCond, expectedCond = FalseLiteral)
testDelete(originalCond, expectedCond = FalseLiteral)
testUpdate(originalCond, expectedCond = FalseLiteral)
testProjection(originalCond, expectedExpr = originalCond)
}

test("CASE WHEN cond THEN true ELSE elseVal END => OR(cond, elseVal)") {
val originalCond = CaseWhen(
Seq((UnresolvedAttribute("i") > Literal(10), TrueLiteral)),
UnresolvedAttribute("b"))
Expand All @@ -164,7 +172,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}

test("case when cond then true end => or(cond, null)") {
test("CASE WHEN cond THEN true END => cond") {
val originalCond = CaseWhen(
Seq((UnresolvedAttribute("i") > Literal(10), TrueLiteral)))
val expectedCond = UnresolvedAttribute("i") > Literal(10)
Expand All @@ -189,7 +197,7 @@ class SimplifyConditionalsInPredicateSuite extends PlanTest {
testProjection(originalCond, expectedExpr = originalCond)
}

test("Not expected type - simplifyConditional") {
test("Not expected type - SimplifyConditionalsInPredicate") {
val e = intercept[AnalysisException] {
testFilter(originalCond = Literal(null, IntegerType), expectedCond = FalseLiteral)
}.getMessage
Expand Down