-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new Acorn-esque filtered HNSW search heuristic #14160
base: main
Are you sure you want to change the base?
Conversation
RandomVectorScorer scorer, | ||
KnnCollector knnCollector, | ||
HnswGraph graph, | ||
int filterSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI missed a javadoc param for filterSize
* exceeded | ||
* @throws IOException When accessing the vector fails | ||
*/ | ||
private int findBestEntryPoint(RandomVectorScorer scorer, KnnCollector collector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on abstracting this since it's identical (minus source of graph
) to HnswGraphSearcher::findBestEntryPoint
? I suppose we might want to investigate multiple entry points in the future so maybe the duplicate code will be gone soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind having two pieces of code vs. the wrong abstraction. We can refactor if we wish in a separate PR later. But if only two things are copy-pasting code, its probably ok.
@msokolov I wonder your opinion here? Do you think the behavior change/result change is worth waiting for a major? I do think folks should be able to use this now, but be able to opt out. Another option I could think of is injecting a parameter or something directly into SPI loading for the hnsw vector readers. But I am not 100% sure how to do that. It does seem like it should be something that is a "global" configuration for a given Lucene instance instead of one that is provided at query time. |
} | ||
if (acceptOrds.get(friendOfAFriendOrd)) { | ||
toScore.add(friendOfAFriendOrd); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting else { toExplore.add(friendOfAFriendOrd) }
here for >2-hop exploration. Did you drop that idea after performance testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benchaplin yeah, going further than 2 hops didn't seem to improve much. We can adjust it later, but it didn't improve anything.
int friendOrd; | ||
while ((friendOrd = graph.nextNeighbor()) != NO_MORE_DOCS && toScore.isFull() == false) { | ||
assert friendOrd < size : "friendOrd=" + friendOrd + "; size=" + size; | ||
if (visited.get(friendOrd) || explorationVisited.getAndSet(friendOrd)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of explorationVisited
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to separate out the explored for 2-hop candidates vs immediate neighborhoods. If we did an expanded search for a candidate, we don't want to search it more during expanded search.
However, if that candidate matches, we DON'T want to skip it (e.g. during regular search).
Maybe we can collapse the two visited sets together, but two of them seemed OK to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes that makes sense. So for immediate neighborhoods wouldn't we just want to do:
if (visited.setAndGet(friendOrd)) {
continue;
}
?
This is a continuation and completion of the work started by @benchaplin in #14085
The algorithm is fairly simple:
Some of the changes to the baseline Acorn algorithm are:
Here are some numbers for 1M vectors, float32 and then int4 quantized.
https://docs.google.com/spreadsheets/d/1GqD7Jw42IIqimr2nB78fzEfOohrcBlJzOlpt0NuUVDQ/edit?gid=163290867#gid=163290867
Something I am unsure about:
TODO:
closes: #13940