-
Notifications
You must be signed in to change notification settings - Fork 169
[GRPC] Extend transport-grpc-spi to support GRPC KNN queries #2833
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
Conversation
10164f8 to
a6be98f
Compare
… to support GRPC KNN queries Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen Xu <[email protected]>
Signed-off-by: Karen X <[email protected]>
|
Hi @navneet1v @shatejas @finnroblin, the CI failure seems unrelated to this PR, as I didn't chnage any of the files related to CMake or JNI. Could you share any insights on how this could be fixed? Thanks! |
|
Hi @peterzhuamazon , do you know why the MacOS build would fail (with no changes to C++/JNI that would break the compilation)? Has any infrastructure changed recently on the MacOS CI runners? Compilation is working for me locally on MacOS 15.3.2 with both JDK 21 and JDK 24. Thanks |
finnroblin
left a comment
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.
LGTM
Vikasht34
left a comment
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 we have complete unit test and if possible unit test for this PR?
...n/java/org/opensearch/knn/grpc/proto/request/search/query/KNNQueryBuilderProtoConverter.java
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/grpc/proto/request/search/query/KNNQueryBuilderProtoUtils.java
Outdated
Show resolved
Hide resolved
|
@karenyrx can you please resolve the comments and get this PR in good shape. The KNN build is failing without this change |
src/main/java/org/opensearch/knn/grpc/proto/request/search/query/KNNQueryBuilderProtoUtils.java
Outdated
Show resolved
Hide resolved
Signed-off-by: karenx <[email protected]>
A good test is that that previous unit tests continue to pass (as this is mostly a refactor), but added some new ones as well to cover more edge cases |
Signed-off-by: karenx <[email protected]>
|
@karenyrx the CIs are failing |
Signed-off-by: karenx <[email protected]>
navneet1v
left a comment
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.
Approving the code, since CI is only left. Overall code looks good to me.
|
@Vikasht34 can you please review this PR now, this PR needs to be merged since this is blocking the builds. |
Vikasht34
left a comment
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.
LGTM
|
@karenyrx @finnroblin @Vikasht34 @navneet1v After this PR is merged we can run into new issues(test): I have tried to do below to fix it in this PR and seems like it can pass the github checks. But when I start the cluster locally and try to test the knn query it will fail with error: Which seems related to the change about Could you help take a look? |
|
|
|
opensearch-project/OpenSearch@b9c5bc7#diff-1767b7f709666659fcea718930d017913630119903232332e9b9b08627ca7c0fR29 may be the culprit of the failures rather than this PR |
Description
Used in conjunction with opensearch-project/OpenSearch#18949
Related Issues
Part of #2816
Related to opensearch-project/OpenSearch#18513
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.