Repository navigation
Conversation
IncrementalHnswGraphMerger#addReader compared a candidate's live vector count against the current base's total node count (graphSize), which includes deleted nodes since GITHUB#15003 admitted graphs with deletes as the base. A graph with deletions could therefore stay the base over a larger deletion-free graph, depending on reader order. Track the live count separately in GraphReader and compare live against live; graphSize stays the total node count used for array sizing.
| /** Represents a vector reader that contains graph info. */ | ||
| /** | ||
| * Represents a vector reader that contains graph info. | ||
| * |
There was a problem hiding this comment.
While you are here, let's add params for the other members as well here for completeness?
There was a problem hiding this comment.
Done, added @param for reader and initDocMap as well.
| Optimizations | ||
| --------------------- | ||
|
|
||
| * GITHUB#16739: IncrementalHnswGraphMerger now picks the base graph for a merge by comparing live |
There was a problem hiding this comment.
I think this should go into Bug Fixes (But I am not sure so no strong opinion).
| try (IndexWriter w = new IndexWriter(dir, cfg)) { | ||
| addSegment(w, "a", 1000); | ||
| addSegment(w, "b", 900); | ||
| for (int i = 0; i < 300; i++) { |
There was a problem hiding this comment.
I think we are trying to keep the delete% under 40% which is set in IncrementalHnswGraphMerger.java.DELETE_PCT_THRESHOLD. What is that threshold changes, this can make the test case buggy as it can fail/succeed for wrong reasons.
We should derive the delete% here based on IncrementalHnswGraphMerger.java.DELETE_PCT_THRESHOLD instead of hardcoding.
| cfg.setCodec(TestUtil.alwaysKnnVectorsFormat(new Lucene99HnswVectorsFormat(16, 100, 0))); | ||
| cfg.setMergePolicy(NoMergePolicy.INSTANCE); | ||
| try (IndexWriter w = new IndexWriter(dir, cfg)) { | ||
| addSegment(w, "a", 1000); |
There was a problem hiding this comment.
Should we make constant variable as we are using same value across multiple places?
private static final int A_DOCS = 1000;
private static final int B_DOCS = 900;
private static final String FIELD_NAME = "id";
| IncrementalHnswGraphMerger merger = | ||
| concurrent | ||
| ? new ConcurrentHnswMerger(fieldInfo, null, 16, 100, null, 2) | ||
| : new IncrementalHnswGraphMerger(fieldInfo, null, 16, 100); |
There was a problem hiding this comment.
Should we move the constant from here as well i.e M and beamWidth?
| * Tests which source graph {@link IncrementalHnswGraphMerger} picks as the base graph when some | ||
| * candidates carry deletions. | ||
| */ | ||
| public class TestIncrementalHnswGraphMergerBaseSelection extends LuceneTestCase { |
There was a problem hiding this comment.
Non blocking: Should we make the test more robust which covers more scenario and not just live(A) < live(B) with total(A) > total(B)?
There was a problem hiding this comment.
Added testBaseGraphHasMostLiveVectorsAmongEligible: 2-6 segments with random sizes and deletion counts (about half of them within the threshold), readers added in 5 shuffled orders, on both IncrementalHnswGraphMerger and ConcurrentHnswMerger.
…zed test - Make DELETE_PCT_THRESHOLD a package-private static constant so the test can derive its deletion counts from it instead of hardcoding them. - Add a randomized test: random segment sizes and deletion counts, readers added in shuffled order, for both mergers. - Name the test constants, document every GraphReader component. - Move the CHANGES entry to Bug Fixes.
…ive-count # Conflicts: # lucene/CHANGES.txt
|
We should try to send this PR in 10.6 release. I don't have permission to add the milestone. |
|
LGTM at first glance, assuming we want to start with a graph having the highest live vector count. However, I'm not super familiar with the original change, and it would be great to get this reviewed by someone who is! Adding the 10.6 milestone for now, given it is a small fix. |
Pulkitg64
left a comment
Pulkitg64
left a comment
There was a problem hiding this comment.
Thanks @LantaoJin! The new changes looks good to me.
3 participants
Description
IncrementalHnswGraphMerger#addReaderpicks the base graph for a merge withcandidateVectorCount > largestGraphReader.graphSize. The left side is the candidate's live vector count. The right side is the current base's total node count, which includes deleted nodes. Before #15003 the two were always equal, because graphs with deletions were never admitted as the base. Since #15003, a graph with up to 40% deletions can become the base, so the comparison mixes units.As a result, which graph becomes the base depends on the order in which readers are added. Example: segment A has 1000 nodes with 300 deleted (700 live), and segment B has 900 nodes with no deletions. If A is added first, B would need 900 > 1000 to replace it, so A stays the base even though B has more live vectors. If B is added first, B is chosen. The merged graph is still correct either way, so this is a performance issue, not a correctness issue. With a worse base, more nodes must be joined or inserted. This hurts most in
ConcurrentHnswMerger, which initializes only from the base graph and inserts every other node one by one.MergingHnswGraphBuilderjoins the other deletion-free graphs, so the effect there is smaller.The fix adds a
liveVectorCountcomponent to theGraphReaderrecord and compares live against live.graphSizestays the total node count, becausegetNewOrdMappinguses it to size the old-to-new ordinal arrays. Ties still keep the earlier reader.ConcurrentHnswMergerinheritsaddReader, so it is covered too.GraphReaderis aprotectedrecord, so adding a component changes its canonical constructor. Nothing in Lucene constructs it outsideIncrementalHnswGraphMerger.