-
Notifications
You must be signed in to change notification settings - Fork 140
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
Adds ef_search support for Lucene kNN queries #1748
Adds ef_search support for Lucene kNN queries #1748
Conversation
@@ -29,7 +29,6 @@ | |||
import java.util.Map; | |||
import java.util.function.Function; | |||
|
|||
import static org.opensearch.knn.index.IndexUtil.isClusterOnOrAfterMinRequiredVersion; |
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.
Why are we removing this ?
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.
It was a leftover from this PR https://github.com/opensearch-project/k-NN/pull/1742/files. The import is not needed. Sorry for the confusion
@@ -85,6 +86,98 @@ public void testCreateLuceneDefaultQuery() { | |||
} | |||
} | |||
|
|||
public void testLuceneFloatVectorQuery() { | |||
Query actualQuery1 = KNNQueryFactory.create( |
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.
Can you add a function to create the query as it is duplicated multiple times
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.
It has a different request everytime. The create request itself is using a builder so wrapping it up with a function is moot
Response response = searchKNNIndex(INDEX_NAME, new KNNQueryBuilder(fieldName, queryVector, k), k); | ||
Response response = searchKNNIndex( | ||
INDEX_NAME, | ||
KNNQueryBuilder.builder().fieldName(fieldName).k(k).vector(queryVector).methodParameters(methodParameters).build(), |
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.
For the end to end test, can we use XContentBuilder to build the query?
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.
Its much more convenient to use KNNBuilder. It converts to XContent underneath.
Not opposed to it but any specific reason we should move to xcontent for happy case tests?
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.
Using XContentBuilder can better mock end-user API calls by simulating JSON content creation and ensuring end-to-end functionality.
KNNBuilder is an internal method, and relying on it in integration test has the possibility to lead to fragile tests that may break with internal changes, even if the public API remains stable.
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.
Fair, will change it
2368565
to
8a33cd0
Compare
Signed-off-by: Tejas Shah <[email protected]>
8a33cd0
to
1af16ac
Compare
f6ab18d
into
opensearch-project:feature/ef-search
Description
Lucene queries use
k
asefsearch
. So we take the max of two to pass is the rightk
value to Lucene. Thesize
parameter will dictate the number of the hitsIssues Resolved
#1537
Check List
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.