xiangfu0 opened a new pull request, #19303: URL: https://github.com/apache/pinot/pull/19303
## Problem On FULL-upsert tables, `WHERE vectorSimilarity(col, query, K)` planned the ANN search over **all physical rows** and only ANDed the upsert doc-ids snapshot around the result afterwards (`FilterPlanNode.run()`). Obsolete row versions that are physically nearest to the query consume the per-segment top-K candidate budget before the snapshot removes them, so a segment can return fewer than K rows and omit nearer current rows entirely. Repro shape: entity A's old version sits at the query vector, its new version far away, entities B/C in between. The old A rows win the K candidate slots, get removed by the snapshot AND, and B/C never become candidates — the query returns the wrong entities. The new integration test's `OPTION(skipUpsert=true)` control query demonstrates the obsolete rows are physically nearest. ## What this PR does The upsert doc-ids snapshot is now a **required candidate filter**, enforced *before* top-K selection on every vector path, and kept strictly separate from the optional metadata prefilter (which retains its adaptive `VectorSearchStrategy` behavior). The outer bitmap AND is retained as defense in depth. This lands as three units (bundled because the integration test exercises them together): ### 1. Required-filter enforcement (pinot-core) - `FilterPlanNode` captures a defensive copy of the snapshot whenever the filter tree contains a vector predicate (any depth — AND/OR/NOT), clamps it to the planned doc range (under `ConsistencyMode.NONE` the snapshot can reference a row still being written), passes it into the vector operators via the new `VectorSearchSpec` construction context, and uses the same instance for the outer AND. - `VectorSimilarityFilterOperator`: with a required filter present, candidate generation always uses the filter-aware 3-arg `getDocIds` (intersected with the optional metadata prefilter when both apply, `effectiveAllowed = required ∩ optional`); an empty snapshot/intersection returns empty without invoking the reader; unfiltered ANN is **refused** (`IllegalStateException`) rather than silently run. - `ExactVectorScanFilterOperator` and `VectorRadiusFilterOperator` scan only the required doc ids when present (top-K and threshold modes) — top-K is selected from the allowed set directly, never computed physically and intersected afterwards. Readers that cannot honor the filter are routed to the exact allowed-doc scan at plan time. - `VECTOR_SIMILARITY_RADIUS` was already *correct* without this (its candidate-saturation fallback guarantees completeness), but is included so obsolete rows stop wasting its candidate budget and triggering the expensive saturation fallback early. - Explain/trace output reports `upsertRequiredDocIdsCardinality`, the search mode actually executed (`FILTER_THEN_ANN` / `EXACT_SCAN`), and a clear `upsert_snapshot_*` fallback reason for non-filter-aware readers. ### 2. Filter-aware mutable HNSW (pinot-segment-local) `MutableVectorIndex` now implements `FilterAwareVectorIndexReader`, so consuming segments use filtered ANN instead of an exact-scan fallback: - Stores the **supplied** Pinot doc id (StoredField + NumericDocValues) and translates every search hit through it. This also fixes a latent bug on the unfiltered path, which used `ScoreDoc.doc` directly and silently assumed Lucene doc ids equal Pinot doc ids — untrue once Lucene merges renumber across commits. - Filtered search runs against a `SearcherManager` near-real-time view over the writer (refresh-coalescing, reader reused when nothing changed), so uncommitted rows — typically the *newest* version of a record — are visible. The unfiltered path keeps the cheaper last-committed view, unchanged. - Bitmap membership is tested per-leaf through doc values (`BasePinotDocIdBitmapFilterQuery`, now shared with the immutable `HnswVectorIndexReader`'s filter query so the correctness-sensitive scaffolding cannot drift). - `FilterAwareVectorIndexReader` Javadoc now documents the strict contract implementations sign up for: filtered results MUST be a subset of the bitmap, never heuristically degraded — upsert correctness depends on it. ### 3. Vector-column encoding validation (pinot-segment-local) `VectorIndexType.validate()` now rejects dictionary-encoded vector columns. Every forward-index read path used by vector search (exact-scan fallback, exact rerank, distance-threshold and radius refinement) calls `getFloatMV`, which dictionary-encoded MV readers do not implement — such configs passed validation but failed at query time. ## :warning: Backward incompatible A table config with a vector index on a column **not** declared `encodingType: RAW` (dictionary encoding is the default) was previously accepted; after this change its next table-config create/update fails validation. Such tables were already broken for the rerank/threshold/exact-scan/radius query paths. **Migration:** declare `encodingType: RAW` (or add the column to `noDictionaryColumns`) and reload segments. All in-tree vector tables already use RAW. Other behavior changes to be aware of: - Vector query results on FULL-upsert tables change — previously silently wrong (obsolete rows consumed candidate slots). - Third-party `VectorIndexReader` plugins that are not filter-aware now take an exact allowed-doc scan on upsert tables (logged at DEBUG with an `upsert_snapshot_*` reason), and fail loudly if no forward index exists where they previously returned upsert-inconsistent unfiltered ANN results. All built-in readers (immutable HNSW/IVF_FLAT/IVF_PQ/IVF_ON_DISK and now mutable HNSW) are filter-aware. - The exact-scan log for the expected upsert fallback dropped WARN → DEBUG; genuinely-missing-index scans keep the WARN. - No wire/storage format changes; the mutable index's new doc-values field lives only in the process-local realtime temp index. ## Testing All deterministic — no reliance on probabilistic ANN recall (fixtures place obsolete rows physically nearest, with distinct distances). - **pinot-core (110 tests)**: planner/operator tests assert the 3-arg filtered reader is invoked with exactly the snapshot (ArgumentCaptor), the unfiltered overload never runs, required ∩ optional intersection is delivered without mutating either input, empty snapshot/intersection skip the reader, null snapshot preserves existing behavior byte-for-byte, non-filter-aware readers route to the allowed-doc exact scan (or fail loudly with no forward index), the snapshot copy is defensive (post-plan mutation doesn't leak), out-of-range snapshot ids are clamped, mutable segments don't wire the *optional* prefilter, and radius filtered/brute-force/saturation paths behave. - **pinot-segment-local (26 tests)**: real immutable HNSW filtered search excludes physically-nearest disallowed docs; mutable index filtered search is NRT-visible for uncommitted rows, translates supplied (offset) Pinot doc ids on both filtered and unfiltered paths, and stays correct under a live concurrent writer; RAW-encoding validation accept/reject. - **Integration (`custom/VectorUpsertTableTest`, shared suite — no dedicated cluster)**: FULL upsert, one Kafka partition, 2-D vectors, stable producer keys, polling-based waits. Covers (1) all records in one consuming segment (NRT filtered mutable HNSW) and (2) obsolete rows sealed into an immutable segment with the new versions in the next consuming segment (filtered immutable HNSW, cross-segment invalidation); both query engines; `skipUpsert=true` control proving obsolete rows are physically nearest; agreement with the exact scalar-distance query. Pre-existing `VectorTest` (19) and `IvfPqVectorRealtimeTest` (6) pass unchanged. - spotless / checkstyle / license clean on all touched modules. ## Notes / follow-ups - Filtered ANN with a near-full snapshot pays an O(numDocs) accept-bitset cost per query per segment that unfiltered ANN did not; this is correctness-mandated, but a JMH comparison on a large lightly-upserted segment would quantify it, and a dense-snapshot fast path (over-fetch + intersect + retry) is a possible optimization. - A public force-prefilter query option (guaranteed tenant/model-scoped top-K over metadata filters) is deliberately out of scope. -- 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]
