[SPARK-24890] [SQL] Short circuiting the `if` condition when `trueValue` and `falseValue` are the same #21848

dbtsai · 2018-07-23T17:32:52Z

What changes were proposed in this pull request?

When trueValue and falseValue are semantic equivalence, the condition expression in if can be removed to avoid extra computation in runtime.

How was this patch tested?

Test added.

dongjoon-hyun · 2018-07-23T18:21:06Z

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

        }

-      case e @ CaseWhen(branches, _) if branches.headOption.map(_._1) == Some(TrueLiteral) =>
+      case CaseWhen(branches, _) if branches.headOption.map(_._1).contains(TrueLiteral) =>


Since we removed Scala 2.10, it seems to be okay. However, if we revert this irrelevant change, this PR becomes neater (and easier for someone to backport this).

Since it's not a bug fix, I guess it's unlikely someone will backport this :)

Yes. In the community, it's not allowed for backport. I mean the others who want to have this.

Eh, in any event, wouldn't it be better to revert this change back if there's any actual advantage against a unrelated style change?

Normally, we avoid adding unneeded refactoring in such a PR. Please avoid it next time. Thanks!

viirya · 2018-07-23T18:23:32Z

~~LGTM~~
We should consider side effect of condition expression. See the discussion below.

viirya · 2018-07-23T18:23:04Z

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

  }
 }

+


nit: extra space line.

I thought we always have two new lines between two objects

I think it's okay to remove it back though assuming from

Use one or two blank line(s) to separate class definitions.

https://github.com/databricks/scala-style-guide#blank-lines-vertical-whitespace

Looks either way is fine.

gatorsmile · 2018-07-23T18:44:25Z

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

      case If(TrueLiteral, trueValue, _) => trueValue
      case If(FalseLiteral, _, falseValue) => falseValue
      case If(Literal(null, _), _, falseValue) => falseValue
+      case If(_, trueValue, falseValue) if trueValue.semanticEquals(falseValue) => trueValue


This is not right. The condition must be deterministic

Can you elaborate?

For trueValue.semanticEquals(falseValue), it's guaranteed that both trueValue and falseValue are deterministic.

def semanticEquals(other: Expression): Boolean = deterministic && other.deterministic && canonicalized == other.canonicalized

Understandable that the condition can be non-deterministic, but this doesn't change the result of If.

The condition could have a side effect. For example, calling a stateful UDF.

Good point.

dongjoon-hyun · 2018-07-23T19:02:50Z

Since this skips the evaluation of if condition, this will cause the following difference.

This PR

scala> sql("select * from t").show
+----+
|   a|
+----+
|   1|
|null|
+----+

scala> sql("select if(assert_true(a is null),a,a) from t").show
+-----------------------------------------------------+
|(IF(CAST(assert_true((a IS NULL)) AS BOOLEAN), a, a))|
+-----------------------------------------------------+
|                                                    1|
|                                                 null|
+-----------------------------------------------------+

Spark 2.3.1

scala> sql("select * from t").show
+----+
|   a|
+----+
|   1|
|null|
+----+

scala> sql("select if(assert_true(a is null),a,a) from t").show
18/07/23 11:59:11 ERROR Executor: Exception in task 0.0 in stage 20.0 (TID 20)
java.lang.RuntimeException: 'isnull(input[0, int, true])' is not true!

viirya · 2018-07-23T19:17:29Z

This is a good point.

…

On Mon, Jul 23, 2018, 12:03 PM Dongjoon Hyun ***@***.***> wrote: Since this skips the evaluation of if condition, this will cause the following difference. *This PR* scala> sql("select * from t").show +----+ | a| +----+ | 1| |null| +----+ scala> sql("select if(assert_true(a is null),a,a) from t").show +-----------------------------------------------------+ |(IF(CAST(assert_true((a IS NULL)) AS BOOLEAN), a, a))| +-----------------------------------------------------+ | 1| | null| +-----------------------------------------------------+ *Spark 2.3.1* scala> sql("select * from t").show +----+ | a| +----+ | 1| |null| +----+ scala> sql("select if(assert_true(a is null),a,a) from t").show 18/07/23 11:59:11 ERROR Executor: Exception in task 0.0 in stage 20.0 (TID 20) java.lang.RuntimeException: 'isnull(input[0, int, true])' is not true! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#21848 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEM99Jsk4Z9Nt7NV5zkdG9oWyUDY4OZks5uJh4RgaJpZM4VbZaO> .

SparkQA · 2018-07-23T20:08:20Z

Test build #93454 has finished for PR 21848 at commit 30ca8cf.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-07-23T20:26:17Z

For now, seems we don't have a good way to know if an expression has side effect. Some expressions like AssertTrue should be marked as one with side effect. Maybe we should create a trait for this purpose.

dbtsai · 2018-07-23T21:15:46Z

@gatorsmile this can remove some of the expensive condition expressions, so I would like to find a way to properly implement this.

Thank you all for chiming in with many good points. Let me summary here.

The cond expression can only be removed when it doesn't have a side effect.
Stateful expression must have a side effect.
Some of the non-stateful expressions such as AssertTrue have a side effect.
Determinstic expression could have a side effect.
Nondeterministic expressions always have a side effect.

This means determinstic is not enough, and we need another variable to check if an expression has a side effect.

gatorsmile · 2018-07-24T00:17:24Z

Currently, we are setting the expressions deterministic to false when they are either having side effect or non-deterministic. We already did it for Hive UDFs who have stateful tags.

We should change deterministic to false for both AssertTrue and AssertNotNull.

dbtsai · 2018-07-24T01:42:14Z

This will simplify the scope of this PR a lot. My concern is the more non-deterministic expressions we have, the less optimization we can do. Luckily, both of them are not used in general expressions.

SparkQA · 2018-07-24T02:31:30Z

Test build #93471 has finished for PR 21848 at commit bf0b2d9.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-07-24T02:57:36Z

Hmm, seems we have limitation on where non deterministic expressions can be in.

kiszk · 2018-07-24T15:17:27Z

@dbtsai I have a question. How does the current code check the following condition?

Stateful expression must have a side effect.

gatorsmile · 2018-07-24T15:28:52Z

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

 case class AssertNotNull(child: Expression, walkedTypePath: Seq[String] = Nil)
  extends UnaryExpression with NonSQLExpression {

+  override lazy val deterministic: Boolean = false


Let us create a separate PR for the changes on deterministic? We need extra changes when we changing the flags.

Fair. I'll create a followup PR for this.

dbtsai · 2018-07-24T23:10:16Z

@kiszk trait Stateful extends Nondeterministic, and this rule will not be invoked when an expression is nondeterministic.

dbtsai · 2018-07-24T23:20:34Z

Here is a followup JIRA for making AssertTrue and AssertNotNull non-deterministic https://issues.apache.org/jira/browse/SPARK-24913

SparkQA · 2018-07-25T03:06:02Z

Test build #93523 has finished for PR 21848 at commit b4f1431.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-07-25T03:20:33Z

LGTM

Thanks! Merged to master.

HyukjinKwon · 2020-12-28T05:05:25Z

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

      case If(FalseLiteral, _, falseValue) => falseValue
      case If(Literal(null, _), _, falseValue) => falseValue
+      case If(cond, trueValue, falseValue)
+        if cond.deterministic && trueValue.semanticEquals(falseValue) => trueValue


Looks like we'll still have this problem by skipping the evaluation of cond ..
Lately, SPARK-33544 introduced another approach for that. I think that superseded SPARK-24913. I think we can switch it to use SPARK-33544 approach.

@dbtsai, can we try and follow up it with using NoThrow?

cc @tgravescs too FYI

Add if short circuit

30ca8cf

dbtsai changed the title ~~[SPARK-24890] [SQ] Short circuiting the if condition when trueValue and falseValue are the same~~ [SPARK-24890] [SQL] Short circuiting the if condition when trueValue and falseValue are the same Jul 23, 2018

dongjoon-hyun reviewed Jul 23, 2018

View reviewed changes

viirya reviewed Jul 23, 2018

View reviewed changes

gatorsmile reviewed Jul 23, 2018

View reviewed changes

kiszk mentioned this pull request Jul 23, 2018

[SPARK-24893] [SQL] Remove the entire CaseWhen if all the outputs are semantic equivalence #21852

Closed

address feedback

bf0b2d9

gatorsmile reviewed Jul 24, 2018

View reviewed changes

dbtsai added 2 commits July 24, 2018 11:18

temp

19f2ccc

Addressed feedback

b4f1431

asfgit closed this in d4c3415 Jul 25, 2018

mgaido91 mentioned this pull request Nov 5, 2018

[SPARK-24913][SQL] Make AssertNotNull and AssertTrue non-deterministic #22947

Closed

wangyum mentioned this pull request Dec 19, 2020

[SPARK-33845][SQL] Remove unnecessary if when trueValue and falseValue are foldable boolean types #30849

Closed

HyukjinKwon mentioned this pull request Dec 28, 2020

[SPARK-33884][SQL] Simplify CaseWhenclauses with (true and false) and (false and true) #30898

Closed

HyukjinKwon reviewed Dec 28, 2020

View reviewed changes

[SPARK-24890] [SQL] Short circuiting the if condition when trueValue and falseValue are the same #21848

[SPARK-24890] [SQL] Short circuiting the if condition when trueValue and falseValue are the same #21848

Uh oh!

Conversation

dbtsai commented Jul 23, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dongjoon-hyun Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 23, 2018

Uh oh!

viirya commented Jul 23, 2018 via email

Uh oh!

SparkQA commented Jul 23, 2018

Uh oh!

viirya commented Jul 23, 2018

Uh oh!

dbtsai commented Jul 23, 2018

Uh oh!

gatorsmile commented Jul 24, 2018

Uh oh!

dbtsai commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 24, 2018

Uh oh!

viirya commented Jul 24, 2018

Uh oh!

kiszk commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbtsai commented Jul 24, 2018

Uh oh!

dbtsai commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 25, 2018

Uh oh!

gatorsmile commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

[SPARK-24890] [SQL] Short circuiting the `if` condition when `trueValue` and `falseValue` are the same #21848

[SPARK-24890] [SQL] Short circuiting the `if` condition when `trueValue` and `falseValue` are the same #21848

dongjoon-hyun Jul 23, 2018 •

edited

Loading

viirya commented Jul 23, 2018 •

edited

Loading

dbtsai commented Jul 24, 2018 •

edited

Loading

dbtsai commented Jul 24, 2018 •

edited

Loading