You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "deepthi912 (via GitHub)" <gi...@apache.org> on 2026/03/20 01:30:20 UTC
[PR] Upserts Pruning Optimization - Do not clone MutableRoaringBitmap for each segment [pinot]
deepthi912 opened a new pull request, #17920:
URL: https://github.com/apache/pinot/pull/17920
In reference to the comment: https://github.com/apache/pinot/pull/17898#discussion_r2949980331
Calling clone() on MutableRoaringBitmap in getMutableRoaringBitmap method can get costly depending on number of segments and the size of valid bitmaps of each segment, so it is better to store if a bitmap is empty or not in the ThreadSafeMutableRoaringBitmap class and call areQueryableDocIdsEmpty for any empty segments that have 0 valid and 0 queryable docs
--
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] Upserts Pruning Optimization - Do not clone MutableRoaringBitmap for each segment [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17920:
URL: https://github.com/apache/pinot/pull/17920#discussion_r2966802177
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/SegmentPrunerService.java:
##########
Review Comment:
(minor) Also remove `@Nullable` here
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertUtils.java:
##########
@@ -57,6 +57,26 @@ public static MutableRoaringBitmap getQueryableDocIdsSnapshotFromSegment(IndexSe
: (useEmptyForNull ? new MutableRoaringBitmap() : null);
}
+ /**
+ * Returns whether the segment has no queryable documents for purposes such as empty-segment pruning.
+ * When {@code skipUpsert} is true, always returns {@code false} (the segment is not treated as empty).
+ * Otherwise uses the same bitmap precedence as {@link #getQueryableDocIdsSnapshotFromSegment(IndexSegment)}:
+ * {@link IndexSegment#getQueryableDocIds()} if non-null, else {@link IndexSegment#getValidDocIds()}; if both are
+ * null, returns {@code false}, the table will be non-upsert. Emptiness is checked via
+ * {@link ThreadSafeMutableRoaringBitmap#isEmpty()} to avoid
+ * cloning the bitmap.
+ */
+ public static boolean areQueryableDocIdsEmpty(IndexSegment segment, boolean skipUpsert) {
Review Comment:
(optional) `hasNoQueryableDocs`
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/SegmentPrunerService.java:
##########
Review Comment:
Process query option here before getting into the loop
--
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] Upserts Pruning Optimization - Do not clone MutableRoaringBitmap for each segment [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17920:
URL: https://github.com/apache/pinot/pull/17920#discussion_r2976374726
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/SegmentPrunerService.java:
##########
@@ -142,27 +141,24 @@ public List<IndexSegment> prune(List<IndexSegment> segments, QueryContext query,
* emptiness is determined from them; otherwise {@link IndexSegment#getValidDocIds()} is used.
* @return the new list with filtered elements. This is the list that have to be used.
*/
- private static List<IndexSegment> removeEmptySegments(List<IndexSegment> segments, @Nullable QueryContext query) {
+ private static List<IndexSegment> removeEmptySegments(List<IndexSegment> segments, QueryContext query) {
int selected = 0;
+ boolean skipUpsert = QueryOptionsUtils.isSkipUpsert(query.getQueryOptions());
for (IndexSegment segment : segments) {
- if (!isEmptySegment(segment, query)) {
+ if (!isEmptySegment(segment, skipUpsert)) {
segments.set(selected++, segment);
}
}
return segments.subList(0, selected);
}
- private static boolean isEmptySegment(IndexSegment segment, @Nullable QueryContext query) {
+ private static boolean isEmptySegment(IndexSegment segment, boolean skipUpsert) {
if (segment.getSegmentMetadata().getTotalDocs() == 0) {
return true;
}
// For upsert tables, treat segments with 0 docs to query as empty only when the query does not skip upsert.
// When skipUpsert=true, the query returns all docs (including replaced), so the segment contributes rows.
// Use query options map directly: _skipUpsert is only set on the server; the broker has options in the map.
Review Comment:
(minor) Update this comment
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/SegmentPrunerService.java:
##########
@@ -142,27 +141,24 @@ public List<IndexSegment> prune(List<IndexSegment> segments, QueryContext query,
* emptiness is determined from them; otherwise {@link IndexSegment#getValidDocIds()} is used.
* @return the new list with filtered elements. This is the list that have to be used.
*/
- private static List<IndexSegment> removeEmptySegments(List<IndexSegment> segments, @Nullable QueryContext query) {
+ private static List<IndexSegment> removeEmptySegments(List<IndexSegment> segments, QueryContext query) {
int selected = 0;
+ boolean skipUpsert = QueryOptionsUtils.isSkipUpsert(query.getQueryOptions());
for (IndexSegment segment : segments) {
- if (!isEmptySegment(segment, query)) {
+ if (!isEmptySegment(segment, skipUpsert)) {
segments.set(selected++, segment);
}
}
return segments.subList(0, selected);
}
- private static boolean isEmptySegment(IndexSegment segment, @Nullable QueryContext query) {
+ private static boolean isEmptySegment(IndexSegment segment, boolean skipUpsert) {
if (segment.getSegmentMetadata().getTotalDocs() == 0) {
return true;
}
// For upsert tables, treat segments with 0 docs to query as empty only when the query does not skip upsert.
// When skipUpsert=true, the query returns all docs (including replaced), so the segment contributes rows.
// Use query options map directly: _skipUpsert is only set on the server; the broker has options in the map.
- if (query != null && query.getQueryOptions() != null && QueryOptionsUtils.isSkipUpsert(query.getQueryOptions())) {
- return false;
- }
- MutableRoaringBitmap queryableDocIds = UpsertUtils.getQueryableDocIdsSnapshotFromSegment(segment);
- return queryableDocIds != null && queryableDocIds.isEmpty();
+ return UpsertUtils.hasNoQueryableDocs(segment, skipUpsert);
Review Comment:
(minor) Suggest taking `skipUpsert` out
```suggestion
return !skipUpsert && UpsertUtils.hasNoQueryableDocs(segment);
```
--
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] Upserts Pruning Optimization - Do not clone MutableRoaringBitmap for each segment [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #17920:
URL: https://github.com/apache/pinot/pull/17920
--
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] Upserts Pruning Optimization - Do not clone MutableRoaringBitmap for each segment [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17920:
URL: https://github.com/apache/pinot/pull/17920#discussion_r2966812952
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/SegmentPrunerService.java:
##########
@@ -152,17 +151,13 @@ private static List<IndexSegment> removeEmptySegments(List<IndexSegment> segment
return segments.subList(0, selected);
}
- private static boolean isEmptySegment(IndexSegment segment, @Nullable QueryContext query) {
+ private static boolean isEmptySegment(IndexSegment segment, QueryContext query) {
if (segment.getSegmentMetadata().getTotalDocs() == 0) {
return true;
}
// For upsert tables, treat segments with 0 docs to query as empty only when the query does not skip upsert.
// When skipUpsert=true, the query returns all docs (including replaced), so the segment contributes rows.
// Use query options map directly: _skipUpsert is only set on the server; the broker has options in the map.
- if (query != null && query.getQueryOptions() != null && QueryOptionsUtils.isSkipUpsert(query.getQueryOptions())) {
- return false;
- }
- MutableRoaringBitmap queryableDocIds = UpsertUtils.getQueryableDocIdsSnapshotFromSegment(segment);
- return queryableDocIds != null && queryableDocIds.isEmpty();
+ return UpsertUtils.areQueryableDocIdsEmpty(segment, QueryOptionsUtils.isSkipUpsert(query.getQueryOptions()));
}
Review Comment:
`removeEmptySegments(..)` explicitly allows `query` to be null, but `isEmptySegment(..)` now dereferences `query.getQueryOptions()` unconditionally. This can cause a NullPointerException if `SegmentPrunerService.prune(..)` is ever invoked with a null `QueryContext` (the API is not annotated non-null). Please restore the null-guard (or accept `@Nullable QueryContext` and treat null as `skipUpsert=false`), and consider adding a unit test to cover the null-query case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] Upserts Pruning Optimization - Do not clone MutableRoaringBitmap for each segment [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #17920:
URL: https://github.com/apache/pinot/pull/17920#discussion_r2973095930
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/SegmentPrunerService.java:
##########
Review Comment:
Done
--
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] Upserts Pruning Optimization - Do not clone MutableRoaringBitmap for each segment [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17920:
URL: https://github.com/apache/pinot/pull/17920#issuecomment-4095228385
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17920?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`a7ad912`](https://app.codecov.io/gh/apache/pinot/commit/a7ad912b082553dac3ad6fd50e621e459a56c30c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`525bdf7`](https://app.codecov.io/gh/apache/pinot/commit/525bdf775fb2e9c324bcfb237c78aff233b91b0e?dropdown=coverage&el=desc&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 (a7ad912) and HEAD (525bdf7). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (a7ad912) | HEAD (525bdf7) |
>|------|------|------|
>|unittests2|2|0|
>|unittests|3|0|
>|java-21|2|1|
>|temurin|7|1|
>|unittests1|1|0|
>|java-11|5|0|
>|integration|4|1|
>|integration1|2|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17920 +/- ##
=============================================
- Coverage 63.16% 0.00% -63.17%
=============================================
Files 3191 3 -3188
Lines 192587 6 -192581
Branches 29537 0 -29537
=============================================
- Hits 121645 0 -121645
+ Misses 61406 6 -61400
+ Partials 9536 0 -9536
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17920/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/17920/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/17920/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17920/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/17920/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17920/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/17920/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-34.22%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17920/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-63.17%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17920/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17920/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/17920/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/17920?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