Re: [PR] Enable filtered vector search on mutable HNSW and share the doc-id filter query [pinot]

["xiangfu0 (via GitHub)" <gi...@apache.org>]

xiangfu0 commented on PR #19303:
URL: https://github.com/apache/pinot/pull/19303#issuecomment-5491033665

   Thanks — follow-ups addressed on `a4e24efb01`. Status per item:
   
   **1. Blocking refresh under concurrent ingestion — benchmarked.** Added `BenchmarkVectorFilterWorkloads`, which
   runs filtered queries against a consuming segment while it ingests. 1 writer + 8 reader threads, 2000 initial +
   2000 streamed docs, 1% selectivity:
   
   | ingest docs/s | Recall@10 | p50 | p95 | QPS |
   |---|---|---|---|---|
   | 4600 | 1.000 | 156 us | 369 us | 38.6k |
   
   So the refresh cost is real but bounded at this scale, and ingestion keeps up alongside it. I also changed the
   path to try the non-blocking `maybeRefresh()` first and only fall back to `maybeRefreshBlocking()` when another
   thread already holds the refresh lock. Proper coalescing via a background reopen thread
   (`ControlledRealTimeReopenThread`, as `RealtimeLuceneIndexRefreshManager` does for text indexes) is the right
   fix but is a larger change; I've recorded the trade-off in the class Javadoc and will send it as a follow-up
   rather than grow this PR.
   
   **2. Empty filter short-circuit — done.** `getDocIds(vector, topK, preFilterBitmap)` returns an empty bitmap
   before `submitSearch`, so an empty filter takes neither the refresh nor the searcher acquire.
   
   **3. Doc-values walk is O(index size) — benchmarked, kept.** The selectivity sweep is in the same benchmark.
   Worth noting the iterator reports `cost()` as the filter cardinality, so Lucene's own planning sees the
   selective filter, and at 0.1%–10% selectivity filtered mutable HNSW measures **recall 1.000** (vs 0.29–0.39 for
   the IVF backends) because a small accepted set makes Lucene short-circuit to an exact search. A reverse
   Pinot-doc-ID mapping would bound the scan but costs memory per consuming segment; I'd rather land that with its
   own numbers than fold it in here.
   
   **4. Live-writer test overlap — done.** The test now uses a `Phaser` plus a filter bitmap that blocks the search
   inside its first membership check until that phase's concurrent writes complete, so the overlap is deterministic
   rather than timing-dependent. The `finally` force-terminates the phaser, interrupts and joins the writer with a
   timeout, and asserts it actually died.
   
   **5. IvfPq test strengthened — done.** Renamed to `testPreFilterExcludesNearestDocsFromCandidateGeneration`. It
   now asserts up front that the allowed docs lie *outside* the unfiltered top-K, then asserts the filtered search
   returns exactly those docs — so an unfiltered-top-K-then-intersect implementation would return empty and fail.
   
   **6. `BaseFilterQuery` name.** Leaving as-is: the previous round asked to shorten it away from
   `BasePinotDocIdBitmapFilterQuery`, and the bitmap/doc-id contract is now stated in the class Javadoc along with
   ownership and thread-safety. Happy to rename if you'd prefer to settle it the other way.
   
   **Minor.** Commit description updated — the private-per-index-directory text was already dropped with that
   change, and the stale `BasePinotDocIdBitmapFilterQuery` reference is now fixed.
   
   Validation on `a4e24efb01`: 76 `pinot-segment-local` + 608 `pinot-core` vector/filter tests, `VectorUpsertTableTest`
   (2/2), and spotless/checkstyle/license/test-compile all pass.
   


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org