fix(index): enforce RFC-109 vector read correctness - #19513
fix(index): enforce RFC-109 vector read correctness#19513chrevanthreddy wants to merge 8 commits into
Conversation
…d exact rerank Add the engine-agnostic vector search core for the RFC-104 vector index (apache#19094), all in hudi-common with no Spark/engine dependencies: - search/: candidate generation and top-K accumulation over MDT postings, fetch planning, execution-mode selection (approx vs exact) with deadline and budget control, record-index candidate arbitration, and continuation - DefaultExactVectorScorer / DefaultVectorExactReranker: exact rerank from authoritative base-table vectors - VectorIndexPruner: RaBitQ error-bounded two-pass pruning - VectorIndexArbiter: approximate vs exact decisioning Pure-CPU algorithm library over the MDT schema (apache#19097) and RaBitQ encoder (apache#19098); IO is injected via interfaces (candidate sources, fetch tasks). Tests: 34 cases across executor, scorer, reranker, fetch planner, arbiter, pruner, execution-mode selector, and continuation. Compiles + tests green on hudi-common (JDK17). Part of apache#19101, apache#19102, apache#19103. Note: VectorIndexMetadataCache (generation-visibility, apache#19100) is deferred to the reader-consistency PR where its shard-count fallback semantics belong.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR adds the engine-neutral RFC-109 vector-search core — candidate/arbitration/rerank pipeline, RaBitQ encoding and posting-block serialization, delta-overlay resolution, continuation windowing, and the new VECTOR_INDEX metadata-table partition — along with a broad unit test suite. The logic traces as internally consistent; a few robustness edge cases are worth a look, notably the continuation controller handing out a live subList view, an IndexOutOfBoundsException risk in the public findTopClusters, and a metadata-consistency question across the two cluster-stats factory methods. Please take a look at the inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A few minor readability nits: inline FQN imports in a test lambda body, and a couple of assertTrue(!...) calls that would read more clearly as assertFalse(...). a couple of naming nits: the index option keys don't follow the hoodie.xxx.yyy.zzz convention used everywhere else in Hudi config, and a control-character literal in the fetch planner would read more clearly as '�'. a few naming and readability suggestions below, most notably a method/field name collision in VectorTopKAccumulator, an undocumented magic constant shared across two files, and a factory overload that silently discards all three parameters it accepts.
| return java.util.Collections.emptyList(); | ||
| } | ||
| int size = cursor == 0 ? initialRerankCandidates : rerankBatchSize; | ||
| int end = Math.min(cursor + size, effectiveMax); |
There was a problem hiding this comment.
🤖 nextBatch() returns orderedPool.subList(...), which is a live view backed by the full pool, and ListVectorCandidatePool passes it straight into HoodieListData.eager(...) (which I confirmed keeps the list by reference, no copy). Two things I'd worry about: ArrayList's SubList isn't Serializable, so if a batch ever gets shipped/serialized it'll throw; and since it's a mutable view, any downstream consumer that sorts/mutates the batch in place would corrupt the pool and later batches. Would it be safer to return new ArrayList<>(orderedPool.subList(cursor, end))?
| * Returns the cluster ids closest to the query (sorted best-first). | ||
| * Linear scan; HNSW routing replaces this in Phase 2. | ||
| */ | ||
| public int[] findTopClusters(float[] query, int numProbes) { |
There was a problem hiding this comment.
🤖 findTopClusters is public but will throw IndexOutOfBoundsException at scored.get(i) if numProbes > centroids.length. probe() guards this via Math.min(...), but the class doc says this is also called directly by the Spark file index and Trino split manager. Since the actually-trained centroid count can be smaller than the configured num_clusters, would it be worth clamping numProbes to centroids.length here too, mirroring probe?
| HoodieVectorIndexClusterStats stats = new HoodieVectorIndexClusterStats( | ||
| 0, | ||
| 1, | ||
| java.util.Collections.emptyList(), |
There was a problem hiding this comment.
🤖 createVectorIndexClusterStatsRecord and createVectorIndexClusterManifestRecord above both write to the same key (clusterStats(generation, clusterId)), but this variant sets fileGroupIds to emptyList() and shardCount to 1. Since VECTOR_INDEX doesn't override combineMetadataPayloads (the default returns newer wholesale), a stats update landing after the manifest record would wipe the reader-critical fileGroupIds/shardCount. Is the intent that these two writers merge field-by-field, or that only one is ever written per key? Looks latent for now (only the manifest variant is wired in), but worth pinning down before incremental maintenance lands.
| return pool; | ||
| }; | ||
| RecordIndexLookup lookup = (keys, instant) -> { | ||
| java.util.Map<String, HoodieRecordGlobalLocation> locations = new java.util.HashMap<>(); |
There was a problem hiding this comment.
🤖 nit: the inline java.util.Map / java.util.HashMap / java.util.ArrayList FQNs inside the lambda bodies are harder to scan than normal imports — could you move these to the top-level import block instead?
| for (VectorSearchResult r : results) { | ||
| keys.add(r.getRecordKey()); | ||
| } | ||
| assertTrue(keys.contains("k1") && keys.contains("k2")); |
There was a problem hiding this comment.
🤖 nit: assertTrue(!keys.contains("k3")) reads more clearly as assertFalse(keys.contains("k3")) — matches how the assertion failure message reads and is consistent with the JUnit idiom.
| fallback++; | ||
| } | ||
| } | ||
| assertEquals(2, positional); |
There was a problem hiding this comment.
🤖 nit: assertTrue(!r.isPositional()) could be assertFalse(r.isPositional()) — same reasoning as the assertTrue(!...) pattern a few lines up with isPositional checks.
| public final class VectorIndexOptions { | ||
|
|
||
| public static final String METRIC = "vector.metric"; | ||
| public static final String QUANTIZER = "vector.quantizer"; |
There was a problem hiding this comment.
🤖 nit: these option keys use a vector.xxx prefix rather than the Hudi-wide hoodie.xxx.yyy.zzz dot-separated lowercase convention. Could they be hoodie.vector.index.metric, hoodie.vector.index.num_clusters, etc.? Using the standard prefix keeps them consistent with every other hoodie.* config key and makes them easier to discover alongside the rest of the table-config surface.
3 participants
Summary
Adds the engine-neutral RFC-109 vector search core and closes the format-sensitive read correctness gaps before incremental maintenance lands.
HoodieDataand only collects bounded per-batch top-K results;This PR is stacked on #19318 and includes its dependency commits until the earlier RFC-109 PRs merge.
Validation
Java 17:
hudi-common: 2,320 tests passed, zero failures/errors/skips;git diff --check: passed.Tracking
Contributes to #19102 and #19103.