[FLINK-9332][TableAPI & SQL] Fix Codegen error of CallGenerator #5988
Conversation
| val nullTermCode = if ( | ||
| nullCheck && | ||
| isReference(returnType) && | ||
| !TypeCheckUtils.isTemporal(returnType)) { |
There was a problem hiding this comment.
Why do you exclude temporal types here?
There was a problem hiding this comment.
In flink we use primitive types such as 'long' in code generation while it's returnType is a time type with isReference is true, so I exclude it to avoid these cases like
long a; if (a == null) {}
| val resultCode = if (nullCheck && operands.nonEmpty) { | ||
| s""" | ||
| |${operands.map(_.code).mkString("\n")} | ||
| |boolean $nullTerm = ${operands.map(_.nullTerm).mkString(" || ")}; | ||
| |$resultTypeTerm $resultTerm; | ||
| |if ($nullTerm) { | ||
| | $resultTerm = $defaultValue; |
There was a problem hiding this comment.
don't we need this case if the result value is a primitive?
There was a problem hiding this comment.
I've set the value to defaultValue at line 85
There was a problem hiding this comment.
Oh sorry. Overlooked that one. Thanks!
| @@ -686,6 +686,28 @@ class SqlITCase extends StreamingWithStateTestBase { | |||
|
|
|||
| assertEquals(List(expected.toString()), StreamITCase.testResults.sorted) | |||
| } | |||
|
|
|||
| @Test | |||
| def testNullableFunctionCall(): Unit = { | |||
There was a problem hiding this comment.
We should test this more lightweight with a unit test instead of running a complete query.
We can extend ScalarFunctionsTest.testLPad() with
testSqlApi(
"LPAD('hello', -1, 'x') IS NULL", "true"
)
to cover this case
|
Merging |
What is the purpose of the change
Fix bugs that nullTerm do not handle function call return value correctly.
Brief change log
Verifying this change
This change added tests and can be verified as follows:
org.apache.flink.table.runtime.stream.sql.SqlITCase#testNullableFunctionCall
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation