xiangfu0 commented on code in PR #20042:
URL: https://github.com/apache/pinot/pull/20042#discussion_r4376994663
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/InvertedIndexFilterOperator.java:
##########
@@ -169,11 +168,29 @@ public BitmapCollection getBitmaps() {
int[] dictIds = _exclusive ? _predicateEvaluator.getNonMatchingDictIds() :
_predicateEvaluator.getMatchingDictIds();
ImmutableRoaringBitmap[] bitmaps = new
ImmutableRoaringBitmap[dictIds.length];
for (int i = 0; i < dictIds.length; i++) {
- bitmaps[i] = _invertedIndexReader.getDocIds(dictIds[i]);
+ bitmaps[i] = getDocIds(dictIds[i]);
}
return new BitmapCollection(_numDocs, _exclusive,
bitmaps).excludingNulls(getNullBitmap());
}
+ /// Returns the documents for the given dictionary id, limited to `[0,
numDocs)`.
+ ///
+ /// On a consuming segment, rows keep being indexed after the query is
planned, so the bitmap read at execution time
+ /// can hold documents at or above `numDocs`, which is read at planning
time. These documents must be dropped: the
+ /// flip for exclusive predicates only covers `[0, numDocs)`, so they would
stay in the result, and the AND and count
+ /// paths don't check the document range again.
Review Comment:
Nice fix, and good call on the trim-based approach over bounding the
iterators. One consistency note: the sibling PR #20043 adds
`FilterOperatorUtils.limitToNumDocs(bitmap, numDocs)` with essentially this
same logic. Worth reconciling the two so there's a single implementation — or,
if you keep this private copy deliberately (it reuses the
`MutableRoaringBitmap` in place and avoids the extra `toMutableRoaringBitmap()`
clone that `limitToNumDocs` always does), a one-line comment pointing to
`limitToNumDocs` and explaining why this one diverges would save the next
reader the diff.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/InvertedIndexFilterOperator.java:
##########
@@ -169,11 +168,29 @@ public BitmapCollection getBitmaps() {
int[] dictIds = _exclusive ? _predicateEvaluator.getNonMatchingDictIds() :
_predicateEvaluator.getMatchingDictIds();
ImmutableRoaringBitmap[] bitmaps = new
ImmutableRoaringBitmap[dictIds.length];
for (int i = 0; i < dictIds.length; i++) {
- bitmaps[i] = _invertedIndexReader.getDocIds(dictIds[i]);
+ bitmaps[i] = getDocIds(dictIds[i]);
}
return new BitmapCollection(_numDocs, _exclusive,
bitmaps).excludingNulls(getNullBitmap());
}
+ /// Returns the documents for the given dictionary id, limited to `[0,
numDocs)`.
+ ///
+ /// On a consuming segment, rows keep being indexed after the query is
planned, so the bitmap read at execution time
+ /// can hold documents at or above `numDocs`, which is read at planning
time. These documents must be dropped: the
+ /// flip for exclusive predicates only covers `[0, numDocs)`, so they would
stay in the result, and the AND and count
+ /// paths don't check the document range again.
+ private ImmutableRoaringBitmap getDocIds(int dictId) {
+ ImmutableRoaringBitmap docIds = _invertedIndexReader.getDocIds(dictId);
+ if (docIds.isEmpty() || docIds.last() < _numDocs) {
+ return docIds;
+ }
+ // Only a mutable inverted index can hold such documents, and it returns a
copy of its bitmap
+ MutableRoaringBitmap mutableDocIds =
+ docIds instanceof MutableRoaringBitmap ? (MutableRoaringBitmap) docIds
: docIds.toMutableRoaringBitmap();
+ mutableDocIds.remove(_numDocs, 0x100000000L);
Review Comment:
The in-place `remove` on the `MutableRoaringBitmap` branch (no copy) is safe
only because the mutable inverted index hands back a `.clone()` on every
`getDocIds` call — verified in
`RealtimeInvertedIndex`/`ThreadSafeMutableRoaringBitmap`. That's a load-bearing
assumption; a short comment here noting "the mutable index already returns a
fresh clone, so we can trim in place" would protect it against future index
changes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
