This is an automated email from the ASF dual-hosted git repository. Jackie-Jiang pushed a commit to branch hotfix_18588 in repository https://gitbox.apache.org/repos/asf/pinot.git
commit 4423eec1c564ccfa9ddc75185cbade74be005013 Author: Chaitanya Deepthi <[email protected]> AuthorDate: Mon May 18 20:30:50 2026 -0700 Preserve queryableDocIds tracking when preloading upsert segment with no valid docs (#18527) * Make sure the queryable doc ids are not null when the valid doc ids are empty * Add unit tests for the regression * Simplify the code (cherry picked from commit a986489af495d8d41f9816a62f7defea5dc2bfc1) --- .../upsert/BasePartitionUpsertMetadataManager.java | 4 +- ...rrentMapPartitionUpsertMetadataManagerTest.java | 60 ++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) 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 6df010ae0e8..f0c8f850b89 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 @@ -437,7 +437,9 @@ public abstract class BasePartitionUpsertMetadataManager implements PartitionUps if (validDocIds.isEmpty()) { _logger.info("Skip preloading segment: {} without valid doc, current primary key count: {}", segment.getSegmentName(), getNumPrimaryKeys()); - segment.enableUpsert(this, new ThreadSafeMutableRoaringBitmap(), null); + ThreadSafeMutableRoaringBitmap queryableDocIds = + (_deleteRecordColumn == null) ? null : new ThreadSafeMutableRoaringBitmap(); + segment.enableUpsert(this, new ThreadSafeMutableRoaringBitmap(), queryableDocIds); return; } if (isTTLEnabled() && !_comparisonColumns.isEmpty()) { diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java index bbba657b985..926af5e34ce 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java @@ -71,6 +71,7 @@ import org.apache.pinot.spi.utils.ReadMode; import org.apache.pinot.spi.utils.builder.TableConfigBuilder; import org.apache.pinot.spi.utils.builder.TableNameBuilder; import org.apache.pinot.util.TestUtils; +import org.mockito.ArgumentCaptor; import org.mockito.MockedConstruction; import org.roaringbitmap.buffer.MutableRoaringBitmap; import org.testng.annotations.AfterClass; @@ -85,6 +86,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockConstruction; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.*; @@ -1356,6 +1358,64 @@ public class ConcurrentMapPartitionUpsertMetadataManagerTest { } } + /** + * Regression test: when preloading an immutable segment whose validDocIds snapshot is empty and the table is + * configured with a deleteRecordColumn, doPreloadSegment's fast path must initialize queryableDocIds to an + * empty bitmap (not null). Passing null would set _queryableDocIds = null on the segment, causing subsequent + * doTakeSnapshot rounds to skip persisting the queryableDocIds bitmap file and leaving the on-disk snapshot + * frozen at its pre-restart contents. + */ + @Test + public void testPreloadSegmentEmptyValidDocIdsWithDeleteColumn() { + _contextBuilder.setEnableSnapshot(true).setDeleteRecordColumn(DELETE_RECORD_COLUMN); + ConcurrentMapPartitionUpsertMetadataManager upsertMetadataManager = + new ConcurrentMapPartitionUpsertMetadataManager(REALTIME_TABLE_NAME, 0, _contextBuilder.build()); + + ImmutableSegmentImpl segment = mock(ImmutableSegmentImpl.class); + when(segment.getSegmentName()).thenReturn("emptyValidDocIdsSegment"); + when(segment.loadDocIdsFromSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME)).thenReturn( + new MutableRoaringBitmap()); + + upsertMetadataManager.preloadSegment(segment); + + ArgumentCaptor<ThreadSafeMutableRoaringBitmap> validDocIdsCaptor = + ArgumentCaptor.forClass(ThreadSafeMutableRoaringBitmap.class); + ArgumentCaptor<ThreadSafeMutableRoaringBitmap> queryableDocIdsCaptor = + ArgumentCaptor.forClass(ThreadSafeMutableRoaringBitmap.class); + verify(segment).enableUpsert(eq(upsertMetadataManager), validDocIdsCaptor.capture(), + queryableDocIdsCaptor.capture()); + assertNotNull(validDocIdsCaptor.getValue()); + assertTrue(validDocIdsCaptor.getValue().getMutableRoaringBitmap().isEmpty()); + assertNotNull(queryableDocIdsCaptor.getValue(), + "queryableDocIds must be non-null when deleteRecordColumn is configured"); + assertTrue(queryableDocIdsCaptor.getValue().getMutableRoaringBitmap().isEmpty()); + } + + /** + * Companion to {@link #testPreloadSegmentEmptyValidDocIdsWithDeleteColumn}: when deleteRecordColumn is not + * configured, the fast path should still pass null for queryableDocIds since queryable tracking is disabled. + */ + @Test + public void testPreloadSegmentEmptyValidDocIdsWithoutDeleteColumn() { + _contextBuilder.setEnableSnapshot(true); + ConcurrentMapPartitionUpsertMetadataManager upsertMetadataManager = + new ConcurrentMapPartitionUpsertMetadataManager(REALTIME_TABLE_NAME, 0, _contextBuilder.build()); + + ImmutableSegmentImpl segment = mock(ImmutableSegmentImpl.class); + when(segment.getSegmentName()).thenReturn("emptyValidDocIdsSegment"); + when(segment.loadDocIdsFromSnapshot(V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME)).thenReturn( + new MutableRoaringBitmap()); + + upsertMetadataManager.preloadSegment(segment); + + ArgumentCaptor<ThreadSafeMutableRoaringBitmap> queryableDocIdsCaptor = + ArgumentCaptor.forClass(ThreadSafeMutableRoaringBitmap.class); + verify(segment).enableUpsert(eq(upsertMetadataManager), any(ThreadSafeMutableRoaringBitmap.class), + queryableDocIdsCaptor.capture()); + assertNull(queryableDocIdsCaptor.getValue(), + "queryableDocIds must be null when deleteRecordColumn is not configured"); + } + @Test public void testAddRecordWithDeleteColumn() throws IOException { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
