Skip to content

Enforce the upsert doc-ids snapshot in vector candidate generation - #19303

Open
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/pinot-upsert-vector-candidates-a592ea
Open

Enforce the upsert doc-ids snapshot in vector candidate generation#19303
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/pinot-upsert-vector-candidates-a592ea

Conversation

@xiangfu0

Copy link
Copy Markdown
Contributor

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.

⚠️ 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.

On FULL-upsert tables, VECTOR_SIMILARITY spent its per-segment top-K candidate
budget on all physical rows and the doc-ids snapshot was only ANDed afterwards,
so obsolete row versions could consume the K slots and crowd out nearer current
rows. The snapshot is now a required candidate filter enforced before top-K
selection in every vector path (VECTOR_SIMILARITY and VECTOR_SIMILARITY_RADIUS,
filtered ANN and exact-scan fallback), with the outer bitmap AND retained as
defense in depth.

- FilterPlanNode passes a clamped defensive copy of the snapshot into the
  vector operators; empty snapshots plan an EmptyFilterOperator; unfiltered ANN
  is refused (fail loud) whenever a required filter cannot be honored.
- MutableVectorIndex implements FilterAwareVectorIndexReader: stores the
  supplied Pinot doc id, translates hits through it instead of assuming
  ScoreDoc.doc == docId (fixes a latent bug on merge-renumbered indexes), and
  serves filtered search from a SearcherManager NRT view so uncommitted rows
  are visible.
- ExactVectorScanFilterOperator and VectorRadiusFilterOperator scan only the
  required doc ids when present; explain output reports the applied filter
  cardinality, search mode and fallback reason.
- VectorSearchSpec construction context replaces the growing operator
  constructor ladders; shared BasePinotDocIdBitmapFilterQuery backs both
  Lucene bitmap filter queries.
- VectorIndexType.validate now rejects dictionary-encoded vector columns
  (they fail at query time on every forward-index read path). Backward
  incompatible for previously-accepted misconfigured table configs: declare
  encodingType RAW / noDictionary for vector columns.
@xiangfu0 xiangfu0 added backward-incompat Introduces a backward-incompatible API or behavior change bug Something is not working as expected release-notes Referenced by PRs that need attention when compiling the next release notes labels Aug 19, 2026
@codecov-commenter

codecov-commenter commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 15.70513% with 263 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.19%. Comparing base (1019930) to head (66ecb21).
⚠️ Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
...operator/filter/ExactVectorScanFilterOperator.java 0.00% 72 Missing ⚠️
...re/operator/filter/VectorRadiusFilterOperator.java 0.00% 63 Missing ⚠️
...ava/org/apache/pinot/core/plan/FilterPlanNode.java 0.00% 44 Missing and 1 partial ⚠️
...perator/filter/VectorSimilarityFilterOperator.java 0.00% 44 Missing ⚠️
...e/pinot/core/operator/filter/VectorSearchSpec.java 0.00% 22 Missing ⚠️
...eaders/vector/BasePinotDocIdBitmapFilterQuery.java 57.14% 8 Missing and 1 partial ⚠️
...local/realtime/impl/vector/MutableVectorIndex.java 80.95% 6 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (1019930) and HEAD (66ecb21). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (1019930) HEAD (66ecb21)
java-25 6 5
temurin 6 5
unittests1 1 0
unittests 2 1
Additional details and impacted files
@@              Coverage Diff              @@
##             master   #19303       +/-   ##
=============================================
- Coverage     67.10%   39.19%   -27.91%     
+ Complexity     1424     1423        -1     
=============================================
  Files          3459     3464        +5     
  Lines        219697   220567      +870     
  Branches      34985    35191      +206     
=============================================
- Hits         147432    86459    -60973     
- Misses        60488   126228    +65740     
+ Partials      11777     7880     -3897     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 39.19% <15.70%> (-27.91%) ⬇️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 39.19% <15.70%> (-27.91%) ⬇️
unittests 39.19% <15.70%> (-27.91%) ⬇️
unittests1 ?
unittests2 39.19% <15.70%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backward-incompat Introduces a backward-incompatible API or behavior change bug Something is not working as expected release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants