From 79b4e8169825aa532432d715f0407cc80bc5ddee Mon Sep 17 00:00:00 2001 From: Chaitanya Deepthi Date: Sat, 30 May 2026 22:57:14 -0700 Subject: [PATCH 1/2] Move partial-upsert resolveComparisonTies from base wrapper into subclasses The base addOrReplaceSegment wrapper in BasePartitionUpsertMetadataManager existed only to call resolveComparisonTies for partial-upsert before delegating to the subclass-specific doAddOrReplaceSegment. This made the abstract doAddOrReplaceSegment signature redundant with its only caller. Inline the wrapper: callers now invoke doAddOrReplaceSegment directly, and the resolveComparisonTies call moves into ConcurrentMapPartitionUpsertMetadataManager and ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes (the only two non-abstract subclasses). Removes one virtual call per segment add/replace and eliminates a layer of indirection. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../BasePartitionUpsertMetadataManager.java | 16 +++------------- ...currentMapPartitionUpsertMetadataManager.java | 3 +++ ...psertMetadataManagerForConsistentDeletes.java | 6 ++++-- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java index f0c8f850b89a..6bff8a63b694 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java @@ -494,17 +494,7 @@ public void addSegment(ImmutableSegmentImpl segment, @Nullable ThreadSafeMutable if (queryableDocIds == null && _deleteRecordColumn != null) { queryableDocIds = new ThreadSafeMutableRoaringBitmap(); } - addOrReplaceSegment(segment, validDocIds, queryableDocIds, recordInfoIterator, null, null); - } - - protected void addOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds, - @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator recordInfoIterator, - @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) { - if (_partialUpsertHandler != null) { - recordInfoIterator = resolveComparisonTies(recordInfoIterator, _hashFunction); - } - doAddOrReplaceSegment(segment, validDocIds, queryableDocIds, recordInfoIterator, oldSegment, - validDocIdsForOldSegment); + doAddOrReplaceSegment(segment, validDocIds, queryableDocIds, recordInfoIterator, null, null); } protected abstract void doAddOrReplaceSegment(ImmutableSegmentImpl segment, @@ -514,7 +504,7 @@ protected abstract void doAddOrReplaceSegment(ImmutableSegmentImpl segment, protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds, @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator recordInfoIterator) { - addOrReplaceSegment(segment, validDocIds, queryableDocIds, recordInfoIterator, null, null); + doAddOrReplaceSegment(segment, validDocIds, queryableDocIds, recordInfoIterator, null, null); } /** @@ -668,7 +658,7 @@ public void replaceSegment(ImmutableSegment segment, @Nullable ThreadSafeMutable if (queryableDocIds == null && _deleteRecordColumn != null) { queryableDocIds = new ThreadSafeMutableRoaringBitmap(); } - addOrReplaceSegment((ImmutableSegmentImpl) segment, validDocIds, queryableDocIds, recordInfoIterator, oldSegment, + doAddOrReplaceSegment((ImmutableSegmentImpl) segment, validDocIds, queryableDocIds, recordInfoIterator, oldSegment, validDocIdsForOldSegment); } if (_upsertViewManager != null) { diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java index a06bd82395d9..f580712576f5 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java @@ -71,6 +71,9 @@ protected long getNumPrimaryKeys() { protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds, @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator recordInfoIterator, @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) { + if (_partialUpsertHandler != null) { + recordInfoIterator = resolveComparisonTies(recordInfoIterator, _hashFunction); + } String segmentName = segment.getSegmentName(); segment.enableUpsert(this, validDocIds, queryableDocIds); diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java index 9a1c4f133cb8..929df2ac3ac3 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java @@ -90,7 +90,9 @@ protected long getNumPrimaryKeys() { protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds, @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator recordInfoIterator, @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) { - if (_partialUpsertHandler == null) { + if (_partialUpsertHandler != null) { + recordInfoIterator = resolveComparisonTies(recordInfoIterator, _hashFunction); + } else { // for full upsert, we are de-duping primary key once here to make sure that we are not adding // primary-key multiple times and subtracting just once in removeSegment. // for partial-upsert, we call this method in base class. @@ -298,7 +300,7 @@ public void replaceSegment(ImmutableSegment segment, @Nullable ThreadSafeMutable if (queryableDocIds == null && _deleteRecordColumn != null) { queryableDocIds = new ThreadSafeMutableRoaringBitmap(); } - addOrReplaceSegment((ImmutableSegmentImpl) segment, validDocIds, queryableDocIds, recordInfoIterator, + doAddOrReplaceSegment((ImmutableSegmentImpl) segment, validDocIds, queryableDocIds, recordInfoIterator, oldSegment, validDocIdsForOldSegment); } if (validDocIdsForOldSegment != null && !validDocIdsForOldSegment.isEmpty()) { From 3cbd93a6da00a6829b06b15d72e8522707cd9956 Mon Sep 17 00:00:00 2001 From: Chaitanya Deepthi Date: Sat, 30 May 2026 23:08:48 -0700 Subject: [PATCH 2/2] Fix Checkstyle line-length violation in BasePartitionUpsertMetadataManager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Line 661 was 121 chars; wrap argument list so each line stays ≤120. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../local/upsert/BasePartitionUpsertMetadataManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java index 6bff8a63b694..d78a23641864 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java @@ -658,8 +658,8 @@ public void replaceSegment(ImmutableSegment segment, @Nullable ThreadSafeMutable if (queryableDocIds == null && _deleteRecordColumn != null) { queryableDocIds = new ThreadSafeMutableRoaringBitmap(); } - doAddOrReplaceSegment((ImmutableSegmentImpl) segment, validDocIds, queryableDocIds, recordInfoIterator, oldSegment, - validDocIdsForOldSegment); + doAddOrReplaceSegment((ImmutableSegmentImpl) segment, validDocIds, queryableDocIds, recordInfoIterator, + oldSegment, validDocIdsForOldSegment); } if (_upsertViewManager != null) { // When using consistency mode, the old segment's bitmap is updated in place, so we get the validDocIds after