You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "J-HowHuang (via GitHub)" <gi...@apache.org> on 2026/06/22 21:07:04 UTC
[PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
J-HowHuang opened a new pull request, #18834:
URL: https://github.com/apache/pinot/pull/18834
## Desciption
H3 index could run into exception
```
2026/06/19 23:02:10.190 ERROR [TaskFactoryRegistry] [TaskStateModelFactory-task_thread-2] Caught
exception while executing task:
Task_SegmentRefreshTask_6e158344-12a2-401e-bc11-5464d0df28af_1781910006722_11
java.lang.IndexOutOfBoundsException
at java.base/java.nio.Buffer$1.apply(Buffer.java:757)
at java.base/java.nio.Buffer$1.apply(Buffer.java:754)
at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:213)
at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:210)
at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:98)
at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:106)
at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:302)
at java.base/java.nio.Buffer.checkIndex(Buffer.java:779)
at java.base/java.nio.DirectByteBuffer.getLong(DirectByteBuffer.java:861)
at org.apache.pinot.segment.spi.memory.PinotByteBuffer.getLong(PinotByteBuffer.java:181)
at org.apache.pinot.segment.local.segment.creator.impl.inv.geospatial.OffHeapH3IndexCreator$ChunkIt
erator.next(OffHeapH3IndexCreator.java:193)
at org.apache.pinot.segment.local.segment.creator.impl.inv.geospatial.OffHeapH3IndexCreator.seal(Of
fHeapH3IndexCreator.java:138)
at org.apache.pinot.segment.local.segment.creator.impl.ColumnIndexCreators.seal(ColumnIndexCreators
.java:98)
```
because empty chunk could be added into the chunk list. An empty chunk would cause out of bound read in `ChunkIterator`
## Change
* Don't add when posting list is empty while flush
* Check and skip empty chunk while iterating
## Test
Added an unit test and verified it would fail without the patch
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18834:
URL: https://github.com/apache/pinot/pull/18834#discussion_r3455786823
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/OffHeapH3IndexCreator.java:
##########
@@ -83,6 +83,11 @@ public void add(@Nullable Geometry geometry)
*/
private void flush()
throws IOException {
+ // Skip flushing an empty posting list map. This could happen when add() didn't add any entry due to invalid
+ // Geometry and _continueOnError is set
+ if (_postingListMap.isEmpty()) {
Review Comment:
`flush()` shouldn't be invoked when `_postingListMap` is empty. Consider changing the check on line 76
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18834:
URL: https://github.com/apache/pinot/pull/18834#discussion_r3456189822
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/OffHeapH3IndexCreator.java:
##########
@@ -113,7 +115,7 @@ public void seal()
}
// Flush the last chunk
- if (_postingListMap.size() % FLUSH_THRESHOLD != 0) {
+ if (!_postingListMap.isEmpty() && _postingListMap.size() % FLUSH_THRESHOLD != 0) {
Review Comment:
(minor) This is actually redundant. Actually it might be more clear to only check whether map is empty
```suggestion
if (!_postingListMap.isEmpty()) {
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18834:
URL: https://github.com/apache/pinot/pull/18834#discussion_r3456191546
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/OffHeapH3IndexCreator.java:
##########
@@ -73,7 +73,9 @@ public OffHeapH3IndexCreator(File indexDir, String columnName, String tableNameW
public void add(@Nullable Geometry geometry)
throws IOException {
super.add(geometry);
- if (_postingListMap.size() % FLUSH_THRESHOLD == 0) {
+ // Skip flushing an empty posting list map. This could happen when add() didn't add any entry due to invalid
+ // Geometry and _continueOnError is set
+ if (!_postingListMap.isEmpty() && _postingListMap.size() % FLUSH_THRESHOLD == 0) {
Review Comment:
Should we simply do:
```suggestion
if (_postingListMap.size() == FLUSH_THRESHOLD) {
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18834:
URL: https://github.com/apache/pinot/pull/18834#issuecomment-4773474389
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18834?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.96%. Comparing base ([`d469cb1`](https://app.codecov.io/gh/apache/pinot/commit/d469cb17507f4e0c35f8bf639272109cb94d9f98?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e481405`](https://app.codecov.io/gh/apache/pinot/commit/e4814058cbd3072a2606a18e45fd0e697e708e3a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 30 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18834?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tor/impl/inv/geospatial/OffHeapH3IndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/18834?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2Finv%2Fgeospatial%2FOffHeapH3IndexCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvZ2Vvc3BhdGlhbC9PZmZIZWFwSDNJbmRleENyZWF0b3IuamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18834?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (d469cb1) and HEAD (e481405). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (d469cb1) | HEAD (e481405) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18834 +/- ##
============================================
- Coverage 64.78% 56.96% -7.82%
+ Complexity 1309 1 -1308
============================================
Files 3381 2603 -778
Lines 209967 152424 -57543
Branches 32891 24743 -8148
============================================
- Hits 136020 86832 -49188
+ Misses 62979 58207 -4772
+ Partials 10968 7385 -3583
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <0.00%> (-7.82%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <0.00%> (-7.82%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <0.00%> (-7.82%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.96% <0.00%> (+<0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18834/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/18834?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #18834:
URL: https://github.com/apache/pinot/pull/18834
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18834:
URL: https://github.com/apache/pinot/pull/18834#discussion_r3455990853
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/OffHeapH3IndexCreator.java:
##########
@@ -83,6 +83,11 @@ public void add(@Nullable Geometry geometry)
*/
private void flush()
throws IOException {
+ // Skip flushing an empty posting list map. This could happen when add() didn't add any entry due to invalid
+ // Geometry and _continueOnError is set
+ if (_postingListMap.isEmpty()) {
Review Comment:
done. also changed the other caller (line 118)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18834:
URL: https://github.com/apache/pinot/pull/18834#discussion_r3456203373
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/OffHeapH3IndexCreator.java:
##########
@@ -113,7 +115,7 @@ public void seal()
}
// Flush the last chunk
- if (_postingListMap.size() % FLUSH_THRESHOLD != 0) {
+ if (!_postingListMap.isEmpty() && _postingListMap.size() % FLUSH_THRESHOLD != 0) {
Review Comment:
oh yeah this one is a different check. thanks for pointing out
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Bug fix] Skip flushing empty chunk in OffHeapH3IndexCreator [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18834:
URL: https://github.com/apache/pinot/pull/18834#discussion_r3456205115
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/OffHeapH3IndexCreator.java:
##########
@@ -73,7 +73,9 @@ public OffHeapH3IndexCreator(File indexDir, String columnName, String tableNameW
public void add(@Nullable Geometry geometry)
throws IOException {
super.add(geometry);
- if (_postingListMap.size() % FLUSH_THRESHOLD == 0) {
+ // Skip flushing an empty posting list map. This could happen when add() didn't add any entry due to invalid
+ // Geometry and _continueOnError is set
+ if (!_postingListMap.isEmpty() && _postingListMap.size() % FLUSH_THRESHOLD == 0) {
Review Comment:
makes sense, `add()` always only adds one entry at a time
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org