Add support for radial search in exact search #2174
Conversation
VijayanB
requested review from
heemin32,
navneet1v,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
ryanbogan and
luyuncheng
as code owners
|
This depends on #2136 |
VijayanB
force-pushed
the
support-exact-search-for-radius
branch
from
cbd28c4
to
a2936f3
Compare
|
I am skipping changelog. While merging back to main will update changelog. |
VijayanB
force-pushed
the
support-exact-search-for-radius
branch
3 times, most recently
from
e0ab310
to
0b33a1b
Compare
| */ | ||
| private boolean isExactSearchRequire(final LeafReaderContext context, final int filterIdsCount, final int annResultCount) { | ||
| if (annResultCount == 0 && isMissingNativeEngineFiles(context)) { | ||
| log.debug("Perform exact search after approximate search since no native engine files are available"); |
There was a problem hiding this comment.
Is this case unexpected so we use debug level log?
There was a problem hiding this comment.
Since this is in path of query, recommendation is to avoid info.
VijayanB
force-pushed
the
support-exact-search-for-radius
branch
3 times, most recently
from
dc81e8f
to
230f286
Compare
src/main/java/org/opensearch/knn/index/query/ExactSearcher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/ExactSearcher.java
Outdated
Show resolved
Hide resolved
When threshold value is set, knn plugin will not be creating graph. Hence, when search request is trigged during that time, exact search will return valid results. However, radial search was never included as part of exact search. This will break radial search when threshold is added and radial search is requested. In this commit, new method is introduced to accept min score and return documents that are greater than min score, similar to how radial search is performed by native engines. This search is independent of engine, but, radial search is supported only for FAISS engine out of all native engines. Signed-off-by: Vijayan Balasubramanian <[email protected]>
VijayanB
force-pushed
the
support-exact-search-for-radius
branch
4 times, most recently
from
6c47860
to
abc6e69
Compare
Added unit test |
| @SneakyThrows | ||
| public void testRadialSearch_whenNoEngineFiles_thenPerformExactSearch() { |
There was a problem hiding this comment.
Thought I see we are using this test to check ExactSearch for radial search and avoided the Unit test for ExactSearch with radial search threshold. See if you want to mock ExactSearcher search class and all its invocation to make it proper Unit test. Since exact search is not getting added more and more extra logic do you see we should have separate unit test class for ExactSearcher class.
There was a problem hiding this comment.
Good point. I can create Github issue for refactor. I see two option, 1) Keep this PR as it is and refactor as part of GH issue since there are more than 3 test cases that uses exact search, or 2) I can only do it for radial search, as part of GH issue others can be moved later
There was a problem hiding this comment.
I like the option 2. Lets do atleast for this test case and create a GH issue too for the test refactor.
There was a problem hiding this comment.
updated this PR. Will create GH issue
Signed-off-by: Vijayan Balasubramanian <[email protected]>
VijayanB
force-pushed
the
support-exact-search-for-radius
branch
2 times, most recently
from
7466091
to
efa7cb3
Compare
Signed-off-by: Vijayan Balasubramanian <[email protected]>
VijayanB
force-pushed
the
support-exact-search-for-radius
branch
from
efa7cb3
to
e51ae11
Compare
| @@ -71,15 +106,17 @@ private Map<Integer, Float> scoreAllDocs(KNNIterator iterator) throws IOExceptio | |||
| return docToScore; | |||
| } | |||
|
|
|||
| private Map<Integer, Float> searchTopK(KNNIterator iterator, int k) throws IOException { | |||
| private Map<Integer, Float> searchTopCandidates(KNNIterator iterator, int limit, @NonNull Predicate<Float> filterScore) | |||
There was a problem hiding this comment.
Curious why k variable is renamed to limit?
There was a problem hiding this comment.
This method will be called by radialSearch as well. Hence, renamed it from K to be more generic.
Description
When threshold value is set, knn plugin will not be creating graph. Hence, when search request is trigged during that time, exact search will return valid results. However, radial search was never included as part of exact search. This will break radial search when threshold is added and radial search is requested. In this commit, new method
is introduced to accept min score and return documents that are greater than min score, similar to how radial search is performed by native engines. This search is independent of engine, but, radial search is supported only for FAISS engine out of all native engines.
Related Issues
Part of #1942
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.