Extract the hnsw graph merging from being part of the vector writer

[benwtrent]

While working on the quantization codec & thinking about how merging will evolve, it became clearer that having merging attached directly to the vector writer is weird.

I extracted it out to its own class and removed the "initializedNodes" logic from the base class builder.

Also, there was on other refactoring around grabbing sorted nodes from the neighbor iterator, I just moved that static method so its not attached to the writer (as all bwc writers need it and all future HNSW writers will as well).

[zhaih]

Overall I think this refactor is great, will take another closer look later

[benwtrent]

@zhaih I updated the API a bit. This is more like I was thinking. Having a builder that accepts readers, doc maps, etc. And then can build with the final merge state.

[zhaih]

I wonder whether it'll be better to allow HnswGraphBuilder to accept a OnHeapHnswGraph as a constructor parameter and then when we init from graph we will use this InitializedHnswGraphBuilder to build a graph and then pass the built graph and a node filter (to avoid reindex the same nodes) to the normal HnswGraphBuilder?

Then for example if we want to have a multi-thread ConcurrentHnswGraphBuilder we can still use this InitializedHnswGraphBuilder to build the init graph and pass it to the ConcurentHnswGraphBuilder?

I mentioned this because in my draft concurrent HNSW merge PR #12660 I do need to pass the HNSW graph to a builder per thread, altho it can be done in various ways. But I still feel using parent/child class to separate this can make things a little bit hard later? Like, if I want a concurrent builder will the concurrent builder extend from this class? If so we need to be quite careful not to inherit a wrong behavior from the original HnswGraphBuilder and things can become quite complex?

[benwtrent]

@zhaih I think we can do that. What I also want to do is remove all this Map<Integer, Integer> oldToNewOrdinalMap. The caller should handle all that, not the constructor, especially since there is probably a way faster and simpler way to do it.

[msokolov]

I didn't follow this closely, but do we really have a map like that? If so, let's at least switch to IntIntHashMap to avoid boxing (if we're unable to remove it)

[benwtrent]

@msokolov I think we can just have int[], where int[oldOrd]=newOrd since old vector ordinals are continuous.

[benwtrent]

Just being paranoid, I tested and verified that recall is absolutely unchanged between these changes.

baseline:

0.500	 0.10	100000	10	4	50	20	11496	1.00	post-filter
0.533	 0.10	100000	10	4	100	20	18504	1.00	post-filter
0.844	 0.21	100000	10	16	50	20	22211	1.00	post-filter
0.875	 0.24	100000	10	16	100	20	44031	1.00	post-filter

candidate:

0.500	 0.10	100000	10	4	50	20	11778	1.00	post-filter
0.533	 0.10	100000	10	4	100	20	18439	1.00	post-filter
0.844	 0.20	100000	10	16	50	20	24012	1.00	post-filter
0.875	 0.25	100000	10	16	100	20	46249	1.00	post-filter

The performance isn't reliable, this was running on my laptop while it was doing lots of other work.

[benwtrent]

I think this was to simulate sparse vectors. But the sparse vector iterator never returns null for the vectors. Instead it just skips to the next non-null vector from what I can tell.

[zhaih]

This can be wrong when index sort is configured? E.g. if we have 2 segments each have 2 docs

seg0
    doc0: rank=0
    doc1: rank=2
seg1
    doc0: rank=1
    doc1: rank=3

So after merge those doc will be in an interleaved order but not simply old order + base?

[benwtrent]

You are correct! I will revert back to the integer map.

[benwtrent]

@zhaih I switched to an int[] where the indices are the old ordinals (since these are contiguous values as we don't allow deleted docs...for now).

The values in the indices are the newOrdinal calculated in a similar loop as before with the Map<Integer, Integer>

[zhaih]

Should we put it in util/hnsw package instead?

[benwtrent]

++

This would make sense for a ConcurrentMerger & "BetterFutureHnswGraphMerger" or whatever it will be.

[zhaih]

LGTM, only some minor nits