Skip to content
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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

benwtrent
Copy link
Member

This is a continuation and completion of the work started by @benchaplin in #14085

The algorithm is fairly simple:

  • Only score and then explore vectors that actually match the filtering criteria
  • Since this will make the graph even sparser, the search spread is increased to also include the candidate's neighbor neighbors (e.g. generally maxConn * maxConn exploration)
  • Additionally, even more scored candidates for a given NSW are considered to combat the increased sparsity

Some of the changes to the baseline Acorn algorithm are:

  • There is some general threshold of filtering that bypasses this algorithm altogether. Early benchmarking seems to indicate that this might be around 50%, but honestly, its not fully convincing...
  • The number of additional neighbors explored is predicated on the percentage of the immediate neighborhood that is filtered out
  • Only look at the extended neighbors if less than 90% of the current neighborhood matches the filter.

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:

  • How to expose this setting to the users? While I am not a fan of more configuration at query time, the behavior seems different enough to justify it.

TODO:

  • More manual testing over more datasets
  • Add some unit and functional tests.

closes: #13940

@benwtrent benwtrent added this to the 10.2.0 milestone Jan 22, 2025
RandomVectorScorer scorer,
KnnCollector knnCollector,
HnswGraph graph,
int filterSize,
Copy link
Contributor

@benchaplin benchaplin Jan 23, 2025

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)
Copy link
Contributor

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.

Copy link
Member Author

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.

@benwtrent
Copy link
Member Author

@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);
}
Copy link
Contributor

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?

Copy link
Member Author

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)) {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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;
}

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Look into ACORN-1, or another algorithm to aid in filtered HNSW search
2 participants