-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[ENH] Make Rust/C++ FFI error handling robust #2667
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
@@ -155,11 +162,6 @@ class Index | |||
} | |||
AllowAndDisallowListFilterFunctor filter = AllowAndDisallowListFilterFunctor(allow_list, disallow_list); | |||
std::priority_queue<std::pair<dist_t, hnswlib::labeltype>> res = appr_alg->searchKnn(query_vector, k, &filter); | |||
if (res.size() < k) |
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.
was this untested behavior?
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.
Not really, this path was never hit, it should have been removed, we always set k to min(k, len()) in calling code.
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.
maybe it pays to be defensive here, I haven't looked but is there any chance of UB if we do send k > res.size()
?
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.
nope, hnsw handles it fine, it just underpopulates the results, this was carried over from the hnsw bindings for python that choose to explicitly throw. We are changing the contract from the python bindings that it is OK to return < K results
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.
neat
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
Added a new test to test add() errors
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
None