Add a de-duplicating flat vector format

[kaivalnp]

Description

Closes #14758

Add a new de-duplicating vector format that only stores unique vectors on disk.
De-duplication is done for vectors across all docs and fields indexed by the format.

Disclaimer: This was mostly written by an AI, with me refining the implementation through prompts -- although I think it did a pretty good job on its own!

Details about the format itself (layout of vectors on disk, de-duplication strategy during flush and merge, performance tradeoffs, etc) are included in a markdown doc in the PR.

[kaivalnp]

luceneutil KNN benchmarks on Cohere v3 vectors, 1024d, DOT_PRODUCT similarity, 1M vectors, 10K queries, no quantization.

Important: filterStrategy = index-time-filter is used here (added in mikemccand/luceneutil#468), which simply creates a new vector field with randomly selected filterSelectivity proportion of docs, and uses the smaller field for search.

main

recall  latency(ms)  netCPU  visited  index(s)  index_docs/s  force_merge(s)  index_size(MB)  filterSelectivity
 0.988        2.987   2.982     8122    239.73       4171.41          418.68         6077.99               0.50
 0.991        2.398   2.397     7415    211.53       4727.37          296.96         4862.33               0.20

This PR

recall  latency(ms)  netCPU  visited  index(s)  index_docs/s  force_merge(s)  index_size(MB)  filterSelectivity
 0.988        2.724   2.720     8129    248.46       4024.79          466.43         4130.17               0.50
 0.991        2.173   2.170     7414    214.40       4664.16          396.85         4085.20               0.20

As we can imagine, if the same vector is indexed in a second field for 50% of docs, the index size increases by ~50% on main (roughly 4GB -> 6GB). However with the de-duplicating vector format, the same vectors are re-used and there is (almost) no increase in index size!

Indexing is slightly slower (<5%) + merges are non-trivially slower (up to 33%, currently looking into speeding this up).

[msokolov]

It seems like we pay a small penalty for doing the indirected lookup. I wonder if we could save this in the case of the "main" field and only pay it for secondary fields if we change the API a bit so that users can specify a "primary" field that is the union of all the deduped fields?

Oh actually I misread the table! It looks as if the deduplicated vectors PR is actually a bit faster?!

[msokolov]

How do you think this would combine with all the other vector formats, such as the quantizing ones? I guess we would want to avoid a lot of code copying ... I guess it should be possible to somehow wrap an existing format so we can independently innovate about quantization? Also .. do you see this as becoming the default flat vector format, or do you think users would select it in a custom codec?

[kaivalnp]

looks as if the deduplicated vectors PR is actually a bit faster?!

Yeah, this was surprising to me too -- it is ~10% faster, though I'm not sure why (there's one additional lookup of vector ord -> ord of position in data file which is stored on-heap as an int[]).

How do you think this would combine with all the other vector formats, such as the quantizing ones?

This is kind of tricky -- the main challenge is that quantization factors can depend on data distribution? (i.e. the same float[] vector may be quantized to a different byte[] for smaller graphs).

A slightly smaller challenge is that quantization formats today store the quantized byte[] vector followed by a float correction factor on disk -- and because both are read at the same time, this layout leads to fewer page cache misses.

For de-duplication to be effective, I guess we'd need to decouple the quantized byte[] vector from the float correction factor? This would lead to more page cache misses -- but FWIW the correction factors are just 4B per vector per field, and can be "hot" too (either explicitly: by keeping on-heap, or implicitly: like HNSW graph edges, they are an order of magnitude smaller).

do you see this as becoming the default flat vector format

IMO it could be the default if the overhead of de-duplication is "small" (personally: something like <10% slower indexing / merge sounds like it would be worth replacing the default).

Since it is currently not: perhaps a separate HNSW format backed by the de-duplicating format in a non-core module?

[kaivalnp]

Iterated with the AI to iron out some performance bottlenecks.

Disclaimer: There's generous use of AI involved in the code, so functions may be more verbose than required! I have reviewed most of it locally, and will do a refactor to try and make it cleaner / more human friendly soon.

Current Approach

See dedup-vectors-format-design.md in this PR for the AI-generated summary, but the flow is roughly:

1. During indexing, maintain a per-field List of vectors on-heap (same as the flat format).
2. During flush (occurs field-wise):
   1. Raw vectors across all fields are stored in a single "vector dictionary".
   2. Iterate over the List of vectors in a field, compute hash for each vector and store it in a map of hash -> (field number, ordinal in field).
   3. On hash collision, check for equality with the vector referenced in the map:
      1. IF vectors are equal, do not write a separate copy to the "vector dictionary" and point to the existing entry.
      2. ELSE use a hash probing strategy (move to hash + 1, then hash + 2, and so on).
   4. At the end, only unique vectors are stored in the dictionary.
   5. Each field stores an additional ord -> position in dictionary mapping (also see "Query Time Cost").
3. During merge (also occurs field-wise):
   1. Get a view of vectors being merged across segments, as an iterator of (docid, vector, original segment idx, ord in original segment).
   2. Compute hash of each vector, and maintain a map of hash -> (field number, original segment idx, ord in original segment).
   3. On hash collision, we have two paths:
      1. IF both vectors (the one being merged + the one that collided) come from the same segment, which is also de-duplicating: we re-use the vector equality that has been resolved earlier (i.e. whether both vectors point to the same location in the original segment's dictionary).
      2. ELSE load the other vector onto heap for an explicit equality check. This happens when:
         1. Hash collisions occur across segments, where equality has not been resolved earlier.
         2. When a normal flat format was used earlier, and we switched to the de-duplicating one now (e.g. during Lucene upgrade).
   4. The logic to write to the dictionary is the same as flush.

Indexing Time Complexity

1. One additional hash of each vector during indexing and merge (equivalent to one vector similarity computation done during HNSW?).
2. Adding into the hash map (constant on average, but expansion of map on reaching capacity can add overhead).
3. Equality checks for all duplicated vectors + hash-collisions.
   1. During indexing, these are all on-heap -- adding a fixed cost.
   2. During merging, this is not expensive when both vectors are present in the same segment (already resolved during indexing, which is re-used).
   3. However, cross-segment equality checks can become costly: loading vectors in a random-access fashion onto heap (page cache misses).
   4. Perhaps we can use a hash with more bits to reduce false positives, or only guarantee de-duplication within a document?

Query Time Cost

1. One additional lookup per vector operation:
   1. Earlier: vectors were stored contiguously, so the offset in the flat file was a direct computation of ord * vectorByteSize.
   2. Now, vectors across all fields are stored in a single file, with multiple ordinals possibly referencing the same vector (i.e. no longer 1:1).
   3. The offset in the "vector dictionary" is something like ordToPosition[ord] * vectorByteSize instead.
   4. Currently, ordToPosition is an array stored on-heap, perhaps this can be index-backed too?

All-in-all, it does not seem too expensive compared to the flat format?

[kaivalnp]

I observed a benchmark issue in the luceneutil KNN benchmark where force_merge(s) was noisy -- the indexing flow in that script is split into three phases:

1. initial indexing
2. wait for running merges to finish
3. force merge

The index(s) column reports (1); force_merge(s) reports (3); but (2) is untracked!
Merges are selected by Lucene, and one run can do more / less work than another -- leading to un-comparable force_merge(s)?

To work around this, I simply moved this line to after waitForMergesWithStatus to include both (1) and (2) in index(s).

Can we look at the sum of index(s) and force_merge(s) as a "total indexing time" proxy?

cc @mikemccand

---

Benchmarks

I ran a luceneutil KNN benchmark with Cohere v3 vectors, 1024d, DOT_PRODUCT similarity, 1M documents, 10K queries, no quantization.

To benchmark this PR, we need to replace the format here with DedupFlatVectorsFormat.

Note: Before the work-around listed above, index(s) was very close in main and this PR, but the untracked running merges were costlier in this PR, leading to artificially lower force_merge(s) (as visible below).

The numbers below do include the work-around that considers initial indexing + wait for running merges in index(s). For this benchmark I'm approximating index(s) + force_merge(s) as "total indexing time" for comparison.

main

recall  latency(ms)  netCPU  avgCpuCount  visited  index(s)  index_docs/s  force_merge(s)  index_size(MB)         filterStrategy  filterSelectivity
 0.973        6.065   6.058        0.999    16735    483.72       2067.31          178.37         4055.40  query-time-pre-filter               0.50
 0.968        5.805   5.804        1.000    16195    483.72       2067.31          178.37         4055.40  query-time-pre-filter               0.20
 0.971        6.120   6.119        1.000    12687    483.72       2067.31          178.37         4055.40  query-time-pre-filter               0.10
 0.988        3.103   3.101        0.999     8118    552.98       1808.40          392.23         6077.95      index-time-filter               0.50
 0.991        2.468   2.467        1.000     7412    485.77       2058.59          276.12         4862.36      index-time-filter               0.20
 0.993        2.218   2.217        1.000     6818    428.49       2333.77          281.16         4457.81      index-time-filter               0.10

This PR

recall  latency(ms)  netCPU  avgCpuCount  visited  index(s)  index_docs/s  force_merge(s)  index_size(MB)         filterStrategy  filterSelectivity
 0.973        6.240   6.238        1.000    16709    592.26       1688.44           80.70         4058.20  query-time-pre-filter               0.50
 0.968        6.094   6.092        1.000    16102    592.26       1688.44           80.70         4058.20  query-time-pre-filter               0.20
 0.971        6.369   6.367        1.000    12611    592.26       1688.44           80.70         4058.20  query-time-pre-filter               0.10
 0.988        2.815   2.811        0.999     8125    775.38       1289.70          239.68         4130.06      index-time-filter               0.50
 0.991        2.161   2.160        0.999     7413    441.66       2264.17          291.74         4085.13      index-time-filter               0.20
 0.993        2.417   2.416        1.000     6827    392.37       2548.60          315.08         4070.74      index-time-filter               0.10

Summary

1. Sanity check: recall is exactly the same on main and this PR.
2. latency(ms) is in the same range too: some runs are higher / lower, but overall appears to be in the range of HNSW noise?
3. As a recap of the options used:
   1. filterStrategy = query-time-pre-filter with filterSelectivity = F applies a filter during graph traversal that matches F proportion of docs. Note that the reported latency does NOT include time spent in BitSet creation for the filter; only reports graph search time.
   2. filterStrategy = index-time-filter (added in Add option for index-time filtering to knnPerfTest.py mikemccand/luceneutil#468) with filterSelectivity = F indexes the same docs (F proportion) into a separate vector field, which is then used at search time. There is no separate query-time cost, apart from the user resolving to the correct field based on the filter themselves.
   3. The difference b/w query-time-pre-filter and index-time-filter is ~2-3x, and demonstrates the tradeoffs nicely: more work during indexing to create a separate HNSW graph, for faster filtered search.
4. "Total indexing time" (index(s) + force_merge(s)) change:
   1. 662.09 s -> 672.96 s (+1.6%) for query-time-pre-filter (i.e. no explicit duplicate vector field).
   2. 945.21 s -> 1015.06 s (+7.4%) for index-time-filter with 50% docs in the smaller field.
   3. 761.89 s -> 733.4 s (-3.7%) for index-time-filter with 20% docs in the smaller field.
   4. 709.65 s -> 707.45 s (-0.3%) for index-time-filter with 10% docs in the smaller field.
   5. All-in-all, there is <10% difference with this PR (slower in most cases, occasionally faster, likely due to indexing non-determinism).
5. There is a sharp reduction in index_size(MB) with this PR as expected: only unique vectors are stored on disk (e.g. ~4GB index size without separate vector field -> ~6GB with 50% duplicates, which is reclaimed with this PR -- main additional disk usage is for the HNSW graph).

As a next step, I'll increase the duplication factor: up to TK fields, each containing a (different) proportion P of docs of the main field (to simulate a practical scenario of the proposal in #14758).

[github-actions]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

[kaivalnp]

Re-did the changes on top of main, benchmark details same as #15979 (comment) (except this one is on 500K docs).

Common:

Results:
NOTE: nDoc = 500000 for all runs; skipping column
NOTE: searchType = KNN for all runs; skipping column
NOTE: topK = 100 for all runs; skipping column
NOTE: fanout = 100 for all runs; skipping column
NOTE: resultSimilarity = N/A for all runs; skipping column
NOTE: decay = N/A for all runs; skipping column
NOTE: resultCount = 100.000 for all runs; skipping column
NOTE: maxConn = 64 for all runs; skipping column
NOTE: beamWidth = 250 for all runs; skipping column
NOTE: quantized = no for all runs; skipping column
NOTE: num_segments = 1 for all runs; skipping column
NOTE: overSample = 1.000 for all runs; skipping column
NOTE: vec_disk(MB) = 1953.125 for all runs; skipping column
NOTE: vec_RAM(MB) = 1953.125 for all runs; skipping column
NOTE: bp-reorder = false for all runs; skipping column
NOTE: indexType = HNSW for all runs; skipping column
NOTE: rerank = no for all runs; skipping column

main:

       filterStrategy  index(s)  index_docs/s  force_merge(s)  index_size(MB)  visited  filterSelectivity  recall  latency(ms)  netCPU  avgCpuCount
    index-time-filter    158.52       3154.18          139.27         2224.54     6183               0.10   0.995        1.952   1.951        0.999
    index-time-filter    137.30       3641.71          182.10         2426.55     6820               0.20   0.993        2.241   2.240        0.999
    index-time-filter    330.23       1514.09          102.27         3034.54     7604               0.50   0.990        2.502   2.501        1.000
query-time-pre-filter    161.97       3087.03          124.74         2024.70    10887               0.10   0.977        5.538   5.537        1.000
query-time-pre-filter    161.97       3087.03          124.74         2024.70    14837               0.20   0.976        5.590   5.589        1.000
query-time-pre-filter    161.97       3087.03          124.74         2024.70    15812               0.50   0.978        5.002   5.001        1.000

This PR:

       filterStrategy  index(s)  index_docs/s  force_merge(s)  index_size(MB)  visited  filterSelectivity  recall  latency(ms)  netCPU  avgCpuCount
    index-time-filter    155.55       3214.44          151.74         2031.48     6197               0.10   0.995        2.040   2.040        1.000
    index-time-filter    174.86       2859.50          151.46         2038.26     6828               0.20   0.993        2.396   2.395        1.000
    index-time-filter    390.92       1279.05           30.06         2059.98     7603               0.50   0.990        2.623   2.621        0.999
query-time-pre-filter    191.61       2609.48           96.57         2025.86    10932               0.10   0.977        5.587   5.586        1.000
query-time-pre-filter    191.61       2609.48           96.57         2025.86    14863               0.20   0.976        6.128   6.126        1.000
query-time-pre-filter    191.61       2609.48           96.57         2025.86    15827               0.50   0.978        4.561   4.560        1.000

[msokolov]

I started to look at this and left some comments, but it's an overwhelming amount of code to review and I have only been able to look at a small amount of it.

One overarching question I have is: how would we use this to support index-time filtering? For example, if I have ten filters I want to encode in the index: say "category (one of a,b,c,d,e)", "popularity (a numeric range filter with a few predetermined ranges)", and maybe some collection of boolean conditions. Then when I index documents, for each one I determine which filters it matches (possibly some filters may be boolean combinations of the above field-value combinations), and for each matching-condition, I maintain a mapping from that condition to a Lucene field, and then I index the document by adding fields with vectors in them for each of the matching conditions? And then I separately use the mapping I maintain (probably modeled as Lucene Query objects?) to field names so that at search time I can do some Query rewriting to replace a set of Query constraints + HNSW matching clause with an HNSW matching clause over a different field (one that incorporates the other constraints)? It just feels weird to explicitly index the same vector multiple times when what I really want is multiple filtered indexes over the same vector. At the same time, if we maintain an API like addVectorField(float[] vector, List filterLabels) then we are free to try out different ways of encoding the multiple filtered HNSW graphs. Do we want to represent as separate graphs? As labels on a single graph?

I think what I'm asking for here is some kind of syntactic sugar/API on top of all this to hide from the user the idea that we are generating multiple fields over the same data? I guess Lucene could maintain a map from "HNSW field f + label a -> HNSW field f_a"?

[msokolov]

what if in future we add other distinguishing factors? EG centered and rotated vs not centered and rotated?

[mikemccand]

Doesn't using this require that all fields that want to be dedup'd must share the same KnnVectorsWriter instance? In which case if there are variations like that (rotated, centered), all fields will do that the same way?

Edit: also, I realized, encoding here is the input type (byte[] or float[]), not how the format is encoding vectors on disk. Maybe clarify that this is input type (byte/float)? It might clear up this confusion.

[kaivalnp]

all fields that want to be dedup'd must share the same KnnVectorsWriter

This is correct -- and is currently left up to the user unfortunately :(
Any ideas to enforce this?

Maybe clarify that this is input type (byte/float)

Makes sense, I'll try to add this

[msokolov]

For me, pool has the connotation of a limited resource that can be reserved and returned. What do you think of "variety"?

[mikemccand]

How about organizes fields into groups with the same dimension and input type (byte/float)?

Edit: actually I think pools is OK here? It's used throughout ... and it grows on you (well, me at least) as I see it situ. It reminds me of what ZFS calls "pools" too.

[kaivalnp]

Makes sense, I'm thinking of "groups"

[msokolov]

what does "level A" mean?

[msokolov]

I think this explanation can be clearer. Are we saying we can avoid vector comparisons for multiple occurrences of the same vector within the same source segment, which we would otherwise (when the source segment is not deduped) have to perform, because we already did the deduping in the source segment, so with this opto, we only need to compare each unique vector with all the other vectors in other segments?

[mikemccand]

I think its optimizing the common case (merging N segments, all of which use this same format)?

In which case I would call it "Step 1" instead of "Level A", is to merge the pools, i.e. sourceVecOrd -> mergedVecOrd. Then "Step 2" is nicely just looking up the int -> int remap for the vector ordinals for each doc. Similar to how docid is rewritten during postings merge to map around deletions. Very clean/fast! And the laziness is clever to: we don't copy over a vector from the pool if it doesn't appear in the merged segment (none of the remaining live docs have it).

[kaivalnp]

Right! I'll try to make comments clearer..

[msokolov]

if the pools are ordered (and dense, right?) why do we need to write the ordinals?

[kaivalnp]

The ordinal here is the VectorEncoding and not the pool ordinal.

[msokolov]

what is meant by "(correctness)" here?

[mikemccand]

I think it's CC stating the purpose of each statement? Optimization vs functionally necessary? I asked my CC instance to review this PR and it said something hilarious:

AI-authored code is fluent but tends to over-document, over-defend, and sometimes encode invariants in comments rather than asserts. The "Level A" naming, the parenthetical "(perf win)" / "(correctness)" asides, the long discursive Javadocs about why caches exist — these are AI tells. Tighten before committing per Lucene convention (Mike: matches your "no dwelling on implementation details in javadocs" rule).

[msokolov]

Yeah, I mean I think at the very least we should be authoring the comments in human English, or some kind of human language anyway

[kaivalnp]

This is an artifact of iterating on the AI's original solution, which eagerly merged "pools" across segments, even if some vector was no longer "referenced" by a document -- leading to ever-growing indices.

I asked it to merge lazily, only copying bytes that are referenced -- and some of those inner workings leaked into the comments.

I'll update to avoid this^

[mikemccand]

I'm working through my slow human review process still, but I asked my local CC Opus 4.7 to review the PR. I haven't reviewed its review! Even THAT is time consuming! Though I see it leaks some of my "steering doc" preferences, heh.

Not sure I agree with all of it, e.g. the code dup across all Codec impls ... we need to be cautious there (changing the format of Codec B should not accidentally then change the format of Codec A).

Humans reviewing AI stuff has become the bottleneck now!

Review of PR #15979 — DedupFlatVectorsFormat

High-level concerns

1. Format versioning / class naming breaks Lucene convention. Lucene formats are named LuceneXXFooFormat and frozen on release; new versions live in new packages. DedupFlatVectorsFormat in codecs.dedup has no version prefix, so there's no migration path if the
   on-disk layout (.dvc/.dvm) ever changes. This is likely a blocker for committing as-is. Either name it Lucene10XDedupFlatVectorsFormat (or similar, matching whatever release it lands in) and place it under codecs/lucene10X/, or land it in the sandbox module rather
   than core while still experimental.

2. ~3.6K added LOC duplicates a lot of Lucene99FlatVectorsFormat. OffHeapDedup{Float,Byte}VectorValues reimplements the entire Dense/Sparse/Empty hierarchy that already exists in OffHeap{Float,Byte}VectorValues. Reviewing, maintaining, and porting future
   improvements doubles this surface. Before committing, please look at whether the dedup variants can either subclass the existing off-heap classes or compose them by holding an int[] docOrdToVecOrd + a KnnVectorValues pool view. The Sparse/Dense/Empty/PoolView
   boilerplate is mostly mechanical in both files.

3. Cross-format generality. This is a wrapper over Lucene99HnswVectorsWriter/Reader specifically. If we add a new HNSW format (Lucene10X), do we add DedupHnswVectorsFormatLucene10X? The flat format is reusable, so this works for the flat case, but the HNSW wrapper
   doesn't compose nicely. Worth thinking about how it interacts with quantized formats (msokolov already asked) before we commit to the API surface.

4. Missing CHANGES.txt entry. New public format requires one.

5. PR description says "Details about the format are included in a markdown doc in the PR" — I don't see any .md file in the diff. Either it was not pushed, or the description should be updated. The format docs in the format Javadoc are decent but a design doc
   justifying the cross-field-pool design (vs. e.g. a single shared field) would help reviewers.

Correctness

6. MergePool.intern byte-verify allocates ~byteSize per probe (readSourceBytesdoesnew byte[byteSize]). For 1024-dim FLOAT32 = 4KB per call. Hash collisions are rare, but probe chains over many same-hash entries on hash-collision aren't, and probing past
   non-empty unrelated slots also needs no allocation. Reuse a single scratch byte[] held on the MergePool.

7. MergePool.intern calls vectorValue(refSourceOrd[candidateOrd]) on a KnnVectorValues instance that may be the same one currently being iterated by the merge (because srcValues is forwarded straight from the merger). The contract is that vectorValue(int) is
   random-access, but the implementations reuse a value buffer and a lastVecOrd cache. Confirm by inspection that no caller retains a returned vector across an intermediate vectorValue call on the same instance — mergeFloatField/mergeByteField do
   System.arraycopy/bb.put immediately, so this looks fine, but please add a comment that documents the invariant. A single misuse here would silently corrupt the dedup pool.

8. DedupHash substitute-for-zero (0xDEDDEADBEEFCAFE0L) is itself a legitimate murmur output. A vector that genuinely hashes to that substitute will collide-probe alongside the (vanishingly rare) zero-hashing ones. Behavior is correct (byte-verify catches it), so
   this is more of a "be honest in the comment" — the substitute doesn't avoid collisions, it just remaps a bias. The text "itself can never match a legitimate murmur output for a different vector with non-trivial probability" overstates this.

9. Float-equality verify in FlushPool.internFloats (line 1145): comment says Arrays.equals(float[]...) is OK because NaN/0.0 cases "would yield different bytes → different hash → different slot." That's true for the first stored candidate, but the verify runs
   against an existing vectorFloats.get(candidateOrd) whose bytes may have hash-collided with bytesCandidate. If the colliding stored vector and the candidate have bytes differing only by +0.0/-0.0 patterns... they have different bytes, different hashes, different
   slots — so they wouldn't collide at all. So the comment's conclusion holds, but the reasoning needs reordering. Cleaner: just compare bytes (you have them), and drop the parallel vectorFloats byte-equality logic. The float[] cache is for read-back, not
   verification.

10. vecOrdScratch = new int[ArrayUtil.oversize(numNodes, Integer.BYTES)] in TranslatingScorer/TranslatingUpdateableScorer: the bytesPerElement arg to oversize is per-element bytes, so 4 is correct for int[]. Just confirming since the name Integer.BYTES is
    misleading-looking next to numNodes.

Test code

11. TestDedupFlatVectorsFormat.poolUniqueCount reflects through three layers (PerFieldKnnVectorsFormat$FieldsReader, Asserting...$delegate|in, Lucene99HnswVectorsReader.flatVectorsReader). This is brittle — any rename in the wrapping chain silently breaks tests
    that aren't testing what they look like they're testing. Add a package-private accessor on the relevant readers and unwrap via that, or expose a stat through FieldInfos/ramBytesUsed.

12. for (boolean u : used) if (u) usedCount++; (line 3516) — one-liner. Lucene style-wise OK, but readable equivalent would be cleaner.

Smaller items

• MergePool.srcInternCache is Map<Integer, int[]> with autoboxing. The reader uses IntObjectHashMap — same here would avoid boxing on every fast-path call.
• static record PoolKey — static is redundant on a nested record (always implicitly static).
• getMaxDimensions returns hardcoded 1024 with no comment why; Lucene99 returns 1024 too, so this is consistent, but worth a @inheritdoc or one-line note.
• if (vals == null) continue; (writer L878, L933) — fine, but Lucene tends to brace one-liners.
• assert cached.length == srcUniqueCount in srcVecToMergedMap: good defensive check — but its message references "readerIdx mismatch" which is wrong; the failure means the source pool size changed between calls, which shouldn't happen.

Author's AI disclosure

The author was upfront that this is largely AI-authored. Reviewers should weight that toward reading every code path manually — AI-authored code is fluent but tends to over-document, over-defend, and sometimes encode invariants in comments rather than asserts. The
"Level A" naming, the parenthetical "(perf win)" / "(correctness)" asides, the long discursive Javadocs about why caches exist — these are AI tells. Tighten before committing per Lucene convention (Mike: matches your "no dwelling on implementation details in
javadocs" rule).

Verdict

Direction is good and benchmark numbers (10% faster on the 1M Cohere benchmark) are encouraging. Not ready to commit as-is — needs (1) format versioning/naming aligned to Lucene convention, (2) significant deduplication against existing OffHeap*VectorValues, (3)
CHANGES entry and the missing design doc, (4) the brittle reflective test plumbing fixed.

[mikemccand]

I think what I'm asking for here is some kind of syntactic sugar/API on top of all this to hide from the user the idea that we are generating multiple fields over the same data? I guess Lucene could maintain a map from "HNSW field f + label a -> HNSW field f_a"?

I agree the current scope isn't perfect, but it's super hard even just to do this first step (the low level sharing), which it looks like is quite clean here (but I'm not done reviewing!).

I'd love to see such sugar, and somehow support for BooleanQuery-like joining of these separate fields at search time, but let's defer that to later phases? Progress not perfection?

Does the dedup'ing optimize the == case maybe? (Just for flushing). So if I index 100 different fields, all with precisely the same float[]. I guess this may be tricky -- we may always make a copy today. We can optimize that later.

[mikemccand]

To work around this, I simply moved this line to after waitForMergesWithStatus to include both (1) and (2) in index(s).

Can we look at the sum of index(s) and force_merge(s) as a "total indexing time" proxy?

Oh no, I missed this first time around. Yeah this is difficult -- I'd rather keep "indexing time" as it is (after the final commit) because one could rollback at that point (presumably quickly) and have an accurate/intact index.

Can we add 2nd time measure, which is time to wait for the in-flight merges?

And, I would rather not pay much attention to either merge time in our comparisons here -- merge timing is exceptionally noisy, and has discrete step changes (a force-merge that needs two rounds vs three rounds for example).

The only way to compare merge time would be single thread running force merge on same geometry index.

Maybe open upstream (luceneutil) issue about this @kaivalnp? Sorry the the slow response!

[mikemccand]

Publishing what I have so far! Still working on it! PnP, even on one review of a PR!

Thanks @kaivalnp -- this is a big change, and will be very needly moving (speed and recall) for Lucene users needing accurate filtered vector search when that filter is known (independent of Query) at indexing time! And it looks like you did at least some polish over the raw AI hot take, thanks.

[mikemccand]

Hmm, is encoding the String SPI identifier, or, a specific instance of one?

[mikemccand]

Oh! This is VectorEncoding -- byte[] vs float[] input vectors, right? Cool, so this works for both!

[mikemccand]

How about organizes fields into groups with the same dimension and input type (byte/float)?

Edit: actually I think pools is OK here? It's used throughout ... and it grows on you (well, me at least) as I see it situ. It reminds me of what ZFS calls "pools" too.

[mikemccand]

Does this format implement prefetch? Maybe add // TODO and/or open followon once we merge? Since this format could "amplify" the randomness (the deref to original ordinal), prefetch is maybe even more important than the non-dup-detecting formats.

[kaivalnp]

Yes -- this PR does implement prefetch for vectors; the API asks for vector ordinals which need to be translated to ordinals on disk.

[kaivalnp]

Although I didn't get why randomness is amplified here?

[mikemccand]

Hmm we also allocate these in the flush case? They just remain empty? Maybe we need to / should factor out into a dedicated merging utility class? Not sure.

[kaivalnp]

Yeah, maybe we can defer the initialization to the first call? (both merge + flush)
I'll try to factor into a separate class for code cleanliness too.

[mikemccand]

I'm confused why we need two layers here (docOrd)? Some docs might not have a vector, so first layer (today, before this change) is docid -> vectorOrd, right? But with this change, it seems like it should still be that? Or is docOrd maybe a synonym for docid? I'm confused :)

[kaivalnp]

Inside the vector classes, vectors are identified using packed ordinals (i.e. docOrd) -- which point to monotonically increasing docids (mapped using ordToDoc, which is a OrdToDocDISIReaderConfiguration)

In the plain format, the mapping b/w ordinals -> vectors on disk is 1:1, so we don't need any additional mapping (i.e. a vector on disk was addressable as docOrd * vectorByteSize).

In the dedup format, multiple vectors can point to the same region on disk, so we need an additional docOrd -> vectorOrd mapping. Also, this is not monotonic, so I didn't use the same OrdToDocDISIReaderConfiguration (instead going with DirectWriter)

[mikemccand]

I think it's CC stating the purpose of each statement? Optimization vs functionally necessary? I asked my CC instance to review this PR and it said something hilarious:

AI-authored code is fluent but tends to over-document, over-defend, and sometimes encode invariants in comments rather than asserts. The "Level A" naming, the parenthetical "(perf win)" / "(correctness)" asides, the long discursive Javadocs about why caches exist — these are AI tells. Tighten before committing per Lucene convention (Mike: matches your "no dwelling on implementation details in javadocs" rule).

[mikemccand]

Actually I think this should be considered a correctness issue too, i.e., all dups should be detected and shared? We should never see two different vecOrd pointing to what are actually identical vectors (false negative)? Hopefully we can assert to that effect, and let's e.g. fix tests to exercise this: intentionally create many dup vectors across segments, and then confirm on force-merge we have exactly the correct number.

[mikemccand]

And, mix in some deletes too!

[kaivalnp]

Makes sense, I'll try to add this^

[github-actions]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

[kaivalnp]

Apologies for the delay, getting back on this PR now!

Thanks @msokolov

overwhelming amount of code to review

Sorry, I think the AI was too verbose in the code + comments, I re-wrote this PR from scratch as a human, only relying on AI for tests + some documentation + finding errors -- I hope the next iteration is clearer :)

some kind of syntactic sugar/API on top of all this to hide from the user the idea that we are generating multiple fields over the same data?

This makes sense to me -- it is ultimately what we're hoping to do with this change!

For this first pass I wanted to add the actual de-duplicating format + make no API changes + test performance of the format more widely -- can we open an issue to add this sugar when the change has baked for a while? (this change also needs a corresponding format for quantized vectors)

Thanks @mikemccand (and AI)

Does the dedup'ing optimize the == case maybe?

It does not -- the current entry point for incoming vectors is here, which is per-field, and there is no explicit guarantee that all fields for a doc will be indexed in sequence, before moving on to the next doc (perhaps the recently added columnar indexing API exercises this).

Moreover, there is an expectation that the same array can be re-used to pass vectors (since the writer has to own its copy). For now, I've relied on full equality checks for de-duping.

Can we add 2nd time measure, which is time to wait for the in-flight merges?

This was added in mikemccand/luceneutil#587

I would rather not pay much attention to either merge time in our comparisons here -- merge timing is exceptionally noisy, and has discrete step changes (a force-merge that needs two rounds vs three rounds for example)

Makes sense, I'll also report single-threaded indexing numbers from now!

The merge path is important to measure for this format because it can involve random IO for de-duplication against a previously written vector, which won't be captured in the indexing path alone.

I think it's CC stating the purpose of each statement? Optimization vs functionally necessary?

Yes, there were a few context leaks about my iterations with the AI :)

[kaivalnp]

Benchmarks

Single-threaded indexing

Common

Results:
NOTE: nDoc = 100000 for all runs; skipping column
NOTE: topK = 100 for all runs; skipping column
NOTE: fanout = 100 for all runs; skipping column
NOTE: maxConn = 64 for all runs; skipping column
NOTE: beamWidth = 250 for all runs; skipping column
NOTE: quantized = no for all runs; skipping column
NOTE: num_segments = 1 for all runs; skipping column
NOTE: filterSelectivity = 0.50 for all runs; skipping column

main

visited  index(s)  index_docs/s  index_size(MB)         filterStrategy  recall  latency(ms)  netCPU  avgCpuCount
   6184    520.32        192.19          603.01      index-time-filter   0.995        2.069   2.068        0.999
  12877    362.57        275.81          402.98  query-time-pre-filter   0.989        4.417   4.416        1.000

This PR

visited  index(s)  index_docs/s  index_size(MB)         filterStrategy  recall  latency(ms)  netCPU
   6184    531.66        188.09          408.98      index-time-filter   0.995        2.174   2.173
  12877    359.93        277.83          403.36  query-time-pre-filter   0.989        4.478   4.477

Multi-threaded indexing with force-merge

(same Common values)

main

visited  index(s)  index_docs/s  merge(s)  force_merge(s)  index_size(MB)         filterStrategy  recall  latency(ms)  netCPU
   6196     22.27       4490.14     21.52           36.74          603.02      index-time-filter   0.995        1.942   1.941
  12944     18.91       5289.61     19.02           19.52          403.01  query-time-pre-filter   0.989        4.343   4.343

This PR

visited  index(s)  index_docs/s  force_merge(s)  index_size(MB)         filterStrategy  recall  latency(ms)  netCPU
   6199     38.90       2570.83           44.97          409.01      index-time-filter   0.995        2.034   2.033
  12883     29.90       3344.71           31.78          403.37  query-time-pre-filter   0.989        4.352   4.351

Notes

• The Single-threaded indexing run is slightly faster for the de-duplicating format in case of no explicit duplicates, and <3% slower with 50% explicit duplicates (i.e. a second field having a subset of 50% docs of the main one), which appears to be in the range of HNSW noise.
• The Multi-threaded indexing with force-merge run is not directly comparable, because the auto-started merges were different (as @mikemccand mentioned). That said, the sum of index(s) + merge(s) + force_merge(s) is ~4% slower for no explicit duplicates, and ~7% slower with 50% explicit duplicates.
• The query latency was consistently slower for the de-duplicating vector format across multiple runs, but this was <5% slower. One point to note: the ordToVecOrd indirection (applicable before scoring each vector) is currently off-heap, and can likely be sped-up by maintaining an on-heap int[] map. I'm hoping that the memory residency of the off-heap map will be in-line with the HNSW graph.
• Note the drop in index_size(MB) -- the index size is about the same with 50% explicit duplicates and without them (the difference being the second HNSW graph).
• These benchmarks also highlight the importance of indexing a separate field for filters known at indexing time (the index-time-filter option is >2x faster than query-time-pre-filter, not even including overheads of BitSet creation and maintenance).