Restore HNSW graph reuse during merges under per-field vectors formats#16403
Restore HNSW graph reuse during merges under per-field vectors formats#16403jeho-rpls wants to merge 2 commits into
Conversation
Since 10.4.0 IncrementalHnswGraphMerger.addReader unwrapped the per-field reader into currKnnVectorsReader but tested the wrapper for HnswGraphProvider, which PerFieldKnnVectorsFormat.FieldsReader does not implement on this branch. Every candidate segment was rejected, so no seed graph was ever selected and every merge rebuilt the merged graph from scratch. Test the unwrapped reader and store it in GraphReader, so the later cast in createBuilder operates on the reader that actually implements the interface. Adds a test asserting that merging deletion-free segments under the default codec goes through MergingHnswGraphBuilder.
| private static final int SEGMENTS = 2; | ||
|
|
||
| /** | ||
| * Above the tiny-segment cutoff of {@code Lucene99HnswVectorsWriter#shouldCreateGraph} (about 650 |
There was a problem hiding this comment.
Do you think it's worth to make it explicit rather than relying on default threshold assumption like done here: https://github.com/apache/lucene/pull/15419/changes ?
There was a problem hiding this comment.
Thanks for the review and the pointer to #15419.
I think both approaches have pros and cons: an explicit threshold like #15419, and the literal default codec this test uses.
The explicit form is simpler and immune to default retuning, but default codec pins new Lucene99HnswVectorsFormat(), so an explicit threshold means a hand-built format that no longer follows the default codec.
Using the default codec here was deliberate.
This is the only test in the tree that asserts reuse actually triggers (TestHnswMergeAbort only checks for the "build graph" prefix, which the full rebuild also emits), and the regression shipped in the default configuration unnoticed.
The dependence you point out is the cost, but a default change there fails loudly, and the javadoc on the segment size documents the cutoff it needs to clear.
|
Thanks @jeho-rpls for noticing and fixing the behavior! I have left just one minor comment. |
Do you mean you think we should make bug fix releases (10.4.1 and 10.5.1)? |
Yes |
|
OK I think we should keep the scope of this PR to fixing on main, and backporting to 10x, and whomever is going to run the release process can handle migrating the change |
3 participants
Fixes #16400.
The guard in
IncrementalHnswGraphMerger.addReadertests the per-field wrapper instead of the unwrapped reader, so under per-field vectors formats reuse never triggers and every merge rebuilds the graph. Details are in the issue.The fix tests the unwrapped reader and stores it in
GraphReader, becausecreateBuilderlater castsGraphReader#readertoHnswGraphProvider.TestHnswGraphReuseasserts that merging deletion-free segments under the default codec takes the graph reuse path.