bug fix for non-normalized dot product vectors returning no matches and causing assertion to fail if using hierarchy - #714
Open
MarkWolters wants to merge 1 commit into
Open
bug fix for non-normalized dot product vectors returning no matches and causing assertion to fail if using hierarchy#714MarkWolters wants to merge 1 commit into
MarkWolters wants to merge 1 commit into
Conversation
Contributor
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
MarkWolters
marked this pull request as ready for review
August 24, 2026 16:25
MarkWolters
requested review from
ashkrisk,
jshook and
tlwillke
as code owners
August 24, 2026 16:25
ashkrisk
reviewed
Aug 27, 2026
| * @param topK the number of results to look for. With threshold=0, the search will continue until at least | ||
| * `topK` results have been found, or until the entire graph has been searched. | ||
| * @param rerankK the number of (approximately-scored) results to rerank before returning the best `topK`. | ||
| * @param threshold the minimum similarity (0..1) to accept; 0 will accept everything. May be used |
Contributor
There was a problem hiding this comment.
Most of the Javadocs for the *search* functions in this file either imply or explicitly state that threshold = 0 will accept everything. We should probably update all of the affected lines to make it clear that this isn't true, and that users should use Float.NEGATIVE_INFINITY if they really want to accept everything.
| int topK, | ||
| float threshold, | ||
| Bits acceptOrds) { | ||
| return search(scoreProvider, topK, topK, threshold, 0.0f, acceptOrds); |
Contributor
There was a problem hiding this comment.
Should we set the rerankFloor parameter here to Float.NEGATIVE_INFINITY instead of 0.0f?
| int topK, | ||
| Bits acceptOrds) | ||
| { | ||
| return search(scoreProvider, topK, 0.0f, acceptOrds); |
Contributor
There was a problem hiding this comment.
Similarly, here we might want to change the hard-coded threshold parameter from 0.0f to Float.NEGATIVE_INFINITY
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR resolves issue #713.
When dot product vsf is used for graph search in conjunction with enabling hierarchy this can result in a failed Assertion when no candidate is found to be sufficiently close to the search vector at the level being searched, in this case where level > 0.
Any candidate whose score is < 0 is silently dropped, never added to approximateResults. If every candidate reachable from the entry point within that hierarchy layer happens to score negative for a given query, approximateResults stays empty and the size() == 1 assert fires.
0.0f is only a valid "accept everything" sentinel for similarity functions that are mathematically bounded to (0, 1] — COSINE and EUCLIDEAN are. DOT_PRODUCT is not bounded unless callers pre-normalize vectors to unit length. VectorDotProductWithLengthTest.testTrueDotproduct (the failing Cassandra unit test that surfaced this issue) deliberately uses non-unit 2D vectors with components in [-100, 100].