Prevent writing un-quantised vectors twice during merging HNSW graphs#15732
Prevent writing un-quantised vectors twice during merging HNSW graphs#15732iverase wants to merge 1 commit intoapache:mainfrom
Conversation
This is only necessary because we do back-linking during HNSW merging. So, maybe if we didn't have to do the backlinking (e.g. #15504). |
|
Thanks for addressing this; it was always a bit silly to write this data twice. But I'm confused why there is a need for a two-phase implementation with the added complexity of deferred runnables. Hmm is it because we might pack the vectors for multiple fields into a single file, so we must merge all fields' flat vector data before we can build any field's index (graph) from it? |
That's correct. We need to first write all the flat vector data. |
|
OK, I understand it better now, thanks. Did you consider restructuring |
I thought about that but decided against it because I did not what to give specific names to the phases (maybe in a future there is no flat vectors but something else? or in some cases we are doing flat and index in the same phase). Another benefit of this approach is that it is easy to share information between the two phases (e.g. FieldInfo). But I don't feel strong about this, if we think is better to expose it via two methods, it can be done. |
|
I guess the flip side is now we have added the ability to defer some unspecified work to a second phase, but maybe we will not need it in the future? I guess it's a matter of opinion, but for me, I'd like to avoid abstractions we don't absolutely need. Also, adding the abstraction opens up questions like what if we have more than two phases (in the future)? Like we want to first compute centroids, then merge the flat vectors, then build some indexing structures, I don't know; in that case we should have a an IOCallable that returns an IOCallable so we can chain them? Obviously it's overkill now. Anyway if you wouldn't mind trying the concrete implementation I think it will be easier to understand. If it looks bad to you, OK, this way works and I won't block it. |
This PR is just addressing the following suggestion from Adrien when merging #601 (comment). This version only supports un-quantised vectors because for quantise vectors the logic is a bit more complex (it depends if the quantisation is symmetric or not) so I leave it as a follow up.
The key aspects of this implementation are:
1.- KnnVectorsWriter#mergeOneField returns now an IORunnable. The merge is done in two steps, first we call this method for the fields collecting the IORunnables. After calling of the fields we iterate over the runnables and execute them.
2.- Introduce a new interface QuantizedVectorsWriter to signal if the field requires quantised vectors for merging. We move the method #mergeOneFieldToIndex from FlatVectorsWriter to this new interface.