Tarun-kishore commented on code in PR #20042:
URL: https://github.com/apache/pinot/pull/20042#discussion_r4377364320
##########
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:
Good point. I added a comment spelling out that the in-place trim relies on
RealtimeInvertedIndex.getDocIds() returning a fresh clone on every call
(ThreadSafeMutableRoaringBitmap.getMutableRoaringBitmap()). The existing
in-place flip in getNextBlockWithoutNullHandling() already relies on the same
thing, so the comment points to it as well.
##########
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:
Thanks! I kept the private helper on purpose. It trims the mutable index's
fresh clone in place, while FilterOperatorUtils.limitToNumDocs() in #20043 has
to copy, since it can't know whether its input may be modified. I added a note
pointing to it. I kept the two PRs independent so either can land first. Once
both are in, I'm happy to fold this into an in-place variant of limitToNumDocs
in a follow-up if you prefer a single implementation.
--
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]
