xiangfu0 commented on PR #19303: URL: https://github.com/apache/pinot/pull/19303#issuecomment-5433295004
## Review — enforce the upsert doc-ids snapshot in vector candidate generation Reviewed at `aebbdae5f6` (18 files, +2221/−239) against current `master`. ### Context first: half of this PR has already landed #19297–#19300 merged the base fix, so `ExactVectorScanFilterOperator`, `VectorSimilarityFilterOperator`, `FilterPlanNode` and their tests are now duplicated by (and will conflict with) `master`. That part of this PR is done. What is **not** in `master`, and not in #19336 either, is the rest — and it is the most valuable part: | Piece | Status elsewhere | | --- | --- | | `FilterAwareVectorIndexReader` contract hardening | nowhere else | | `BasePinotDocIdBitmapFilterQuery` | nowhere else | | `HnswVectorIndexReader` (−72 lines of duplication) | nowhere else | | `VectorIndexType` | nowhere else | | `VectorRadiusFilterOperator` | nowhere else | | `MutableVectorIndex` | overlaps #19336 | The recommendation is to reduce this PR to those rows and let it be the mutable-index/SPI PR, rather than close it — closing it as superseded would silently drop the items above. ### What is genuinely good - **The SPI hardening is the standout.** Turning `supportsPreFilter()` into an unconditional commitment — with the class-level note that the filter is a correctness contract and an implementation must never heuristically degrade to unfiltered search — closes a real hazard. Under the old "may support it only for certain selectivities" wording, a reader could legally answer `true` at plan time and degrade at execution time, which is precisely the case where obsolete row versions would silently re-enter top-K. Making it binary, with exact scan as the honest fallback, is the right contract. - **`BasePinotDocIdBitmapFilterQuery` is the correct factoring.** One constant-score weight/scorer, a `createLeafIterator` hook per reader, `isCacheable=false`, identity-based `equals`/`hashCode` including `getClass()`. It removes 72 lines from `HnswVectorIndexReader` instead of adding a second copy — the opposite of what #19336 currently does. - Stores the **supplied** Pinot doc id rather than an internal counter, fixing the same latent out-of-order-ingestion bug. - Keeps `DirectoryReader.open(_indexDirectory)`, so it does **not** have the unpublished-document exposure that NRT reading introduces in #19336. - Extending the required-doc-ids restriction to `VectorRadiusFilterOperator` is unique to this PR, and the Javadoc is commendably honest that radius results were already correct and this is about not burning the candidate budget (and the saturation latency cliff) on obsolete rows. ### 1. (Blocking, process) Drop the already-merged half Rebase onto current `master` and remove the base-fix files. Without that this PR cannot merge, and its diff hides the parts that still need review. ### 2. (Major) `VectorSearchSpec` reverses a merged review decision `master` deliberately passes the required doc-id bitmap directly. The wrapper type that used to hold it was removed during review of #19297 — the reviewer's point was that a single-field wrapper adds a layer without adding meaning, and a type should be introduced when a second field actually exists. Reintroducing `VectorSearchSpec` would undo that. If it carries more than one field, that changes the argument — but then the extra state should be the justification, called out explicitly. ### 3. (Minor) Translate through doc values, not stored fields `translateTopDocs` resolves each hit with `storedFields.document(scoreDoc.doc)`, which decompresses a stored-field block per hit, while the numeric doc values needed for filtering are already maintained on the same documents. Reading the doc id from doc values instead is cheaper on the hot path and would let the `StoredField` be dropped entirely, halving the per-document id storage. (#19336 does the translation this way, and additionally sorts hits by Lucene doc id first, which is required for forward-only `advanceExact`.) ### 4. Freshness trade-off worth stating Committed-only reading means rows added since the last commit are invisible to vector search, so a FULL-upsert query can return fewer than K rows until the next commit fires. That is `master`'s existing behavior and not a regression — but since #19336 takes the opposite approach (NRT, fresher, and needing a publication bound), whichever lands should say which trade-off it is choosing and why. --- _🤖 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]
