You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/06/02 08:33:45 UTC
[GitHub] [flink] leonardBang opened a new pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
leonardBang opened a new pull request #12436:
URL: https://github.com/apache/flink/pull/12436
## What is the purpose of the change
* This pull request fix ArrayIndexOutOfBoundsException happens in codegen operator.
- We throw an exception in compile(codegen) phase if the index <=0;
- We follow the Calcite to return a null value when visited a invalid index in runtime.
## Brief change log
* Update file org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
## Verifying this change
* Add unit tests for SQL and Table API.
## 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? (not applicable / docs / JavaDocs / not documented)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e98155d0d16e4c8aff16f018a349587cdadadede Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558)
* ebc9048a44859813911595912641e087d8ce4448 UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] leonardBang commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
leonardBang commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r433724667
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
I think the `idxStr` has a certain value in runtime, both for `idx < 0` and `idx >= array.size()` we should return null.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3078f1dc40f7f79e96270051eec8bf22e3b90a6b Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552)
* e98155d0d16e4c8aff16f018a349587cdadadede Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3078f1dc40f7f79e96270051eec8bf22e3b90a6b Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552)
* e98155d0d16e4c8aff16f018a349587cdadadede UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] libenchao commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
libenchao commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r433776897
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
consider this case:
```sql
CREATE TABLE my_source (
idx int,
my_arr array<int>
) WITH (...);
SELECT my_arr[idx] FROM my_source;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e98155d0d16e4c8aff16f018a349587cdadadede Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] leonardBang commented on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
leonardBang commented on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-638018884
I updated the PR @libenchao @wuchong ,could you take a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* ebc9048a44859813911595912641e087d8ce4448 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612)
* d553feca300e2668e3e5c16c1c524d73acde141c Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong closed pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
wuchong closed pull request #12436:
URL: https://github.com/apache/flink/pull/12436
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] libenchao commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
libenchao commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r433717924
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
Review comment:
If we cannot do this in calcite's parsing/validating phase, it's ok to me to check it here.
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
`${array.resultTerm}` maybe null?
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
it seems `idxStr` could ` < 0` at runtime (if it's not a literal, and cannot validate before runtime), I think we can throw exception for this case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] leonardBang commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
leonardBang commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r434381251
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1648,28 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ index.literalValue match {
+ case Some(v: Int) if v < 1 =>
+ throw new ValidationException(
+ s"Array element access needs an index starting at 1 but was $v.")
+ case _ => //nothing
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
Review comment:
the generated code will too long If not, I'm ok to keep them in shorter lines
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r433775688
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
Review comment:
"Array element reference requires an index starts from 1, but was"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
}, {
"hash" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711",
"triggerID" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"triggerType" : "PUSH"
}, {
"hash" : "63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d553feca300e2668e3e5c16c1c524d73acde141c Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627)
* 4d1f156fd758a44f4c394a478b6f37bc2de61c86 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711)
* 63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] leonardBang commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
leonardBang commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r433815328
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
Review comment:
> "Array element reference requires an index starts from 1, but was"
This text is same to [existed Table API validation](https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/expressions/collection.scala#L212) so that we can keep align.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] libenchao commented on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
libenchao commented on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-638829696
@wuchong I have no more concerns, the pr LGTM now, +1 for merging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
}, {
"hash" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d553feca300e2668e3e5c16c1c524d73acde141c Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627)
* 4d1f156fd758a44f4c394a478b6f37bc2de61c86 UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3078f1dc40f7f79e96270051eec8bf22e3b90a6b Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552)
* e98155d0d16e4c8aff16f018a349587cdadadede UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r433776867
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
I think it's better to return null than throwing exception, because streaming is an online service. Besides, the definition of element accessing in Calcite also returns null for array element error. `org.apache.calcite.sql.fun.SqlStdOperatorTable#ITEM`
But could you add description for this on documentation? @leonardBang
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] leonardBang commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
leonardBang commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r433724667
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
I think the `idxStr` has a certain value in runtime, both for `idx < 0` and 'idx >= array.size' we should return null.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] leonardBang commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
leonardBang commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r434295803
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
> consider this case:
>
> ```sql
> CREATE TABLE my_source (
> idx int,
> my_arr array<int>
> ) WITH (...);
>
> SELECT my_arr[idx] FROM my_source;
> ```
For this case, idx has been an integer value rather than an input reference in runtime IIUC.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] libenchao commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
libenchao commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r434372145
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1648,28 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ index.literalValue match {
+ case Some(v: Int) if v < 1 =>
+ throw new ValidationException(
+ s"Array element access needs an index starting at 1 but was $v.")
+ case _ => //nothing
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
Review comment:
why change the indent?
##########
File path: flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/ArrayTypeTest.scala
##########
@@ -375,4 +375,27 @@ class ArrayTypeTest extends ArrayTypeTestBase {
"[1984-03-12, 1984-02-10]"
)
}
+
+ @Test
+ def testArrayIndexStaticCheckForTableApi(): Unit = {
+ thrown.expect(classOf[ValidationException])
+ thrown.expectMessage("Array element access needs an index starting at 1 but was 0.")
+ testTableApi( 'f2.at(0), "1")
Review comment:
additional space.
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1648,28 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ index.literalValue match {
+ case Some(v: Int) if v < 1 =>
+ throw new ValidationException(
+ s"Array element access needs an index starting at 1 but was $v.")
+ case _ => //nothing
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |boolean $nullTerm = ${array.nullTerm} ||
+ | ${index.nullTerm} ||
+ | ${idxStr} < 0 ||
+ | ${idxStr} >= ${array.resultTerm}.size() ||
+ | $arrayIsNull;
Review comment:
`$arrayIsNull` should be put before `${idxStr} >= ${array.resultTerm}.size()`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r435038062
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1648,25 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ index.literalValue match {
+ case Some(v: Int) if v < 1 =>
+ throw new ValidationException(
+ s"Array element access needs an index starting at 1 but was $v.")
+ case _ => //nothing
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull ||
Review comment:
Move `$arrayIsNull` to the end? Otherwise, it is possible to throw `ArrayIndexOutOfBoundsException`. You can try to test `f11[3]`. Maybe we should rename `arrayIsNull` to `arrayElementIsNull`.
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1648,25 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ index.literalValue match {
+ case Some(v: Int) if v < 1 =>
+ throw new ValidationException(
+ s"Array element access needs an index starting at 1 but was $v.")
+ case _ => //nothing
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull ||
+ | ${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size();
Review comment:
`${idxStr}` -> `$idxStr`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3078f1dc40f7f79e96270051eec8bf22e3b90a6b Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
wuchong commented on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-638765700
Do you have other concerns @libenchao ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d553feca300e2668e3e5c16c1c524d73acde141c Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
}, {
"hash" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711",
"triggerID" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"triggerType" : "PUSH"
}, {
"hash" : "63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2721",
"triggerID" : "63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4d1f156fd758a44f4c394a478b6f37bc2de61c86 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711)
* 63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2721)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e98155d0d16e4c8aff16f018a349587cdadadede Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558)
* ebc9048a44859813911595912641e087d8ce4448 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612)
* d553feca300e2668e3e5c16c1c524d73acde141c UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
wuchong commented on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-638894806
Build is passed in my repo: https://dev.azure.com/imjark/Flink/_build/results?buildId=136&view=results
Merging...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
}, {
"hash" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711",
"triggerID" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"triggerType" : "PUSH"
}, {
"hash" : "63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2721",
"triggerID" : "63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d553feca300e2668e3e5c16c1c524d73acde141c Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627)
* 4d1f156fd758a44f4c394a478b6f37bc2de61c86 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711)
* 63831f0ba3dc2a6ec31acc7fb1cfd1b51288945b Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2721)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
}, {
"hash" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711",
"triggerID" : "4d1f156fd758a44f4c394a478b6f37bc2de61c86",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d553feca300e2668e3e5c16c1c524d73acde141c Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627)
* 4d1f156fd758a44f4c394a478b6f37bc2de61c86 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2711)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] libenchao commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
libenchao commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r434297494
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
What I want to express is, the index could be a literal or a field reference.
And we want to throw Exception for index < 1, there are two cases that index < 1:
- literal value, it has been checked in code gen, and throws ValidationException.
- field reference, currently you just return null for this case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3078f1dc40f7f79e96270051eec8bf22e3b90a6b UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637384193
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 3078f1dc40f7f79e96270051eec8bf22e3b90a6b (Tue Jun 02 08:35:50 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## 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](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
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 <summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12436:
URL: https://github.com/apache/flink/pull/12436#issuecomment-637399261
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2552",
"triggerID" : "3078f1dc40f7f79e96270051eec8bf22e3b90a6b",
"triggerType" : "PUSH"
}, {
"hash" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558",
"triggerID" : "e98155d0d16e4c8aff16f018a349587cdadadede",
"triggerType" : "PUSH"
}, {
"hash" : "ebc9048a44859813911595912641e087d8ce4448",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612",
"triggerID" : "ebc9048a44859813911595912641e087d8ce4448",
"triggerType" : "PUSH"
}, {
"hash" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627",
"triggerID" : "d553feca300e2668e3e5c16c1c524d73acde141c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e98155d0d16e4c8aff16f018a349587cdadadede Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2558)
* ebc9048a44859813911595912641e087d8ce4448 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2612)
* d553feca300e2668e3e5c16c1c524d73acde141c Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2627)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wuchong commented on a change in pull request #12436: [FLINK-17847][table sql / planner] ArrayIndexOutOfBoundsException happens in StreamExecCalc operator
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12436:
URL: https://github.com/apache/flink/pull/12436#discussion_r434317766
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,32 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
Review comment:
Can be simplified to
```scala
index.literalValue match {
case Some(v: Int) if v < 1 =>
throw new ValidationException(
s"Array element access needs an index starting at 1 but was $v")
case _ => // nothing
}
```
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,32 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
Review comment:
`* @see [[org.apache.calcite.sql.fun.SqlStdOperatorTable.ITEM]]`
Could you move this Javadoc to the above of method signature? Javadoc is usually not used in code.
##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##########
@@ -1643,19 +1644,35 @@ object ScalarOperatorGens {
val resultTypeTerm = primitiveTypeTermForType(componentInfo)
val defaultTerm = primitiveDefaultValue(componentInfo)
+ if (index.literalValue.isDefined &&
+ index.literalValue.get.isInstanceOf[Int] &&
+ index.literalValue.get.asInstanceOf[Int] < 1) {
+ throw new ValidationException(s"Array element access needs an index starting at 1 but was " +
+ s"${index.literalValue.get.asInstanceOf[Int]}.")
+ }
val idxStr = s"${index.resultTerm} - 1"
val arrayIsNull = s"${array.resultTerm}.isNullAt($idxStr)"
val arrayGet =
rowFieldReadAccess(ctx, idxStr, array.resultTerm, componentInfo)
+ /**
+ * Return null when array index out of bounds which follows Calcite's behaviour.
+ * @see org.apache.calcite.sql.fun.SqlStdOperatorTable
+ */
val arrayAccessCode =
- s"""
- |${array.code}
- |${index.code}
- |boolean $nullTerm = ${array.nullTerm} || ${index.nullTerm} || $arrayIsNull;
- |$resultTypeTerm $resultTerm = $nullTerm ? $defaultTerm : $arrayGet;
- |""".stripMargin
-
+ s"""
+ |${array.code}
+ |${index.code}
+ |$resultTypeTerm $resultTerm;
+ |boolean $nullTerm;
+ |if (${idxStr} < 0 || ${idxStr} >= ${array.resultTerm}.size()) {
Review comment:
I got what's your concern @libenchao . I think we don't have divergency on the runtime null behavior, the only concern is the compile exception. But I still prefer the current way, because it's very likely a typo if `my_arr[0]` is used, so a hint exception is more useful than surprising result.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org