xzhang9292 opened a new issue, #16726: URL: https://github.com/apache/lucene/issues/16726
Hi all, I've been reading through the HNSW construction path (HnswGraphBuilder, OnHeapHnswGraph, NeighborArray, InitializedHnswGraphBuilder) and found something I couldn't explain from the code, so I'd like to check whether it's deliberate. [InitializedHnswGraphBuilder.rebalanceGraph()](https://github.com/apache/lucene/blob/e357029271b3de560a6ff13c9cab4d4c8103b53b/lucene/core/src/java/org/apache/lucene/util/hnsw/InitializedHnswGraphBuilder.java#L429) creates its own random source with no seed: ```java private void rebalanceGraph() throws IOException { SplittableRandom random = new SplittableRandom(); // no seed ... if (random.nextDouble() < invMaxConn && !hnsw.nodeExistAtLevel(level, node)) { hnsw.addNode(level, node); ``` This decides which nodes get promoted to upper levels after a merge. Everywhere else on this path the randomness is threaded deliberately: HnswGraphBuilder takes a seed, and its javadoc says "Provide this to ensure repeatable construction" getRandomGraphLevel(ml, random) uses that seeded instance HnswConcurrentMergeBuilder passes HnswGraphBuilder.randSeed to each worker HnswGraphTestCase.testRandomReadWriteAndMerge explicitly sets HnswGraphBuilder.randSeed = seed before exercising merges with deletes Since #15003, the repair/rebalance path only runs when the segment has deletes. testRandomReadWriteAndMerge does randomly delete documents, so as far as I can tell it reaches this path — but the promotion decisions inside rebalanceGraph() don't use randSeed, so re-running with the same seed would not reproduce the same level structure. This doesn't show up as flakiness today, because the tests that exercise merge-with-deletes assert fairly shallow properties — testRandomReadWriteAndMerge asserts that the vector dimension survived, and testGraphMergeWithDeletes asserts the live node count. BaseIndexFileFormatTestCase.testMergeStability, which does compare merged output, indexes and force-merges without deletions, so it doesn't reach this path at all. So the concern isn't a failing test — it's that if something does go wrong on this path (which was raised as the main testing concern during review of #15003), the seed wouldn't fully reproduce it. Two questions: Is the unseeded random intentional — e.g. because rebalancing is a heuristic whose exact outcome isn't meant to be reproducible? If not, would it make sense to thread the builder's seed (or HnswGraphBuilder.randSeed) through to this method? I'd be glad to put up a patch. I may be missing context — if the seeding choice was discussed somewhere, a pointer would be welcome. Thanks, Xin Zhang -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
