Copilot commented on code in PR #16710:
URL: https://github.com/apache/pinot/pull/16710#discussion_r2309200855
##########
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()
+ : immutableSegment.getValidDocIds();
immutableSegment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
- immutableSegment.getValidDocIds());
+ validDocIdsBitMap);
if (_deleteRecordColumn != null) {
+ queryableDocIdsBitMap =
+ immutableSegment.getQueryableDocIds() == null ? new
ThreadSafeMutableRoaringBitmap()
+ : immutableSegment.getQueryableDocIds();
immutableSegment.persistDocIdsSnapshot(V1Constants.QUERYABLE_DOC_IDS_SNAPSHOT_FILE_NAME,
- immutableSegment.getQueryableDocIds());
+ queryableDocIdsBitMap);
}
_updatedSegmentsSinceLastSnapshot.remove(segment);
numImmutableSegments++;
- numPrimaryKeysInSnapshot +=
immutableSegment.getValidDocIds().getMutableRoaringBitmap().getCardinality();
+ numPrimaryKeysInSnapshot +=
validDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
if (_deleteRecordColumn != null) {
numQueryableDocIdsInSnapshot +=
-
immutableSegment.getQueryableDocIds().getMutableRoaringBitmap().getCardinality();
+ queryableDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
}
Review Comment:
[nitpick] The variable `queryableDocIdsBitMap` is initialized to null but
only used conditionally. Consider initializing it only when needed inside the
if block to make the code clearer and avoid potential confusion.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -940,16 +947,21 @@ protected void doTakeSnapshot() {
continue;
}
try {
-
segment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap segmentQueryableDocIdsBitMap = null;
+ ThreadSafeMutableRoaringBitmap segmentValidDocIdsBitMap =
+ segment.getValidDocIds() == null ? new
ThreadSafeMutableRoaringBitmap() : segment.getValidDocIds();
+
segment.persistDocIdsSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME,
segmentValidDocIdsBitMap);
if (_deleteRecordColumn != null) {
+ segmentQueryableDocIdsBitMap = segment.getQueryableDocIds() ==
null ? new ThreadSafeMutableRoaringBitmap()
+ : segment.getQueryableDocIds();
segment.persistDocIdsSnapshot(V1Constants.QUERYABLE_DOC_IDS_SNAPSHOT_FILE_NAME,
- segment.getQueryableDocIds());
+ segmentQueryableDocIdsBitMap);
}
_updatedSegmentsSinceLastSnapshot.remove(segment);
numImmutableSegments++;
- numPrimaryKeysInSnapshot +=
segment.getValidDocIds().getMutableRoaringBitmap().getCardinality();
+ numPrimaryKeysInSnapshot +=
segmentValidDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
if (_deleteRecordColumn != null) {
- numQueryableDocIdsInSnapshot +=
segment.getQueryableDocIds().getMutableRoaringBitmap().getCardinality();
+ numQueryableDocIdsInSnapshot +=
segmentQueryableDocIdsBitMap.getMutableRoaringBitmap().getCardinality();
}
Review Comment:
[nitpick] The variable `segmentQueryableDocIdsBitMap` is initialized to null
but only used conditionally. Consider initializing it only when needed inside
the if block to make the code clearer and avoid potential confusion.
--
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]
