You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "itschrispeck (via GitHub)" <gi...@apache.org> on 2025/02/27 01:39:42 UTC
[PR] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
itschrispeck opened a new pull request, #15139:
URL: https://github.com/apache/pinot/pull/15139
Closes `ChunkReaderContext` in `ProjectionOperator` and scan based iterators to reduce direct buffer usage. Lightly tested in a local staging cluster - will add some more query shapes soon.
For the simple query: `select lower(message_logtype) from loggingstaging_wallet_api_gateway where lower(message_logtype) = 'asdf'` we see up to 80GB direct buffer memory usage, which can take 10s of minutes to be collected. e.g., per host metrics:
<img width="768" alt="image" src="https://github.com/user-attachments/assets/812751fb-af33-4ef4-87cf-4b8b052af5a3" />
With the patch, the same query creates no discernable spike in direct buffer usage (query executed at 17:43):
<img width="768" alt="image" src="https://github.com/user-attachments/assets/a0863f15-54b3-415a-8bbc-31cb4a4fd215" />
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2008612350
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/ChunkReaderContext.java:
##########
@@ -52,11 +51,8 @@ public ChunkReaderContext(int maxChunkSize) {
}
@Override
- public void close()
- throws IOException {
- if (CleanerUtil.UNMAP_SUPPORTED) {
- CleanerUtil.getCleaner().freeBuffer(_chunkBuffer);
- }
+ public void close() {
+ CleanerUtil.cleanQuietly(_chunkBuffer);
Review Comment:
I still feel it is safer to add a boolean flag `_closed` to ensure clean is only invoked once. It is OS dependent behavior
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #15139:
URL: https://github.com/apache/pinot/pull/15139
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r1984381792
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java:
##########
@@ -445,6 +444,7 @@ private void decompressViaDirectBuffer(ByteBuffer compressed, byte[] target)
@Override
public void close() {
CleanerUtil.cleanQuietly(_decompressedBuffer);
+ _decompressedBuffer = null;
Review Comment:
Do we need to set it to `null` after cleaning it?
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader : _columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
Same for other places
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader : _columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
Should we throw the exception out? Swallow exception silently is not good practice unless it is expected
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataBlockCache.java:
##########
@@ -406,4 +406,11 @@ private void putValues(FieldSpec.DataType dataType, String column, Object values
public void addDataSource(String fullColumnKeyName, DataSource keyDataSource) {
_dataFetcher.addDataSource(fullColumnKeyName, keyDataSource);
}
+
+ /**
+ * Close the data block cache and release all resources.
+ */
+ public void close() {
Review Comment:
Make these classes implement `AutoCloseable` so that they can be used within try-with-resource
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2008311704
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/ChunkReaderContext.java:
##########
@@ -52,11 +51,8 @@ public ChunkReaderContext(int maxChunkSize) {
}
@Override
- public void close()
- throws IOException {
- if (CleanerUtil.UNMAP_SUPPORTED) {
- CleanerUtil.getCleaner().freeBuffer(_chunkBuffer);
- }
+ public void close() {
+ CleanerUtil.cleanQuietly(_chunkBuffer);
Review Comment:
Can you try releasing the same buffer twice? My concern is this could cause segmentation fault
##########
pinot-core/src/main/java/org/apache/pinot/core/common/BlockDocIdIterator.java:
##########
@@ -47,4 +47,11 @@ public interface BlockDocIdIterator {
* @see {https://github.com/RoaringBitmap/RoaringBitmap/pull/243#issuecomment-381278304}
*/
int OPTIMAL_ITERATOR_BATCH_SIZE = 256;
+
+ /**
+ * Close resources if applicable.
+ */
+ default void close() {
Review Comment:
(minor)
```suggestion
@Override
default void close() {
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ForwardIndexReaderContext.java:
##########
@@ -28,4 +28,7 @@
* inside the context in order to accelerate the following reads.
*/
public interface ForwardIndexReaderContext extends Closeable {
Review Comment:
This can also be `AutoCloseable`
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15139:
URL: https://github.com/apache/pinot/pull/15139#issuecomment-2686690025
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15139?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `1.69492%` with `58 lines` in your changes missing coverage. Please review.
> Project coverage is 33.95%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`cf14671`)](https://app.codecov.io/gh/apache/pinot/commit/cf14671ad53adf1d9eb77daffcca9e0e913d71c1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1768 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15139?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...or/dociditerators/ExpressionScanDocIdIterator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Fdociditerators%2FExpressionScanDocIdIterator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9FeHByZXNzaW9uU2NhbkRvY0lkSXRlcmF0b3IuamF2YQ==) | 0.00% | [17 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/operator/dociditerators/SVScanDocIdIterator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Fdociditerators%2FSVScanDocIdIterator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9TVlNjYW5Eb2NJZEl0ZXJhdG9yLmphdmE=) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...java/org/apache/pinot/core/common/DataFetcher.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fcommon%2FDataFetcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vRGF0YUZldGNoZXIuamF2YQ==) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...in/java/org/apache/pinot/core/common/Operator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fcommon%2FOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vT3BlcmF0b3IuamF2YQ==) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/operator/dociditerators/MVScanDocIdIterator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Fdociditerators%2FMVScanDocIdIterator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9NVlNjYW5Eb2NJZEl0ZXJhdG9yLmphdmE=) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...core/operator/dociditerators/AndDocIdIterator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Fdociditerators%2FAndDocIdIterator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9BbmREb2NJZEl0ZXJhdG9yLmphdmE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/pinot/core/operator/ProjectionOperator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2FProjectionOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9Qcm9qZWN0aW9uT3BlcmF0b3IuamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/core/common/DataBlockCache.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fcommon%2FDataBlockCache.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vRGF0YUJsb2NrQ2FjaGUuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../core/operator/dociditerators/OrDocIdIterator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Fdociditerators%2FOrDocIdIterator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9PckRvY0lkSXRlcmF0b3IuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...not/core/operator/combine/BaseCombineOperator.java](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Fcombine%2FBaseCombineOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [1 more](https://app.codecov.io/gh/apache/pinot/pull/15139?src=pr&el=tree-more&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 (59551e4) and HEAD (cf14671). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (cf14671) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|1|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|1|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15139 +/- ##
=============================================
- Coverage 61.75% 33.95% -27.80%
- Complexity 207 678 +471
=============================================
Files 2436 2751 +315
Lines 133233 154592 +21359
Branches 20636 23822 +3186
=============================================
- Hits 82274 52491 -29783
- Misses 44911 97858 +52947
+ Partials 6048 4243 -1805
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15139/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/15139/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/15139/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/15139/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/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15139/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/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.95% <1.69%> (-27.68%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.95% <1.69%> (-27.80%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.95% <1.69%> (-27.80%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.95% <1.69%> (-27.80%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15139/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.95% <1.69%> (+6.22%)` | :arrow_up: |
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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/15139?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).
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r1972677661
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/SVScanDocIdIterator.java:
##########
@@ -138,6 +138,7 @@ public MutableRoaringBitmap applyAnd(BatchIterator docIdIterator, OptionalInt fi
}
_numEntriesScanned += limit;
}
+ close();
Review Comment:
This makes an assumption that applyAnd will not be called multiple times, or after EOF has been returned by `next`/`advance` - but I don't think this contract is explicitly defined (yet? I can add a comment in the interface)
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2002314552
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader : _columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
Updated to `CleanerUtil.closeQuietly()` and ForwardIndexReaderContext to no longer throw an exception on close as none of the implementations do.
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2008120713
##########
pinot-core/src/main/java/org/apache/pinot/core/common/BlockDocIdIterator.java:
##########
@@ -47,4 +47,11 @@ public interface BlockDocIdIterator {
* @see {https://github.com/RoaringBitmap/RoaringBitmap/pull/243#issuecomment-381278304}
*/
int OPTIMAL_ITERATOR_BATCH_SIZE = 256;
+
+ /**
+ * Close resources if applicable.
+ */
+ default void close() {
Review Comment:
Suggest making it extending `AutoCloseable`
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -587,11 +585,20 @@ public void readNumValuesMV(int[] docIds, int length, int[] numValuesBuffer) {
}
@Override
- public void close()
- throws IOException {
+ public void close() {
if (_readerContext != null) {
_readerContext.close();
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the ForwardIndexReaderContext off-heap buffers).
+ */
+ @Override
+ public void close() {
+ for (ColumnValueReader columnValueReader : _columnValueReaderMap.values()) {
+ columnValueReader.close();
Review Comment:
Fix the indentation
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/OrDocIdIterator.java:
##########
@@ -117,6 +117,11 @@ private void removeExhaustedIterators() {
int i = 0;
while (i < _numNotExhaustedIterators) {
if (_nextDocIds[i] == Constants.EOF) {
+ // Close the exhausted iterator
+ if (_docIdIterators[i] instanceof ScanBasedDocIdIterator) {
Review Comment:
No need for the check
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java:
##########
@@ -71,4 +72,12 @@ public int advance(int targetDocId) {
_nextDocId = targetDocId;
return next();
}
+
+ private void closeIterators() {
+ for (BlockDocIdIterator it : _docIdIterators) {
+ if (it instanceof ScanBasedDocIdIterator) {
+ ((ScanBasedDocIdIterator) it).close();
+ }
Review Comment:
No need to do instanceof check
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/MVScanDocIdIterator.java:
##########
@@ -251,4 +251,11 @@ public boolean doesValueMatch(int docId) {
return _predicateEvaluator.applyMV(_buffer, length);
}
}
+
+ @Override
+ public void close() {
+ if (_readerContext != null) {
+ _readerContext.close();
Review Comment:
Does it work if it is closed multiple times?
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2008339976
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/ChunkReaderContext.java:
##########
@@ -52,11 +51,8 @@ public ChunkReaderContext(int maxChunkSize) {
}
@Override
- public void close()
- throws IOException {
- if (CleanerUtil.UNMAP_SUPPORTED) {
- CleanerUtil.getCleaner().freeBuffer(_chunkBuffer);
- }
+ public void close() {
+ CleanerUtil.cleanQuietly(_chunkBuffer);
Review Comment:
I double checked by calling `context.close()` multiple times in the v4 reader/writer UT. We have also been running this portion of the PR in a prod cluster for a couple weeks without issues.
My understanding was that invokeCleaner uses the cleaner API, [which only runs the cleaner if it has not been run before](https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/master/src/java.base/share/classes/jdk/internal/ref/Cleaner.java#L137). Doing multiple unsafe calls to .freeMemory() for the memory address would seg fault
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r1972677661
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/SVScanDocIdIterator.java:
##########
@@ -138,6 +138,7 @@ public MutableRoaringBitmap applyAnd(BatchIterator docIdIterator, OptionalInt fi
}
_numEntriesScanned += limit;
}
+ close();
Review Comment:
This makes an assumption that applyAnd will not be called multiple times, or after EOF has been returned by `next`/`advance` - but I don't think this contract is explicitly defined (yet?)
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2001839004
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/ProjectPlanNode.java:
##########
@@ -80,9 +80,10 @@ public BaseProjectOperator<?> run() {
DocIdSetOperator docIdSetOperator =
_maxDocsPerCall > 0 ? new DocIdSetPlanNode(_segmentContext, _queryContext, _maxDocsPerCall,
_filterOperator).run() : null;
- ProjectionOperator projectionOperator =
- ProjectionOperatorUtils.getProjectionOperator(dataSourceMap, docIdSetOperator);
- return hasNonIdentifierExpression ? new TransformOperator(_queryContext, projectionOperator, _expressions)
- : projectionOperator;
+ try (ProjectionOperator projectionOperator =
Review Comment:
This doesn't seem correct. Can we close the operator at planning phase? Shouldn't it be closed after query execution?
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2001830694
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader : _columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
Using `CleanerUtil.closeQuietly()` might be a good solution. At least we know that is intended
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2001833170
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ScanBasedDocIdIterator.java:
##########
@@ -57,9 +57,15 @@ default MutableRoaringBitmap applyAnd(ImmutableRoaringBitmap docIds) {
/**
* Returns the estimated (effective) cardinality of the underlying data source
*/
-
default float getEstimatedCardinality(boolean isAndDocIdSet) {
//default N/A behavior so that it always get picked in the end
return isAndDocIdSet ? Float.NEGATIVE_INFINITY : Float.POSITIVE_INFINITY;
}
+
+ /**
+ * Close resources if applicable.
+ */
+ default void close() {
Review Comment:
Suggest adding this to `BlockDocIdIterator`
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2008163230
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/MVScanDocIdIterator.java:
##########
@@ -251,4 +251,11 @@ public boolean doesValueMatch(int docId) {
return _predicateEvaluator.applyMV(_buffer, length);
}
}
+
+ @Override
+ public void close() {
+ if (_readerContext != null) {
+ _readerContext.close();
Review Comment:
yes, ForwardIndexReaderContext.close() adheres to the `Closable` interface contract that close must be idempotent
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2001833823
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java:
##########
@@ -376,7 +375,7 @@ public void close() {
private static final class CompressedReaderContext extends ReaderContext {
- private final ByteBuffer _decompressedBuffer;
+ private ByteBuffer _decompressedBuffer;
Review Comment:
Revert
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r2002313422
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/ProjectPlanNode.java:
##########
@@ -80,9 +80,10 @@ public BaseProjectOperator<?> run() {
DocIdSetOperator docIdSetOperator =
_maxDocsPerCall > 0 ? new DocIdSetPlanNode(_segmentContext, _queryContext, _maxDocsPerCall,
_filterOperator).run() : null;
- ProjectionOperator projectionOperator =
- ProjectionOperatorUtils.getProjectionOperator(dataSourceMap, docIdSetOperator);
- return hasNonIdentifierExpression ? new TransformOperator(_queryContext, projectionOperator, _expressions)
- : projectionOperator;
+ try (ProjectionOperator projectionOperator =
Review Comment:
Good catch, it isn't. The change looks more involved, I re-scoped this PR to address the scan cases for now as it's easier to cherry-pick for us internally. I can add this as a follow up?
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r1987864905
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader : _columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
I think it becomes a backwards incompat change if we rethrow it - changing it to close the context on a jvm that does not support the same cleaner methods would be breaking
Another approach might be to use `CleanerUtil.closeQuietly(..)` within each context's close() impl, and then just use AutoClosable instead of Closable for the ColumnValueReader
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r1987860220
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java:
##########
@@ -445,6 +444,7 @@ private void decompressViaDirectBuffer(ByteBuffer compressed, byte[] target)
@Override
public void close() {
CleanerUtil.cleanQuietly(_decompressedBuffer);
+ _decompressedBuffer = null;
Review Comment:
Nope, had seen some blogs mention it is required so a/b tested it, and it is not.
--
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] Close `ChunkReaderContext` to improve projection/scan based filter memory for raw columns [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r1987864905
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader : _columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
I think it becomes a backwards incompat change if we rethrow it - changing it to close the context on a jvm that does not support the same cleaner methods would be breaking
Another approach might be to use `CleanerUtil.closeQuietly(..)` within each context's close() impl, and then just use AutoClosable instead of Closable for the ColumnValueReader (v4 uses closeQuietly, some others would throw an exception)
--
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