[FLINK-15266] Fix NPE for case operator code gen in blink planner #10594

libenchao · 2019-12-16T12:59:49Z

What is the purpose of the change

This pr fixes the NPE bug in blink case operator code gen as described in FLINK-15266.

Brief change log

For case operator result type, generate primitive type only when trueAction and falseAction both return non-null types.

Verifying this change

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (yes)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)

flinkbot · 2019-12-16T13:03:48Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 2c31b9b (Mon Dec 16 13:03:48 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-12-16T13:43:57Z

CI report:

2c31b9b Travis: FAILURE Azure: FAILURE
4d89fc7 Travis: FAILURE Azure: FAILURE
c819f09 Travis: CANCELED Azure: FAILURE
1e57bc7 Travis: SUCCESS Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

KurtYoung

Thanks for fixing this! Could you also add a test case?

KurtYoung · 2019-12-17T07:12:02Z

Please fix the code style error as well.

wuchong · 2019-12-19T09:26:58Z

Hi @libenchao , do you have time to add some tests for this?

As this is a blocker issue of release 1.10, if you don't have enough time, I can help to improve the fixing.

libenchao · 2019-12-19T09:34:46Z

@KurtYoung @wuchong Thanks for the review. Sorry for the late reply.

About the tests, I've been working around. I'd appreciate it if you could give some advice here. I come out with two testing methods:

Just add a ITCaseTest to cover this scenario, like select case when 0 < EXPR$0 then cast(EXPR$1 as bigint) else 0 end from (values (1, ''), (2, '12')).
Like other code gen testings, build a GeneratedClass, get it's instance, invoke it's method, verify the result.

JingsongLi · 2019-12-19T11:32:36Z

Hi @libenchao , thanks for fixing, I'll review this.

JingsongLi · 2019-12-19T11:35:10Z

About the tests:
1.If possible, please add test in ScalarOperatorsTest. This is the first choice.
2.If it is hard to add test in expression tests, should add test in CalcITCase.

This case, I think you can try # 1 first.

JingsongLi · 2019-12-19T11:36:37Z

...r-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala

@@ -1278,7 +1278,11 @@ object ScalarOperatorGens {
      val falseAction = generateIfElse(ctx, operands, resultType, i + 2)

      val Seq(resultTerm, nullTerm) = newNames("result", "isNull")
-      val resultTypeTerm = primitiveTypeTermForType(resultType)
+      val resultTypeTerm = if (trueAction.resultType.isNullable || falseAction.resultType.isNullable) {
+        boxedTypeTermForType(resultType)


You should not modify here. We always use primitiveTypeTermForType as much as possible.

JingsongLi · 2019-12-19T11:37:30Z

...r-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala

+        boxedTypeTermForType(resultType)
+      } else {
+        primitiveTypeTermForType(resultType)
+      }

      val operatorCode = if (ctx.nullCheck) {
        s"""


I think the reason is here, should first check nullTerm, if it is true, should not assign resultTerm.

Thanks for the review. The result for NPE is that we assign null to a primitive type variable here, if we not assign resultTerm when nullTerm is true, then resultTerm will not be null at last. IMHO, this should be null for resultTerm.

Or we can make ${trueAction.resultTerm} be the resultTerm for the returned GeneratedExpression ?

Ignore my above comment...
I've got your point, and will update the code very soon.

JingsongLi · 2019-12-20T02:04:17Z

...r-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala

@@ -1287,12 +1287,16 @@ object ScalarOperatorGens {
           |boolean $nullTerm;
           |if (${condition.resultTerm}) {
           |  ${trueAction.code}
-           |  $resultTerm = ${trueAction.resultTerm};
+           |  if (!${trueAction.nullTerm}) {


This will lead to a risk, if trueAction.nullTerm is a expression instead of a variable, maybe will some problem, so I suggest you:

$nullTerm = ${trueAction.nullTerm}; if (!$nullTerm) { $resultTerm = ${trueAction.resultTerm}; }

At least, we can ensure nullTerm is just a variable.

sure, thanks for the suggestion.
code updated.

BTW, I encountered below error running ScalarOperatorsTest after updating the code:

java.lang.VerifyError: Expecting a stackmap frame at branch target 32 Exception Details: Location: TestFunction$163.map(Ljava/lang/Object;)Ljava/lang/Object; @19: ifne Reason: Expected stackmap frame at this location. Bytecode: 0x0000000: 2bc0 000e 4d2c 100d b900 1402 0036 0401 0x0000010: 4e15 049a 000d 2c10 0d05 b900 1803 004e 0x0000020: 2d3a 4415 049a 000e 2ab4 001c 1944 b600 0x0000030: 223a 442c 05b9 0014 0200 3632 0236 3115 ......

It seems to be a bug in Janino. After I searched Janino's issues, I found this janino-compiler/janino#90 (comment)

Have you ever met the same problem?

Never encounter... But maybe because our code generate code has some problem, we can run again after fixing.

@libenchao @JingsongLi

Hello! This might be a bit unrelated, but I am developing on flink and I am also running into a very similar VerifyError pointing to a Janino code generated method (specifically in the Flink protobuf row serialization codepath). How did you workaround/resolve this issue?

@kurnoolsaketh We didn't solve it, it just disappeared.

interesting.. thanks for the info.

JingsongLi · 2019-12-20T02:36:04Z

...r-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala

           |  $nullTerm = ${trueAction.nullTerm};
+           |  if (!$nullTerm) {
+           |    $resultTerm = ${trueAction.resultTerm};


You need assign resultTerm to a default value too.
val defaultValue = primitiveDefaultValue(resultType)

thanks for the reminder. done.

JingsongLi

LGTM after travis passing.

libenchao · 2019-12-20T03:10:52Z

@flinkbot run travis

wuchong

Thanks for the fixing @libenchao and thanks for the reviewing @JingsongLi .

Looks good to me too.

Will merge it.

…EN operator in blink planner (#10594)

[FLINK-15266] Fix NPE for case operator code gen in blink planner

2c31b9b

rmetzger added the review=description? label Dec 16, 2019

rmetzger added the component=TableSQL/Runtime label Dec 16, 2019

KurtYoung reviewed Dec 17, 2019

View reviewed changes

KurtYoung self-assigned this Dec 17, 2019

JingsongLi reviewed Dec 19, 2019

View reviewed changes

add tests, and check null before assignment

4d89fc7

JingsongLi reviewed Dec 20, 2019

View reviewed changes

Use instead of GeneratedExpression's nullTerm

c819f09

JingsongLi reviewed Dec 20, 2019

View reviewed changes

add default value to resultTerm

1e57bc7

JingsongLi approved these changes Dec 20, 2019

View reviewed changes

wuchong approved these changes Dec 20, 2019

View reviewed changes

wuchong merged commit 5f86f2d into apache:master Dec 20, 2019

wuchong pushed a commit that referenced this pull request Dec 20, 2019

[FLINK-15266][table-planner-blink] Fix NPE for code generated CASE WH…

319466a

…EN operator in blink planner (#10594)

wuchong pushed a commit that referenced this pull request Dec 20, 2019

[FLINK-15266][table-planner-blink] Fix NPE for code generated CASE WH…

73c82c3

…EN operator in blink planner (#10594)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-15266] Fix NPE for case operator code gen in blink planner #10594

[FLINK-15266] Fix NPE for case operator code gen in blink planner #10594

libenchao commented Dec 16, 2019

flinkbot commented Dec 16, 2019

flinkbot commented Dec 16, 2019 •

edited

Loading

KurtYoung left a comment

KurtYoung commented Dec 17, 2019

wuchong commented Dec 19, 2019

libenchao commented Dec 19, 2019

JingsongLi commented Dec 19, 2019

JingsongLi commented Dec 19, 2019 •

edited

Loading

JingsongLi Dec 19, 2019

JingsongLi Dec 19, 2019

libenchao Dec 19, 2019

libenchao Dec 19, 2019

JingsongLi Dec 20, 2019

libenchao Dec 20, 2019

libenchao Dec 20, 2019

JingsongLi Dec 20, 2019

kurnoolsaketh May 4, 2023

libenchao May 5, 2023

kurnoolsaketh May 5, 2023

JingsongLi Dec 20, 2019

libenchao Dec 20, 2019

JingsongLi left a comment

libenchao commented Dec 20, 2019

wuchong left a comment

[FLINK-15266] Fix NPE for case operator code gen in blink planner #10594

[FLINK-15266] Fix NPE for case operator code gen in blink planner #10594

Conversation

libenchao commented Dec 16, 2019

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Dec 16, 2019

Automated Checks

Review Progress

flinkbot commented Dec 16, 2019 • edited Loading

CI report:

KurtYoung left a comment

Choose a reason for hiding this comment

KurtYoung commented Dec 17, 2019

wuchong commented Dec 19, 2019

libenchao commented Dec 19, 2019

JingsongLi commented Dec 19, 2019

JingsongLi commented Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JingsongLi left a comment

Choose a reason for hiding this comment

libenchao commented Dec 20, 2019

wuchong left a comment

Choose a reason for hiding this comment

flinkbot commented Dec 16, 2019 •

edited

Loading

JingsongLi commented Dec 19, 2019 •

edited

Loading