jbellis commented on PR #12255: URL: https://github.com/apache/lucene/pull/12255#issuecomment-1551523734
 ^ This is almost the entire diff, there is also the same change for byte[] and then `private void searchLevel` takes the NeighborQueue as a parameter. The differences that I see are - we have a redundant call to `clear()` now for the very first time that `results` is used, but that's just setting size variables to `0` - If we return early while traversing the index levels because we hit visit visitedLimit, the new code allocates the full NQ of `topK` instead of just 1. (I also see that Builder isn't taking advantage of the new API, so that's a missed opportunity on my part but I assume QPS isn't measuring build time.) It's hard for me to tell what code path the tester is exercising, but if it is specifying a low-ish visitedLimit then that would explain the regression. (It would also mean that the tester should expect pretty bad recall since it's not actually getting to the bottom level of the graph.) I tried pushing to the branch for this PR but it didn't seem to do anything, so I opened a new PR that addresses the Builder and the visitedLimit issues at #12303. P.S. I also notice that the byte[] version is missing these lines from the float[] version, not sure if that's intended or not ``` int initialEp = graph.entryNode(); if (initialEp == -1) { return new NeighborQueue(1, true); } ``` -- 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: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
