Use upsert snapshot view in hasNoQueryableDocs to fix consistency-mode pruning#18478
Merged
Jackie-Jiang merged 23 commits intoMay 15, 2026
Merged
Conversation
…e prune races
SegmentPrunerService.removeEmptySegments uses UpsertUtils.hasNoQueryableDocs to drop
empty segments before query execution. Today that method reads the segment's live
ThreadSafeMutableRoaringBitmap from getQueryableDocIds() / getValidDocIds(). For
upsert tables running with consistency mode SYNC or SNAPSHOT, the live bitmap can
diverge from the per-query snapshot maintained by UpsertViewManager between
refreshes — so pruning a segment as "empty here" can drop one that is non-empty in
the snapshot view the query will actually scan, producing wrong row counts.
Surface the snapshot to the pruner without breaking SPI layering:
* IndexSegment (pinot-segment-spi) gains two default methods:
- getQueryableDocIdsSnapshot() -> @nullable MutableRoaringBitmap
- isUpsertConsistencyModeEnabled() -> boolean
Both default to null / false so non-upsert segments are unaffected and no
module dependency changes.
* ImmutableSegmentImpl / MutableSegmentImpl (pinot-segment-local) override
both, delegating to PartitionUpsertMetadataManager.getUpsertViewManager().
Existence of the view manager is exactly equivalent to consistency mode
being enabled (see BasePartitionUpsertMetadataManager#159-167).
* PartitionUpsertMetadataManager gains @nullable UpsertViewManager
getUpsertViewManager(); BasePartitionUpsertMetadataManager already
implements it, so no impl changes are needed.
* UpsertViewManager gains a narrow lookup
getQueryableDocIdsSnapshot(IndexSegment) that returns the snapshot bitmap
for the segment from the most recent refresh, or null. Lock-free: reads the
volatile map reference; writers replace the map atomically under
_upsertViewLock, never mutating the old one in place.
* UpsertUtils.hasNoQueryableDocs now consults the snapshot first, returns
false when consistency mode is on but the snapshot is not yet populated
(first refresh hasn't run, or segment was just tracked), and otherwise
falls back to live bitmaps as before.
Behavior on non-consistency-mode tables is unchanged.
Seven cases covered:
* Non-upsert / non-consistency-mode tables (existing behavior preserved):
- live queryable bitmap empty -> true
- live queryable bitmap non-empty -> false
- queryable null, valid bitmap empty -> true
- both bitmaps null -> false
* Consistency-mode upsert tables (new snapshot-aware behavior):
- snapshot empty, live bitmap non-empty -> true (snapshot wins)
- snapshot non-empty, live bitmap empty -> false (snapshot wins)
- snapshot absent (first refresh hasn't run or segment just tracked),
live bitmap empty -> false (conservative deferral)
…ke sure we don't see inconsistencies in queries
…ed to make sure we don't see inconsistencies in queries" This reverts commit d61e953.
deepthi912
added
upsert
deepthi912
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18478 +/- ##
============================================
- Coverage 63.68% 63.68% -0.01%
Complexity 1684 1684
============================================
Files 3266 3266
Lines 199836 199922 +86
Branches 31023 31055 +32
============================================
+ Hits 127272 127321 +49
- Misses 62424 62442 +18
- Partials 10140 10159 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
reviewed
May 13, 2026
Contributor
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal issue; see inline comment.
Fixes checkstyle UnusedImports violation introduced by an earlier refactor; the symbol is no longer referenced after the simplification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Jackie-Jiang
approved these changes
May 14, 2026
Addresses PR review comment from @Jackie-Jiang — switch the new javadoc blocks (getQueryableDocIdsSnapshot, isUpsertConsistencyModeEnabled, hasNoQueryableDocs) from /** */ with <ol><li> to /// markdown style, matching the style already used elsewhere in this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Left over after the SPI was simplified back to a no-op default hasNoQueryableDocs. Fixes the checkstyle UnusedImports violation that broke the pinot-segment-spi build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The earlier mock(IndexSegment.class) approach didn't actually exercise the
hasNoQueryableDocs default impl on the SPI (Mockito returns false for
abstract methods) and couldn't reach the consistency-mode branch.
Switch to constructing a minimal ImmutableSegmentImpl with mocked
SegmentDirectory + SegmentMetadataImpl, wire the upsert path via
enableUpsert(...), and stub PartitionUpsertMetadataManager +
UpsertViewManager only where each test needs them. Same pattern that
BasePartitionUpsertMetadataManagerTest already uses.
Covers all five hasNoQueryableDocs branches:
- Non-upsert (manager null) — returns false
- Non-consistency upsert: live queryable empty / non-empty / missing
(falls back to validDocIds) / both bitmaps missing
- Consistency-mode upsert: snapshot empty / non-empty / absent
8 tests, all green locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…segments The upsert helper (mockUpsertIndexSegment) previously returned mock(IndexSegment.class). SegmentPrunerService.removeEmptySegments calls segment.hasNoQueryableDocs(), and Mockito returns the default (false) for that method on a plain interface mock — so the "empty" tests (emptyValidPruned, emptyQueryablePruned) never saw the segment as empty and failed. Construct a real ImmutableSegmentImpl(SegmentDirectory, SegmentMetadataImpl, emptyMap, null), stub SegmentMetadataImpl#getTotalDocs, and wire the upsert state via enableUpsert(mockManager, validDocIds, queryableDocIds) where the manager's getUpsertViewManager() returns null (non-consistency mode). The real hasNoQueryableDocs body then evaluates the live bitmaps the test set up. Same pattern BasePartitionUpsertMetadataManagerTest already uses. All 10 tests pass locally; checkstyle + spotless clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SegmentPrunerService.removeEmptySegmentscallsUpsertUtils.hasNoQueryableDocsto drop empty segments before query execution. Today that method reads the segment's liveThreadSafeMutableRoaringBitmapviaIndexSegment.getQueryableDocIds()/getValidDocIds().For upsert tables running with consistency mode
SYNCorSNAPSHOT, the live bitmap can diverge from the per-query snapshot maintained byUpsertViewManagerbetween refreshes. So a "no queryable docs here" reading on the live bitmap can cause the pruner to drop a segment that is non-empty in the snapshot view the query is about to scan — producing wrong row counts.Behavior on non-consistency-mode tables and on non-upsert tables is identical to today.
Tests
New
UpsertUtilsTestwith 7 cases — passing locally:truefalsenull, valid bitmap emptytruenullfalsetrue(snapshot wins)false(snapshot wins)false(conservative deferral)