[SPARK-35449][SQL] Only extract common expressions from CaseWhen values if elseValue is set by Kimahriman · Pull Request #32595 · apache/spark

Kimahriman · 2021-05-19T23:31:57Z

What changes were proposed in this pull request?

This PR fixes a bug with subexpression elimination for CaseWhen statements. #30245 added support for creating subexpressions that are present in all branches of conditional statements. However, for a statement to be in "all branches" of a CaseWhen statement, it must also be in the elseValue.

Why are the changes needed?

Fix a bug where a subexpression can be created and run for branches of a conditional that don't pass. This can cause issues especially with a UDF in a branch that gets executed assuming the condition is true.

Does this PR introduce any user-facing change?

Yes, fixes a potential bug where a UDF could be eagerly executed even though it might expect to have already passed some form of validation. For example:

val col = when($"id" < 0, myUdf($"id"))
spark.range(1).select(when(col > 0, col)).show()

myUdf($"id") is considered a subexpression and eagerly evaluated, because it is pulled out as a common expression from both executions of the when clause, but if id >= 0 it should never actually be run.

How was this patch tested?

Updated existing test with new case.

Kimahriman · 2021-05-19T23:32:43Z

@viirya

viirya · 2021-05-20T00:21:19Z

ok to test

viirya

looks good. waiting for CI

SparkQA · 2021-05-20T01:50:46Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43245/

SparkQA · 2021-05-20T02:26:41Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43245/

SparkQA · 2021-05-20T05:08:25Z

Test build #138722 has finished for PR 32595 at commit abc6150.

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

viirya · 2021-05-20T05:23:31Z

cc @maropu @cloud-fan too

cloud-fan · 2021-05-20T06:45:02Z

conditions4 is not used anywhere.

Copy pasta, fixed the below line

cloud-fan · 2021-05-20T06:46:08Z

is it a perf-only issue?

viirya · 2021-05-20T07:43:44Z

is it a perf-only issue?

In most cases, it is. But what I understand from @Kimahriman's description, seems in their usage like when(..., udf(col)), udf(col) will be wrongly treated as sub-expr and run in advance. But the udf fails for the input because it doesn't satisfy the condition.

So it could cause query error.

Kimahriman · 2021-05-20T10:26:24Z

is it a perf-only issue?

Yeah because of the UDF issue I'd consider it more a bug with performance side-effects. Whether those side-effects are positive or negative largely depends on whether #32559 is merged. Without it, this can increase performance by reducing the cases where you could have unused subexpressions generated. With it, it can decrease performance by not being able to create subexpressions for simple when clauses like when(length(col) > 0, col). I'm working on a follow up to address cases like that for a performance improvement

SparkQA · 2021-05-20T11:44:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43268/

SparkQA · 2021-05-20T12:16:57Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43268/

SparkQA · 2021-05-20T15:18:01Z

Test build #138746 has finished for PR 32595 at commit a8b9e89.

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

HyukjinKwon · 2021-05-21T02:59:27Z

@Kimahriman would you mind describing what behaviour change (bug fix) happens in "Does this PR introduce any user-facing change?"? The fix itself looks making sense but it would be great to clarify what bug is fixed by this too.

Kimahriman · 2021-05-21T11:17:54Z

@Kimahriman would you mind describing what behaviour change (bug fix) happens in "Does this PR introduce any user-facing change?"? The fix itself looks making sense but it would be great to clarify what bug is fixed by this too.

Updated, sorry I never know what to put for that section for bug fixes.

maropu

The fix itself looks fine. I left minor comments. btw, could you fill the description in the jira? It looks empty now: https://issues.apache.org/jira/browse/SPARK-35449

maropu · 2021-05-22T15:08:41Z

However, for a statement to be in "all branches" of a CaseWhen statement, it must also be in the elseValue.
It's better to leave a simple comment here about why we need to check elseValue.

Added a comment in

maropu · 2021-05-22T15:11:45Z

Could you put this new test in a new test unit `test("SPARK-35449: ..."?

Moved to a new test

…n elimination

Kimahriman · 2021-05-23T16:17:15Z

The fix itself looks fine. I left minor comments. btw, could you fill the description in the jira? It looks empty now: https://issues.apache.org/jira/browse/SPARK-35449

I updated the JIRA and rebased off master to get the latest subexpression PRs in. Had to update one test based on these changes.

SparkQA · 2021-05-23T17:21:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43371/

SparkQA · 2021-05-23T17:56:39Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43371/

SparkQA · 2021-05-23T19:29:19Z

Test build #138849 has finished for PR 32595 at commit e024cec.

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

SparkQA · 2021-05-23T20:09:17Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43372/

SparkQA · 2021-05-23T20:44:38Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43372/

SparkQA · 2021-05-23T23:30:19Z

Test build #138850 has finished for PR 32595 at commit 5f203d0.

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

viirya · 2021-05-24T07:26:23Z

Thanks all! Merging to master.

viirya · 2021-05-24T07:29:49Z

This should to be fixed in branch-3.1 too. @Kimahriman Can you make a backport PR?

Also cc @dongjoon-hyun as he is release manager of 3.1.2.

maropu · 2021-05-24T08:15:08Z

@viirya It looks you forgot to close jira?

viirya · 2021-05-24T08:16:45Z

Ah, right, @maropu, because backport to 3.1 fails, so the script doesn't update it. I forgot to do it manually too. Let me do it now. Thanks!

Kimahriman · 2021-05-24T11:43:28Z

This should to be fixed in branch-3.1 too. @Kimahriman Can you make a backport PR?
Also cc @dongjoon-hyun as he is release manager of 3.1.2.

Created #32651

…es if elseValue is set This PR fixes a bug with subexpression elimination for CaseWhen statements. apache#30245 added support for creating subexpressions that are present in all branches of conditional statements. However, for a statement to be in "all branches" of a CaseWhen statement, it must also be in the elseValue. Fix a bug where a subexpression can be created and run for branches of a conditional that don't pass. This can cause issues especially with a UDF in a branch that gets executed assuming the condition is true. Yes, fixes a potential bug where a UDF could be eagerly executed even though it might expect to have already passed some form of validation. For example: ``` val col = when($"id" < 0, myUdf($"id")) spark.range(1).select(when(col > 0, col)).show() ``` `myUdf($"id")` is considered a subexpression and eagerly evaluated, because it is pulled out as a common expression from both executions of the when clause, but if `id >= 0` it should never actually be run. Updated existing test with new case. Closesapache#32595 from Kimahriman/bug-case-subexpr-elimination. Authored-by: Adam Binford <adamq43@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

github-actionsBot added the SQL label May 19, 2021

viirya reviewed May 20, 2021
View reviewed changes

viirya approved these changes May 20, 2021
View reviewed changes

cloud-fan reviewed May 20, 2021
View reviewed changes

Kimahriman force-pushed the bug-case-subexpr-elimination branch from abc6150 to a8b9e89Compare May 20, 2021 10:23

maropu reviewed May 22, 2021
View reviewed changes

CaseWhen needs to have an elseValue to be considered for subexpressio…
4334128
…n elimination

Kimahriman force-pushed the bug-case-subexpr-elimination branch from a8b9e89 to e024cecCompare May 23, 2021 16:15

Add comment, create new test, and fix test after rebasing
5f203d0

Kimahriman force-pushed the bug-case-subexpr-elimination branch from e024cec to 5f203d0Compare May 23, 2021 19:13

maropu approved these changes May 24, 2021
View reviewed changes

viirya approved these changes May 24, 2021
View reviewed changes

HyukjinKwon approved these changes May 24, 2021
View reviewed changes

viirya closed this in 6c0c617May 24, 2021

Uh oh!

Conversation

Kimahriman commented May 19, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Kimahriman commented May 19, 2021

Uh oh!

viirya commented May 20, 2021

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

viirya commented May 20, 2021

Uh oh!

cloud-fanMay 20, 2021

Choose a reason for hiding this comment

Uh oh!

KimahrimanMay 20, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented May 20, 2021

Uh oh!

viirya commented May 20, 2021

Uh oh!

Kimahriman commented May 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

HyukjinKwon commented May 21, 2021

Uh oh!

Kimahriman commented May 21, 2021

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

maropuMay 22, 2021

Choose a reason for hiding this comment

Uh oh!

KimahrimanMay 23, 2021

Choose a reason for hiding this comment

Uh oh!

maropuMay 22, 2021

Choose a reason for hiding this comment

Uh oh!

KimahrimanMay 23, 2021

Choose a reason for hiding this comment

Uh oh!

Kimahriman commented May 23, 2021

Uh oh!

SparkQA commented May 23, 2021

Uh oh!

SparkQA commented May 23, 2021

Uh oh!

SparkQA commented May 23, 2021

Uh oh!

SparkQA commented May 23, 2021

Uh oh!

SparkQA commented May 23, 2021

Uh oh!

SparkQA commented May 23, 2021

Uh oh!

viirya commented May 24, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented May 24, 2021

Kimahriman commented May 19, 2021•
edited
Loading

Kimahriman commented May 20, 2021•
edited
Loading

viirya commented May 24, 2021•
edited
Loading