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/05/24 14:24:16 UTC
[GitHub] [flink] lsyldliu opened a new pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
lsyldliu opened a new pull request #12303:
URL: https://github.com/apache/flink/pull/12303
## What is the purpose of the change
fix the bug of ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
## Brief change log
- *fix the bug of ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction*
- *fix the bug of currentTopNum will always increase even remove retired element in AppendOnlyTopNFunction#processElementWithoutRowNumber method*
## Verifying this change
This change is already covered by existing tests in TopNFunctionTestBase.
## 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, Kubernetes/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 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] lsyldliu commented on a change in pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on a change in pull request #12303:
URL: https://github.com/apache/flink/pull/12303#discussion_r435658176
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
Review comment:
But, I find `TopNBuffer#removeLast` also iterate the list, the efficient problem also exist.
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12303:
URL: https://github.com/apache/flink/pull/12303#discussion_r435244412
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
Review comment:
Remove on the list use index will be more efficient than `buffer.remove`.
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
+ }
+ if (size <= 1) {
buffer.removeAll(lastKey);
dataState.remove(lastKey);
} else {
+ buffer.remove(lastKey, lastElement);
dataState.put(lastKey, lastList);
Review comment:
```suggestion
dataState.put(lastKey, new ArrayList<>(lastList));
```
We should do a shallow copy for the `lastList`, see FLINK-17918.
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633239070
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 3f6b52073802835bc193796a41d095c5670f5d59 (Sun May 24 14:26:16 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] lsyldliu commented on a change in pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on a change in pull request #12303:
URL: https://github.com/apache/flink/pull/12303#discussion_r435652445
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
Review comment:
Good idea, I don't want to break the atomicity of `TopNBuffer`, so I don't want to add new method which can operate currentTopNum directly in this class, prefer to use `TopNBuffer#removeLast`
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3f6b52073802835bc193796a41d095c5670f5d59 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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12303:
URL: https://github.com/apache/flink/pull/12303#discussion_r435340328
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
Review comment:
Good point. I just find that maybe we can use `TopNBuffer#removeLast` directly here.
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12303:
URL: https://github.com/apache/flink/pull/12303#discussion_r435243944
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
Review comment:
```suggestion
lastElement = lastList.remove(size - 1);
```
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
}, {
"hash" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854",
"triggerID" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"triggerType" : "PUSH"
}, {
"hash" : "f09242997da0a62bc31ae611aa66c9f9e05fdf2f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2894",
"triggerID" : "f09242997da0a62bc31ae611aa66c9f9e05fdf2f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1c92b48de9d7b9068a646053e7dc9b4d076625ab Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854)
* f09242997da0a62bc31ae611aa66c9f9e05fdf2f Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2894)
<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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3f6b52073802835bc193796a41d095c5670f5d59 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081)
<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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3f6b52073802835bc193796a41d095c5670f5d59 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081)
<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] lsyldliu commented on pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-640573716
> minor
@wuchong , thanks for your work, it looks good to me.
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
}, {
"hash" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854",
"triggerID" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"triggerType" : "PUSH"
}, {
"hash" : "f09242997da0a62bc31ae611aa66c9f9e05fdf2f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "f09242997da0a62bc31ae611aa66c9f9e05fdf2f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1c92b48de9d7b9068a646053e7dc9b4d076625ab Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854)
* f09242997da0a62bc31ae611aa66c9f9e05fdf2f 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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
}, {
"hash" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854",
"triggerID" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3f6b52073802835bc193796a41d095c5670f5d59 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081)
* 1c92b48de9d7b9068a646053e7dc9b4d076625ab Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854)
<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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
}, {
"hash" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854",
"triggerID" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"triggerType" : "PUSH"
}, {
"hash" : "f09242997da0a62bc31ae611aa66c9f9e05fdf2f",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2894",
"triggerID" : "f09242997da0a62bc31ae611aa66c9f9e05fdf2f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f09242997da0a62bc31ae611aa66c9f9e05fdf2f Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2894)
<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 merged pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
wuchong merged pull request #12303:
URL: https://github.com/apache/flink/pull/12303
----------------------------------------------------------------
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] lsyldliu commented on pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-639951595
@wuchong Thanks for your review and I've addressed your comments.
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
wuchong commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-640359798
It looks good to me in general. I just push a minor commit to improve the code. Please have a look @lsyldliu.
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
}, {
"hash" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854",
"triggerID" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1c92b48de9d7b9068a646053e7dc9b4d076625ab Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2854)
<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] lsyldliu commented on a change in pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on a change in pull request #12303:
URL: https://github.com/apache/flink/pull/12303#discussion_r435333138
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
Review comment:
@wuchong I agree with you, but I think we also should subtract buffer.currentTopNum = buffer.currentTopNum - 1 after remove lastElement, So is it appropriate to add a method in TopNBuffer such as `public void minusTopNum(int delta) {
currentTopNum -= delta;
}`,
we should call this method behind `dataState.put(lastKey, new ArrayList<>(lastList))`, what do you think it?
----------------------------------------------------------------
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] lsyldliu commented on pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-638737701
cc @beyond1920
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633242378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "3f6b52073802835bc193796a41d095c5670f5d59",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081",
"triggerID" : "3f6b52073802835bc193796a41d095c5670f5d59",
"triggerType" : "PUSH"
}, {
"hash" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "1c92b48de9d7b9068a646053e7dc9b4d076625ab",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 3f6b52073802835bc193796a41d095c5670f5d59 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=2081)
* 1c92b48de9d7b9068a646053e7dc9b4d076625ab 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] lsyldliu commented on pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-633238883
@flinkbot
----------------------------------------------------------------
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] lsyldliu commented on pull request #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
lsyldliu commented on pull request #12303:
URL: https://github.com/apache/flink/pull/12303#issuecomment-638736205
cc @wuchong
----------------------------------------------------------------
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 #12303: [FLINK-17625] [table] Fix ArrayIndexOutOfBoundsException in AppendOnlyTopNFunction
Posted by GitBox <gi...@apache.org>.
wuchong commented on a change in pull request #12303:
URL: https://github.com/apache/flink/pull/12303#discussion_r435667861
##########
File path: flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/runtime/operators/rank/AppendOnlyTopNFunction.java
##########
@@ -210,14 +210,19 @@ private void processElementWithoutRowNumber(RowData input, Collector<RowData> ou
RowData lastKey = lastEntry.getKey();
List<RowData> lastList = (List<RowData>) lastEntry.getValue();
// remove last one
- RowData lastElement = lastList.remove(lastList.size() - 1);
- if (lastList.isEmpty()) {
+ int size = lastList.size();
+ RowData lastElement = null;
+ if (size > 0) {
+ lastElement = lastList.get(size - 1);
Review comment:
Yes. I think we should improve `TopNBuffer#removeLast`, we can call `remove(index)` and `get(index)` when it is a `List`.
----------------------------------------------------------------
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