Add a de-duplicating flat vector format - #15979
Conversation
|
Important:
This PR 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 Indexing is slightly slower (<5%) + merges are non-trivially slower (up to 33%, currently looking into speeding this up). |
|
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?! |
|
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? |
Yeah, this was surprising to me too -- it is ~10% faster, though I'm not sure why (there's one additional lookup of
This is kind of tricky -- the main challenge is that quantization factors can depend on data distribution? (i.e. the same A slightly smaller challenge is that quantization formats today store the quantized For de-duplication to be effective, I guess we'd need to decouple the quantized
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? |
|
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 ApproachSee
Indexing Time Complexity
Query Time Cost
All-in-all, it does not seem too expensive compared to the flat format? |
|
I observed a benchmark issue in the
The To work around this, I simply moved this line to after Can we look at the sum of cc @mikemccand BenchmarksI ran a To benchmark this PR, we need to replace the format here with Note: Before the work-around listed above, The numbers below do include the work-around that considers initial indexing + wait for running merges in
This PR Summary
As a next step, I'll increase the duplication factor: up to TK fields, each containing a (different) proportion |
|
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! |
078e915 to
6b6b12d
Compare
|
Re-did the changes on top of Common:
This PR: |
There was a problem hiding this comment.
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"?
|
|
||
| /** | ||
| * Flat vector format that stores each unique vector exactly once per segment. Multiple documents | ||
| * (within the same field, and across fields with matching {@code (dimension, encoding)}) may share |
There was a problem hiding this comment.
what if in future we add other distinguishing factors? EG centered and rotated vs not centered and rotated?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
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!
|
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 Does the dedup'ing optimize the |
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 ( |
mikemccand
left a comment
There was a problem hiding this comment.
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.
|
|
||
| public DedupFlatVectorsReader(SegmentReadState state, FlatVectorsScorer scorer) | ||
| throws IOException { | ||
| this(state, scorer, DataAccessHint.RANDOM); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes -- this PR does implement prefetch for vectors; the API asks for vector ordinals which need to be translated to ordinals on disk.
There was a problem hiding this comment.
Although I didn't get why randomness is amplified here?
There was a problem hiding this comment.
I just thought the added indirection though a "group" means more (and non-monotonic) randomness?
Before if I iterate a bunch of hits in docid order and pull vector (say for re-ranking) it was monotonic seeking to load the vector bytes?
But with a group deref, the seeking will not be monotonic necessarily since my vector could've been seen by prior fields / documents.
There was a problem hiding this comment.
Ah right, monotonic docid traversal happens in the exact search path -- although I don't think we explicitly advise the kernel with a sequential hint on the vectors (because the same file may be concurrently used for random-access HNSW traversal from other searches).
The benefit in the original format is that vectors matching the filter + present close to each other on disk may be retrieved from disk in the same load? (but this would be highly dependent on the dimension of vectors, filter, and index sort)
I guess this implicit benefit can be ported to the de-duplicating format if we lay out vectors on disk in the same index sort order? Although this has its own caveats around choosing which doc to "primarily" attribute the vector to, for sorting purposes.
Perhaps radical change: if we only de-dupe a vector within a document, it simplifies some things for us..
- Since each vector is associated to exactly one document, the raw file can be deterministically sorted by index sort.
- Merging of already de-duped segments becomes much cheaper, because there is no read-back of vectors from disk on hash collisions across documents (for the full equality check).
- The added cost is the per-document tracking at flush time + lower overall de-duplication factor.
I think this can be a reasonable option, but needs some work to implement. Should we add a TODO to evaluate de-duping only within a document?
| int maxVecOrd = Math.max(0, pool.uniqueCount() - 1); | ||
| bitsPerVecOrd = DirectWriter.bitsRequired(Math.max(1, maxVecOrd)); | ||
| DirectWriter writer = DirectWriter.getInstance(vectorData, cardinality, bitsPerVecOrd); | ||
| for (int v : docOrdToVecOrd) { |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Does this explanation exist somewhere in the new new KnnVectors``Format or Reader or Writer? Let's add it? This high level doc / ord / group mapping is critical! Thanks.
There was a problem hiding this comment.
There's some information in this comment, let me know if you think it should be more explicit..
|
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! |
|
Apologies for the delay, getting back on this PR now! Thanks @msokolov
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 :)
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)
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.
This was added in mikemccand/luceneutil#587
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.
Yes, there were a few context leaks about my iterations with the AI :) |
6b6b12d to
23ed23a
Compare
BenchmarksSingle-threaded indexingCommon
This PR Multi-threaded indexing with force-merge(same Common values)
This PR Notes
|
msokolov
left a comment
There was a problem hiding this comment.
I think it's almost ready, although there seem to be some merge conflicts to resolve
| @Ignore | ||
| public void testWriterRamEstimate() {} | ||
| @SuppressWarnings("unchecked") | ||
| public void testWriterRamEstimate() throws IOException { |
There was a problem hiding this comment.
thanks for the nice test!
|
Thanks for the review @msokolov! Also added float16 support for the format after #16383 was recently merged. One broad question: this new de-duplicating format is currently marked "experimental" until tested more widely, and we can look into making it the default (in the future) if performance is comparable to the flat format. Until then, do you think we should move the classes to another module like |
Once this is merged, let's expose simple option in luceneutil to use it (not default yet)? |
|
Maybe we should finish a data blind (I can't help but be reminded of "blind dates", something totally different!!) quantization before trying to get dedup working with quantization? I.e. always rotate indexed and searched vectors by "deterministic random" Hadamard matrix (#16092, also implemented in luceneutil now). After that we should implement data blind quantization (eventually making it default vector format if it performs well) which doesn't look at the incoming vectors to compute quantization, and then (I think?) wouldn't need per-vector correction factors. At that point it's just the quantized bits stored for each vector, and then dedup can just key off of that instead of the input full precision vectors? |
mikemccand
left a comment
There was a problem hiding this comment.
This is looking great... I left a bunch of head scratching comments and polish, nothing major, really just me trying to understand! I ran out of time this AM so I'm publishing what i have so far. Thank you for persisting on such an important change @kaivalnp ... this will make filtered vector recall for highly selective filters especially, actually work with great recall/performance.
| import org.apache.lucene.internal.hppc.ObjectCursor; | ||
| import org.apache.lucene.util.Accountable; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Can you open a spinoff to somehow expose stats of how many duplicate vectors across fields? Maybe just during CheckIndex, or maybe an experimental public API to retrieve stats. This can be important information when figuring out how to optimize / query plan high recall filtered vector search, but also for detecting unexpected heavy duplication within vectors you thought were unique.
Edit: one simple stat/approximation (in lieu of/until finishing that spinoff) would be: get the total disk usage for the flat reader, divide by sum of total vectors counts for each dedup'd field, and you'll get a coarse "dedup factor" showing how much storage reduction you saw, i.e. on average how many times is each vector dup'd.
There was a problem hiding this comment.
Makes sense -- I don't see vector format specific information in CheckIndex, perhaps we can make this method public from the de-duplicating HNSW format?
There was a problem hiding this comment.
, perhaps we can make this method
publicfrom the de-duplicating HNSW format?
+1, but we should waiting until / when / if this format is in core module. CheckIndex could then do instanceof + cast check and print more details. Hmm but is that method field specific? Or its for all groups across all fields that share this dedup instance?
Beyond printing useful stats, we should also fix CheckIndex to validate any integrity beyond what its current vector checks exercise. E.g. if there is an API that says how many unique vectors this field has maybe Checkindex could count as it iterates vectors for that field and see if they agree, or so. But all of this dreamy stuff is for later!
|
|
||
| /** | ||
| * Map for vector hash -> group ord. Only a hint and not the ground truth due to possibility of | ||
| * hash collisions, where a full equality check must be performed. |
There was a problem hiding this comment.
I'm curious how often you hit false hash collisions (same 64 bit hash but different vectors) in a "typical" data set. Hopefully we have a good hash function vs "typical" vectors encountered, but of course for any hash function there are by mathematical / set theory necessity many adversarial vector corpora that have high false collisions (just hopefully rare in practice).
There was a problem hiding this comment.
how often you hit false hash collisions
I don't have this handy -- but it seems measurable in offline runs with local changes..
A good hash function is indeed important because it reduces unnecessary equals checks during flush, and unnecessary index IO + equals checks during merge.
In earlier local iterations, I kept the hash function as an int and used Arrays.hashCode for a simple start -- which was producing a measurably slower indexing time (by a few %) compared to the current long hash using murmurhash3 on byte representations (even with the overhead of converting float[] -> byte[] for computing the hash).
I'm not aware of a better hash function for float[] and float16[] vectors, and ideally one where conversion to byte[] is not necessary.
I also found the interesting Birthday Problem to be relevant here -- where hash collisions become a near guarantee with a large number of vectors, even with a "good" hashing function.
There was a problem hiding this comment.
Cool, I hadn't realized the birthday paradox applies to hash tables, but it clearly does! Only 23 people in a 365.2422 sized hash table and you have a 50% chance of a "collision" (two people with same birthday). So that is the best-case for hash functions (when there is truly no correlation b/w hash function and input vectors, so the hash locations are effectively random-like).
And yet we size our hash table arrays for X (75? 50?) % occupancy before growing, far higher than 23 / 365.2422 = ~6.297% occupancy. But that's OK, the 23 people figure just means when you did those 23 inserts, did any of them hit a false collision (and need linear probing). Seen more positively, there is a 50% chance that those 23 inserts had no conflict...
| this.current = new ObjectCursor<>(); | ||
| } | ||
|
|
||
| abstract long hash(T vector) throws IOException; |
There was a problem hiding this comment.
Do we have a test case for the worst case "heat death of the universe" corpus (many documents AND fields all have the same "all 0s" vector)? Every insert is then a (true) hash collision..
There was a problem hiding this comment.
We don't have a test case right now -- are you suggesting a @Monster test, or an offline run?
I'm guessing the HNSW graph construction will still be the bottleneck here, but I haven't tried this^
There was a problem hiding this comment.
How about a unit test for semi-realistic case where say each vector is duplicated 20X ish times? I just want to exercise the many duplicates case, maybe not the precise heat death "all duplicates" case. I think that's a separate spinoff -- maybe we already have test cases for heat death (since it struck for real in the wild, longish ago)?
There was a problem hiding this comment.
semi-realistic case where say each vector is duplicated 20X ish times?
Added now!
|
|
||
| @Override | ||
| public float score(int node) throws IOException { | ||
| return groupView.score(ordToVecOrd.get(node)); |
There was a problem hiding this comment.
Is ordToVecOrd mapping this fields vector ordinal (same as docid if all docs have a vector) to group ord? Maybe rename to ordToGroupOrd? Or maybe vecOrdToGroupOrd? Or fieldVecOrdToGroupOrd? Naming is the hardest part! But seriously the two levels of ord is throwing me for a loop. Do you spell out these two layers somewhere?
There was a problem hiding this comment.
Makes sense, I'm thinking of fieldOrdToGroupOrd
Do you spell out these two layers somewhere?
Some info can be found in #15979 (comment)
There was a problem hiding this comment.
LOL thanks, links back to my question / your answer ... history is repeating itself. I am stuck in a loop now.
|
There was a build failure with a recent commit, and I think the issue was present earlier -- it just surfaced with a particular seed value: This test indexes the same two vectors multiple times, and reads them back at the end. However, it assumes that the ordinal in the vector values is the same as the original ordinal of insertion, which is not necessarily true (Lucene's random test suite can re-order documents). As a fix, I started storing the original ordinal in an |
|
I moved the de-duplicating format to the Also threw Claude Opus 5 at the change for fun, and it found some interesting bugs!
IMO the change looks more polished / complete now, would appreciate a final review! |
# Conflicts: # lucene/CHANGES.txt
|
Phew, this has become one of those "takes 7 seconds for GH to render just the first page" let alone all the comments it hides by default... |
mikemccand
left a comment
There was a problem hiding this comment.
This change looks awesome -- my comments are polish now. +1 to merge into sandbox and address the nice-to-haves as spinoffs. I hope we can eventually promote this to core and make it the default out-of-the-box.
This new feature is vital for high-recall high-efficiency filtered vector search, especially for restrictive filters where Lucene otherwise has no good options today (ACORN-1 won't cut it below a certain filter selectivity, and, especially when the filter is orthogonal to vector similarity), and post filtering is typically terrible recall.
Beyond that "fresh feature", this will also be surprisingly helpful for users who accidentally or knowingly are indexing duplicate vectors. I suppose a simple test for users is to index before (no dedup) and after (dedup), force merge, and see if the size dropped for their index (in lieu of having CheckIndex give some visibility).
Vectors are so hard because they are so opaque to human eyes, and, because they can quite easily have all sorts of undetected problems so you are "flying blind" (look at all the unique vector smells luceneutil now detects! Not on unit sphere, non-isotropic, useless dimensions (e.g., same constant value), many duplicates (this fix, yay!), highly curved manifold (whatever THAT is), low intrinsic dims!!).
Thank you @kaivalnp for persisting so long on such a big / important change.
# Conflicts: # lucene/CHANGES.txt
|
Thank you for the reviews @msokolov @mikemccand! Unless there are further comments, I plan to merge this in a few days (and open an issue for the follow-ups discussed).. |
|
I opened #16493 for backport to 10.x |
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.