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: [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]
