deepthi912 commented on code in PR #16710:
URL: https://github.com/apache/pinot/pull/16710#discussion_r2350389535
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -905,18 +905,25 @@ protected void doTakeSnapshot() {
segmentsWithoutSnapshot.add(immutableSegment);
continue;
}
+ ThreadSafeMutableRoaringBitmap queryableDocIdsBitMap = null;
+ ThreadSafeMutableRoaringBitmap validDocIdsBitMap =
+ immutableSegment.getValidDocIds() == null ? new
ThreadSafeMutableRoaringBitmap()
Review Comment:
Sometimes when loading a snapshot file: `loadDocIdsFromSnapshot` In many
cases, we are handling it creating it as new ThreadSafeMutableRoaringBitmap()
for null values, for the safer side I added this check to handle null values.
Also let's say we have segment compaction tasks running on a segment
removing all the validDocs, but until next iteration this segment will have 0
validDocs marking it EmptySegment. In such cases we might end up having null
values I think where empty segments remain in Pinot until they are deleted by
the next cycle of task generation.
Wondering if this addition would cause any issues in query performance or
something? I am trying to understand : `Empty bitmap means the entire segment
should be skipped` -- Isn't that what we want?
--
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]
