[SPARK-33619][SQL] Fix GetMapValueUtil code generation error by leanken-zz · Pull Request #30560 · apache/spark

leanken-zz · 2020-12-01T09:34:16Z

What changes were proposed in this pull request?

Code Gen bug fix that introduced by SPARK-33460

GetMapValueUtil

s"""throw new NoSuchElementException("Key " + $eval2 + " does not exist.");"""

SHOULD BE

s"""throw new java.util.NoSuchElementException("Key " + $eval2 + " does not exist.");"""

And the reason why SPARK-33460 failed to detect this bug via UT, it was because that checkExceptionInExpression did not work as expect like checkEvaluation which will try eval expression with BOTH CODEGEN_ONLY and NO_CODEGEN mode, and in this PR, will also fix this Test bug, too.

Why are the changes needed?

Bug Fix.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UT and Existing UT.

leanken-zz · 2020-12-01T09:36:26Z

Why were

checkExceptionInExpression[Exception](expr, errMsg)
sql/testOnly org.apache.spark.sql.SQLQueryTestSuite – -z ansi/map.sql
in SPARK-33460 can't detect this Bug

It's because
1. checkExceptionInExpression is some how with error, too, but i don't know of the history. It should wrap with
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key like CheckEvalulation, AND WE SHOULD FIX this later.
2. SQLQueryTestSuite ansi/map.sql failed to detect because of the Constant Folding rules, it is calling eval instead of code gen in this case

leanken-zz · 2020-12-01T09:37:20Z

@cloud-fan @HyukjinKwon FYI.

HyukjinKwon · 2020-12-01T10:06:16Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

@leanken, can we add a test?

@leanken, can we add a test?

Currently, this case being missed in SPARK-33460 it is because checkExceptionInExpression is not as expected. And as i investigated, there are some cases would fail if I correct the behavior of checkExceptionInExpression by using withSQLConf set CodeGenFallBack to false, like ObjectExpressionsSuite and RegexpExpressionsSuite and etc.

So I need your advise. Should I

Fix all these wrong behavior separately and then finally fix checkExceptionInExpression.

Fix all these wrong behavior and checkExceptionInExpression with the same patch.

The reason why i submit this PR it's because this bug introduced by me and I know of the history, but all those other behavior is fresh to me, and the all-in-one fix might be big.

As for add a test to the java.util.NoSuchElementException, I think the offline manual verification is enough, If we fix checkExceptionInExpression, and make it reliable, then we don't need to check code gen behavior for every new added or updated operation, right?

@leanken, can we add a test?

maybe you can try reproduce ObjectExpressionsSuite and RegexpExpressionsSuite in your environment so that you can get clearer picture.

And as i investigated, there are some cases would fail if I correct the behavior of checkExceptionInExpression by using withSQLConf set CodeGenFallBack to false, like ObjectExpressionsSuite and RegexpExpressionsSuite and etc.

How about making a PR to fix checkExceptionInExpression first? I'm currently not sure how many failures caused by it exist. If there are too many failures, we can separate the work to fix them then.

+1 for @maropu 's advice.

…RK-33460]. Fix code style of [SPARK-33572]. Change-Id: I7744f8bf35e1a10afa7c155b917835a1facdd1ce

leanken-zz · 2020-12-02T02:45:50Z

let's see how many error occurs after I change the checkExceptionInExpression

@maropu @HyukjinKwon @dongjoon-hyun @cloud-fan

SparkQA · 2020-12-02T04:40:19Z

Test build #132019 has finished for PR 30560 at commit 8e4ca3d.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Change-Id: Icdd9c60778b9fa155e4b2cb62e2e6d841a43c174

leanken-zz · 2020-12-02T05:40:54Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

should remove the entire function, but here we run an entire test to see if there are regressions.

SparkQA · 2020-12-02T05:42:04Z

Test build #132015 has finished for PR 30560 at commit 6d4fb1d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class MakeDate(

maropu · 2020-12-02T06:09:41Z

 org.apache.spark.sql.catalyst.expressions.DateExpressionsSuite.Consistent error handling for datetime formatting and parsing functions	16 ms	1
 org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite.SPARK-23594 GetExternalRowField should support interpreted execution	49 ms	1
 org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite.SPARK-23595 ValidateExternalType should support interpreted execution	0.64 sec	1
 org.apache.spark.sql.catalyst.expressions.RegexpExpressionsSuite.RegexExtract	0.25 sec	1
 org.apache.spark.sql.catalyst.expressions.RegexpExpressionsSuite.RegexExtractAll

It seems the five errors? If so, IMO its okay to fix all of them at once. Anyway, nice catch!

leanken-zz · 2020-12-02T06:18:10Z

 org.apache.spark.sql.catalyst.expressions.DateExpressionsSuite.Consistent error handling for datetime formatting and parsing functions	16 ms	1
 org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite.SPARK-23594 GetExternalRowField should support interpreted execution	49 ms	1
 org.apache.spark.sql.catalyst.expressions.ObjectExpressionsSuite.SPARK-23595 ValidateExternalType should support interpreted execution	0.64 sec	1
 org.apache.spark.sql.catalyst.expressions.RegexpExpressionsSuite.RegexExtract	0.25 sec	1
 org.apache.spark.sql.catalyst.expressions.RegexpExpressionsSuite.RegexExtractAll

It seems the five errors? If so, IMO its okay to fix all of them at once. Anyway, nice catch!

it should be zero error after the final patch 16940ce. please ignore the former test result ^_^

Change-Id: I035eca72ebdc6d79676fe14cc79ac4ac2eb4430c

SparkQA · 2020-12-02T07:13:01Z

Test build #132022 has finished for PR 30560 at commit 16940ce.

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

cloud-fan · 2020-12-02T07:35:17Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

For simplicity, can we always test it with 2 modes?

should be OK. since setting mode does not affect evaluateWithoutCodegen

maropu · 2020-12-02T08:35:59Z

Could you update the title and the description, too?

leanken-zz · 2020-12-02T09:21:13Z

and

OK

Change-Id: Ifcd5d186c16879159b84223dbc79fcf0ec628c81

leanken-zz · 2020-12-02T10:17:02Z

Could you update the title and the description, too?

update desc and add another UT in ExpressionEvalHelperSuite to make sure the checkExceptionInExpression change work as expect.

SparkQA · 2020-12-02T11:18:38Z

Test build #132025 has finished for PR 30560 at commit 6570d70.

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

cloud-fan · 2020-12-02T11:41:29Z

retest this please

SparkQA · 2020-12-02T15:08:10Z

Test build #132040 has finished for PR 30560 at commit 95bdd37.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class BadCodegenAndEvalExpression() extends LeafExpression

cloud-fan · 2020-12-02T16:10:42Z

thanks, merging to master!

SparkQA · 2020-12-02T16:30:28Z

Test build #132047 has finished for PR 30560 at commit 95bdd37.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class BadCodegenAndEvalExpression() extends LeafExpression

leanken-zz · 2020-12-03T02:22:38Z

retest this please

dongjoon-hyun · 2021-01-04T23:21:33Z

Hi, All.
This seems to break Scala 2.13 UT. Please see https://issues.apache.org/jira/browse/SPARK-33948?focusedCommentId=17258217&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17258217 .

HyukjinKwon · 2021-01-06T02:50:10Z

Thank you @dongjoon-hyun and @LuciferYang for investigating and checking this!

LuciferYang · 2021-01-06T03:58:14Z

@HyukjinKwon @dongjoon-hyun I will give a pr try to fix this, It seems that there is some wrong with the generated code. But I am not sure why this pr triggered the problem ....

github-actions bot added the SQL label Dec 1, 2020

HyukjinKwon reviewed Dec 1, 2020

View reviewed changes

HyukjinKwon changed the title ~~[SPARK-33619] GetMapValueUtil code generation error.~~ [SPARK-33619][SQL] GetMapValueUtil code generation error. Dec 1, 2020

[SPARK-33619] GetMapValueUtil code generation error. relative to [SPA…

6d4fb1d

…RK-33460]. Fix code style of [SPARK-33572]. Change-Id: I7744f8bf35e1a10afa7c155b917835a1facdd1ce

dongjoon-hyun changed the title ~~[SPARK-33619][SQL] GetMapValueUtil code generation error.~~ [SPARK-33619][SQL] Fix GetMapValueUtil code generation error Dec 1, 2020

leanken-zz force-pushed the leanken-SPARK-33619 branch from be5093a to 6d4fb1d Compare December 2, 2020 02:37

first draft.

16940ce

Change-Id: Icdd9c60778b9fa155e4b2cb62e2e6d841a43c174

leanken-zz force-pushed the leanken-SPARK-33619 branch from 8e4ca3d to 16940ce Compare December 2, 2020 05:28

leanken-zz commented Dec 2, 2020

View reviewed changes

fix ExpressionEvalHelperSuite failures.

6570d70

Change-Id: I035eca72ebdc6d79676fe14cc79ac4ac2eb4430c

cloud-fan reviewed Dec 2, 2020

View reviewed changes

add UT for checkExceptionInExpression.

95bdd37

Change-Id: Ifcd5d186c16879159b84223dbc79fcf0ec628c81

cloud-fan approved these changes Dec 2, 2020

View reviewed changes

cloud-fan closed this Dec 2, 2020

cloud-fan reopened this Dec 2, 2020

cloud-fan closed this in 58583f7 Dec 2, 2020

Conversation

leanken-zz commented Dec 1, 2020 • 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!

leanken-zz commented Dec 1, 2020

Uh oh!

leanken-zz commented Dec 1, 2020

Uh oh!

HyukjinKwon Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

leanken-zz Dec 1, 2020 • 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!

leanken-zz Dec 1, 2020 • 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!

maropu Dec 1, 2020 • 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!

dongjoon-hyun Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

leanken-zz commented Dec 2, 2020

Uh oh!

SparkQA commented Dec 2, 2020

Uh oh!

leanken-zz Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 2, 2020

Uh oh!

maropu commented Dec 2, 2020

Uh oh!

leanken-zz commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Dec 2, 2020

Uh oh!

cloud-fan Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

leanken-zz Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

maropu commented Dec 2, 2020

Uh oh!

leanken-zz commented Dec 2, 2020

Uh oh!

leanken-zz commented Dec 2, 2020

Uh oh!

SparkQA commented Dec 2, 2020

Uh oh!

cloud-fan commented Dec 2, 2020

Uh oh!

SparkQA commented Dec 2, 2020

Uh oh!

cloud-fan commented Dec 2, 2020

Uh oh!

SparkQA commented Dec 2, 2020

Uh oh!

leanken-zz commented Dec 3, 2020

Uh oh!

dongjoon-hyun commented Jan 4, 2021

Uh oh!

HyukjinKwon commented Jan 6, 2021

Uh oh!

LuciferYang commented Jan 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

leanken-zz commented Dec 1, 2020 •

edited

Loading

leanken-zz Dec 1, 2020 •

edited

Loading

leanken-zz Dec 1, 2020 •

edited

Loading

maropu Dec 1, 2020 •

edited

Loading

leanken-zz commented Dec 2, 2020 •

edited

Loading