-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add only forward links when repairing HNSW merge (fixes #15467) #15470
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
base: main
Are you sure you want to change the base?
Conversation
| popToScratch(candidates, scratchArray); | ||
|
|
||
| // Add diverse neighbors and establish bidirectional connections | ||
| addDiverseNeighbors(targetLevel, node, scratchArray, scorer, true); |
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 think we can keep addDiverseNeighbors here? This is also used during addition of new node to a level (rebalancing step).
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.
oops, I'll add this back! I wonder if we should do at least a quick test to make sure this change isn't harming recall too much in the heavy deletion case, but I'm unsure how you ran the tests. I guess it was with luceneutil? Did you commit any changes to thatin support of the 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.
Yes, that's a good idea. Let me check the recall with these changes.
Did you commit any changes to thatin support of the testing?
No, not yet. Will try to raise PR for adding deletes support in knnPerfTest in some time.
|
Thanks @msokolov for catching and fixing this issue. I should have been more careful in my PR. |
|
We are seeing recall drop with this code fix: I indexed 100k docs in a single segment (using force-merge), then deleted 40% random docs and then again performed force-merge and then computed recall. Here is the recall results between baseline and candidate and we are seeing recall drop consistently across all maxConn values:
Next: Running more tests with different delete %. |
|
I have raised another PR with brute force approach to fix the test: #15478. In that approach, I am not seeing any recall regression but seeing around 2% regression in indexing rate. Please let me know your thoughts if we should pursue that approach or not. |
This is what I was thinking -- when "repairing" only add links from the node being repaired and not back again. This is needed because ordinarily we are adding nodes in order and each node is only added once, so there is no opportunity for duplication, but in this case we are effectively "re-adding" the node. So we would have to check for duplicates when making links, or we can just forgo adding the reverse links. Is this good enough? I don't know, I haven't had time to do any extensive testing, but it at least gets rid of the duplicates.