You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "lnbest0707-uber (via GitHub)" <gi...@apache.org> on 2025/02/24 19:44:43 UTC
[PR] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
lnbest0707-uber opened a new pull request, #15120:
URL: https://github.com/apache/pinot/pull/15120
`enhancement` `ingestion`
Inspired by https://github.com/apache/pinot/pull/14479
Apart from the MVForwardIndex check added in that PR, this PR adds similar check for SVForwardIndex and Dictionary. The PR only adds the interface and UTs.
The actual check policy is not within this PR and could be an open question to discuss. The end criteria needs to consider
- ForwardIndex would end in a 4GB limit once converted into immutable segment.
- Dictionary has its cardinality limit
While it is tricky to make it correct as:
- 4GB limit is a **compressed** size, during ingestion into mutable segment, it cannot predict the correct compression ratio
- With `optimizeDictionary` enabled, Dictionary encoding could also end in immutable forward index. It would be even harder to guess
- If Dictionary would be converted to ForwardIndex
- If converted, what would be the final compressed size
Some proposed policies:
- Make the uncompressed size limit configurable, relying on that value to set the mutable segment's threshold.
- Use the last segment's compression ratio as reference to predict current segment's.
- For dictionary, use the same policy (but maybe a larger number by config) as the forward index.
--
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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on code in PR #15120:
URL: https://github.com/apache/pinot/pull/15120#discussion_r2059092527
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/MutableDictionary.java:
##########
@@ -48,4 +48,11 @@ default int insertionIndexOf(String stringValue) {
// This method should not be called for unsorted dictionary.
throw new UnsupportedOperationException();
}
+
+ /**
+ * This method returns a boolean denoting whether the mutable index can consume any more rows or not.
Review Comment:
```suggestion
* This method returns a boolean denoting whether the mutable dictionary can consume any more rows or not.
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/mutable/VarByteSVMutableForwardIndexTest.java:
##########
@@ -82,6 +83,7 @@ public void testBytes()
try (VarByteSVMutableForwardIndex readerWriter = new VarByteSVMutableForwardIndex(DataType.STRING, _memoryManager,
"StringColumn", initialCapacity, estimatedAvgStringLength)) {
int rows = 1000;
+ int totalLength = 0;
Review Comment:
this is being used?
--
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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15120:
URL: https://github.com/apache/pinot/pull/15120#issuecomment-2679567719
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15120?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 `20.00000%` with `36 lines` in your changes missing coverage. Please review.
> Project coverage is 55.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 [(`e4f83f7`)](https://app.codecov.io/gh/apache/pinot/commit/e4f83f75dc97c1e51cec737fbc2b059a89773e40?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1753 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15120?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://app.codecov.io/gh/apache/pinot/pull/15120?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Findexsegment%2Fmutable%2FMutableSegmentImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | 20.45% | [33 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/segment/spi/index/mutable/MutableDictionary.java](https://app.codecov.io/gh/apache/pinot/pull/15120?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fmutable%2FMutableDictionary.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L211dGFibGUvTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15120?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 (59551e4) and HEAD (e4f83f7). Click for more details.
>
> <details><summary>HEAD has 48 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (e4f83f7) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|2|
>|java-21|7|2|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|1|
>|unittests|5|2|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15120 +/- ##
============================================
- Coverage 61.75% 55.95% -5.81%
- Complexity 207 711 +504
============================================
Files 2436 2149 -287
Lines 133233 115671 -17562
Branches 20636 18583 -2053
============================================
- Hits 82274 64720 -17554
- Misses 44911 45645 +734
+ Partials 6048 5306 -742
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15120/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/15120/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/15120/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/15120/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/15120/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/15120/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/15120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.95% <20.00%> (-5.68%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.93% <20.00%> (-5.81%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.86% <20.00%> (+28.13%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.95% <20.00%> (-5.81%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.95% <20.00%> (-5.80%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.95% <20.00%> (+9.06%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15120/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/15120?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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on PR #15120:
URL: https://github.com/apache/pinot/pull/15120#issuecomment-2837116318
Can you comment or add documentation on this PR what is the current behavior if the size threshold is reached?
--
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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "lnbest0707-uber (via GitHub)" <gi...@apache.org>.
lnbest0707-uber commented on code in PR #15120:
URL: https://github.com/apache/pinot/pull/15120#discussion_r2065174424
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -189,6 +178,11 @@ public class MutableSegmentImpl implements MutableSegment {
// the valid doc ids won't be updated.
private final ThreadSafeMutableRoaringBitmap _validDocIds;
private final ThreadSafeMutableRoaringBitmap _queryableDocIds;
+ private boolean _indexCapacityThresholdBreached;
Review Comment:
Yes, it is one for all check. The PR is only extracting the variable to an instance field. The name was unchanged. But I am open to rename.
--
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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #15120:
URL: https://github.com/apache/pinot/pull/15120
--
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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "lnbest0707-uber (via GitHub)" <gi...@apache.org>.
lnbest0707-uber commented on PR #15120:
URL: https://github.com/apache/pinot/pull/15120#issuecomment-2834265058
@noob-se7en resolved you comments, could you pls review again? Thanks
--
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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #15120:
URL: https://github.com/apache/pinot/pull/15120#issuecomment-2737719261
@noob-se7en @KKcorps Can you help take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] Add segment end criteria check for SVForwardIndex and Dictionary [pinot]
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #15120:
URL: https://github.com/apache/pinot/pull/15120#discussion_r2065095621
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/MutableDictionary.java:
##########
@@ -48,4 +48,11 @@ default int insertionIndexOf(String stringValue) {
// This method should not be called for unsorted dictionary.
throw new UnsupportedOperationException();
}
+
+ /**
+ * This method returns a boolean denoting whether the mutable dictionary can consume any more rows or not.
Review Comment:
Make it clear by saying "Return true if the mutable dictionary can consume an additional row"
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -189,6 +178,11 @@ public class MutableSegmentImpl implements MutableSegment {
// the valid doc ids won't be updated.
private final ThreadSafeMutableRoaringBitmap _validDocIds;
private final ThreadSafeMutableRoaringBitmap _queryableDocIds;
+ private boolean _indexCapacityThresholdBreached;
Review Comment:
Is this threshold check for all indexes/dictionaries in one segment?
Suggest to rename to _indexThresholdReached and also add javadoc.
--
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