xiangfu0 commented on PR #19336:
URL: https://github.com/apache/pinot/pull/19336#issuecomment-5433294832
## Review — mutable HNSW filtered search
Reviewed at `e4ab80c90a` (2 files, +393/−31), against `master` now that
#19297–#19300 have landed. The restack to just the mutable-index layer is a big
improvement over the earlier cumulative version, and the stray
`org.jetbrains:annotations` dependency is gone.
### What is good
- **Fixes a latent bug.** The old code stored `_nextDocId++`, an
index-internal counter, but `MutableIndex.add(..., docId)` documents that "rows
can be added in no particular order, so the docId is required". Storing the
supplied doc id is correct; the counter would mis-map doc ids under
out-of-order ingestion.
- `translateTopDocs` sorts hits by Lucene doc id before `advanceExact` —
necessary, since `NumericDocValues` only advances forward. Easy to get wrong;
it is right here.
- Translating through doc values (rather than stored fields) keeps the
per-hit cost low.
- `isCacheable=false` plus identity-based `equals` is the correct pairing
for a query-scoped bitmap — it keeps a per-query filter out of Lucene's query
cache.
- Per-instance index directories fix Lucene write-lock collisions between
same-JVM replicas.
- Tests cover the genuinely risky paths: NRT visibility, same-generation
translation, bitmap copy before async dispatch, directory isolation.
### 1. (Major) NRT reading has no publication bound
`DirectoryReader.open(_indexWriter)` makes rows visible as soon as they are
added to Lucene — including rows the segment has **not yet published to
queries**. Nothing bounds them: `translateTopDocs` applies no watermark, and
`_numDocs` is used only for commit cadence and debug stats.
An earlier revision of this PR bounded exactly this, via
`VectorCandidateScope.forMutableSegment(upsertDocIds, numDocs, ...)`. That
bound lived in the base-fix half, which is now merged in a different form, and
nothing replaced it here.
Where it bites: on a **non-upsert realtime table** `FilterPlanNode` supplies
no required doc ids, so the search is unfiltered and the result set is
unbounded. A returned doc id at or beyond the `numDocs` watermark the plan
captured is out of range for everything downstream. Upsert tables are protected
only incidentally, because the snapshot happens to bound them.
Suggested fix: no bitmap needed. Capture the watermark and drop hits at or
above it in `translateTopDocs`, or test `pinotDocId < numPublishedDocs` inside
`FilteredDocIdSetIterator` — an O(1) check per candidate and no allocation.
(Bounding in the iterator preserves top-K; bounding after selection can return
fewer than K.)
### 2. (Major) Duplicates a filter query that already exists
`HnswVectorIndexReader.RoaringBitmapFilterQuery` already implements
bitmap-filtered traversal for the immutable reader. This PR adds a second,
private implementation for the mutable one. Two copies of Lucene
filter-iterator logic will drift.
#19303 already solves this: it extracts `BasePinotDocIdBitmapFilterQuery`
and deletes 72 lines from `HnswVectorIndexReader`, leaving one implementation
both readers extend. Adopting that extraction here would be strictly better
than a second private copy.
### 3. (Major) An NRT reader is opened on every search
`DirectoryReader.open(_indexWriter)` runs per query. Lucene's guidance is
`SearcherManager`/`ReaderManager` with `maybeRefresh()`. This was noted as
deferred while the fallback was rare, but this PR makes filtered mutable search
the default path for realtime vector queries, so the per-search reader is now
on the hot path. Worth a benchmark, or an issue to track, rather than silence.
### 4. (Minor) Temporary directories lose attribution
`Files.createTempDirectory("pinot-mutable-vector-")` replaces the old
`<tmp>/<segment>/<column>` layout. Isolation is the right fix, but after a JVM
crash the orphaned directories can no longer be traced to a segment or column.
Including a sanitized segment and column name in the prefix keeps both
properties.
---
_🤖 Automated review by [Claude Code](https://claude.com/claude-code)_
--
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]
