You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by Xpray <gi...@git.apache.org> on 2018/05/11 05:10:06 UTC
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
GitHub user Xpray opened a pull request:
https://github.com/apache/flink/pull/5988
[FLINK-9332][TableAPI & SQL] Fix Codegen error of CallGenerator
## What is the purpose of the change
Fix bugs that nullTerm do not handle function call return value correctly.
## Brief change log
- check function call return value and reset nullTerm if necessary.
## 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:
- 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): no
- 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
- If yes, how is the feature documented? no
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Xpray/flink FLINK-9332
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5988.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #5988
----
commit 6feff0619846d3fb04b10566cb532b9ce5329128
Author: Xpray <le...@...>
Date: 2018-05-11T05:06:31Z
[FLINK-9332][TableAPI & SQL] Fix Codegen error of CallGenerator
----
---
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
Posted by Xpray <gi...@git.apache.org>.
Github user Xpray commented on a diff in the pull request:
https://github.com/apache/flink/pull/5988#discussion_r187636169
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/CallGenerator.scala ---
@@ -64,17 +65,28 @@ object CallGenerator {
val (auxiliaryStmt, result) = call(operands.map(_.resultTerm))
+ val nullTermCode = if (
+ nullCheck &&
+ isReference(returnType) &&
+ !TypeCheckUtils.isTemporal(returnType)) {
--- End diff --
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) {}
---
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/5988#discussion_r187575678
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/CallGenerator.scala ---
@@ -64,17 +65,28 @@ object CallGenerator {
val (auxiliaryStmt, result) = call(operands.map(_.resultTerm))
+ val nullTermCode = if (
+ nullCheck &&
+ isReference(returnType) &&
+ !TypeCheckUtils.isTemporal(returnType)) {
+ s"""
+ |if ($resultTerm == null) {
+ | $nullTerm = true;
+ |}
+ """.stripMargin
+ } else {
+ ""
+ }
+
val resultCode = if (nullCheck && operands.nonEmpty) {
s"""
|${operands.map(_.code).mkString("\n")}
|boolean $nullTerm = ${operands.map(_.nullTerm).mkString(" || ")};
- |$resultTypeTerm $resultTerm;
- |if ($nullTerm) {
- | $resultTerm = $defaultValue;
--- End diff --
don't we need this case if the result value is a primitive?
---
[GitHub] flink issue #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of CallGen...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/5988
Merging
---
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/5988#discussion_r187568481
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/CallGenerator.scala ---
@@ -64,17 +65,28 @@ object CallGenerator {
val (auxiliaryStmt, result) = call(operands.map(_.resultTerm))
+ val nullTermCode = if (
+ nullCheck &&
+ isReference(returnType) &&
+ !TypeCheckUtils.isTemporal(returnType)) {
--- End diff --
Why do you exclude temporal types here?
---
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/5988#discussion_r187710610
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/CallGenerator.scala ---
@@ -64,17 +65,28 @@ object CallGenerator {
val (auxiliaryStmt, result) = call(operands.map(_.resultTerm))
+ val nullTermCode = if (
+ nullCheck &&
+ isReference(returnType) &&
+ !TypeCheckUtils.isTemporal(returnType)) {
+ s"""
+ |if ($resultTerm == null) {
+ | $nullTerm = true;
+ |}
+ """.stripMargin
+ } else {
+ ""
+ }
+
val resultCode = if (nullCheck && operands.nonEmpty) {
s"""
|${operands.map(_.code).mkString("\n")}
|boolean $nullTerm = ${operands.map(_.nullTerm).mkString(" || ")};
- |$resultTypeTerm $resultTerm;
- |if ($nullTerm) {
- | $resultTerm = $defaultValue;
--- End diff --
Oh sorry. Overlooked that one. Thanks!
---
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
Posted by Xpray <gi...@git.apache.org>.
Github user Xpray commented on a diff in the pull request:
https://github.com/apache/flink/pull/5988#discussion_r187636663
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/CallGenerator.scala ---
@@ -64,17 +65,28 @@ object CallGenerator {
val (auxiliaryStmt, result) = call(operands.map(_.resultTerm))
+ val nullTermCode = if (
+ nullCheck &&
+ isReference(returnType) &&
+ !TypeCheckUtils.isTemporal(returnType)) {
+ s"""
+ |if ($resultTerm == null) {
+ | $nullTerm = true;
+ |}
+ """.stripMargin
+ } else {
+ ""
+ }
+
val resultCode = if (nullCheck && operands.nonEmpty) {
s"""
|${operands.map(_.code).mkString("\n")}
|boolean $nullTerm = ${operands.map(_.nullTerm).mkString(" || ")};
- |$resultTypeTerm $resultTerm;
- |if ($nullTerm) {
- | $resultTerm = $defaultValue;
--- End diff --
I've set the value to defaultValue at line 85
---
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/5988#discussion_r187577471
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/stream/sql/SqlITCase.scala ---
@@ -686,6 +686,28 @@ class SqlITCase extends StreamingWithStateTestBase {
assertEquals(List(expected.toString()), StreamITCase.testResults.sorted)
}
+
+ @Test
+ def testNullableFunctionCall(): Unit = {
--- End diff --
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
---
[GitHub] flink pull request #5988: [FLINK-9332][TableAPI & SQL] Fix Codegen error of ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5988
---