Introduces NativeEngineKNNQuery which executes ANN on rewrite #1877

shatejas · 2024-07-25T00:20:24Z

Description

All integration tests are passing. Cannot support radial currently since k is not significant, there are ways to do it but since its not planned for 2.17 this should work

Related Issues

Resolves #1845

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

src/main/java/org/opensearch/knn/search/NativeEngineKnnVectorQuery.java

navneet1v · 2024-07-25T00:33:47Z

@shatejas Lets ensure all CI are working. I know this code is only for feedback. but successful CIs gives confidence.

navneet1v

Overall code looks good to me. I would like to see other usecase of KNNQuery getting added here like currently filters are not present here. Similarly Radial search, other ITs and BWC tests.

src/main/java/org/opensearch/knn/search/NativeEngineKnnVectorQuery.java

jmazanec15

Overall looks good. Few minor comments

src/main/java/org/opensearch/knn/index/query/nativelib/DocAndScoreQuery.java

src/main/java/org/opensearch/knn/index/query/KNNScorer.java

src/main/java/org/opensearch/knn/index/query/KNNWeight.java

src/main/java/org/opensearch/knn/index/query/nativelib/NativeEngineKnnVectorQuery.java

jmazanec15

LGTM

Vikasht34

Looks Good to me .

CHANGELOG.md

src/main/java/org/opensearch/knn/index/KNNSettings.java

vibrantvarun · 2024-08-05T18:23:34Z

src/main/java/org/opensearch/knn/index/query/KNNWeight.java

+     * @param context LeafReaderContext
+     * @return A Map of docId to scores for top k results
+     */
+    public Map<Integer, Float> searchLeaf(LeafReaderContext context) throws IOException {

        final BitSet filterBitSet = getFilteredDocsBitSet(context);
        int cardinality = filterBitSet.cardinality();
        // We don't need to go to JNI layer if no documents are found which satisfy the filters
        // We should give this condition a deeper look that where it should be placed. For now I feel this is a good
        // place,


I think this comment does not describe the new change added. Can you describe why are we returning an empty Map?

It still applies - Its an optimization where we don't search (don't go to JNI layer). filters being empty indicates no results were found. Doing additional search will always results in empty response.

We are returning empty Map to indicate that there were no matches. This is for both NativeEngineKnnVectorQuery as well as scorer in this class. The emtyScorer part is simply moved to line 113

navneet1v · 2024-08-05T18:25:58Z

Minor comments, overall code looks good to me

navneet1v · 2024-08-05T22:36:22Z

@shatejas please resolve conflicts.

jmazanec15

Can you fix changelog? Other than that looks good

jmazanec15 · 2024-08-06T16:17:37Z

CHANGELOG.md

@@ -28,5 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 * Clean up parsing for query [#1824](https://github.com/opensearch-project/k-NN/pull/1824)
 * Refactor engine package structure [#1913](https://github.com/opensearch-project/k-NN/pull/1913)
 * Refactor method structure and definitions [#1920](https://github.com/opensearch-project/k-NN/pull/1920)
+* Generalize lib interface to return context objects [#1925](https://github.com/opensearch-project/k-NN/pull/1925)


duplicate changelog entry

Signed-off-by: Tejas Shah <[email protected]>

jmazanec15

LGTM thanks

opensearch-trigger-bot · 2024-08-06T18:38:20Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1877-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 df7627c6e580843beb2361f5c2ec3519efd52280
# Push it to GitHub
git push --set-upstream origin backport/backport-1877-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1877-to-2.x.

…arch-project#1877) Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit df7627c)

…#1943) Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit df7627c)

shatejas requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners July 25, 2024 00:20

shatejas commented Jul 25, 2024

View reviewed changes

src/main/java/org/opensearch/knn/search/NativeEngineKnnVectorQuery.java Outdated Show resolved Hide resolved

navneet1v reviewed Jul 25, 2024

View reviewed changes

jmazanec15 reviewed Jul 25, 2024

View reviewed changes

src/main/java/org/opensearch/knn/search/NativeEngineKnnVectorQuery.java Outdated Show resolved Hide resolved

Vikasht34 reviewed Jul 26, 2024

View reviewed changes

src/main/java/org/opensearch/knn/search/NativeEngineKnnVectorQuery.java Outdated Show resolved Hide resolved

shatejas force-pushed the poc/rewrite branch 2 times, most recently from d923ba0 to 8d9b082 Compare August 1, 2024 02:17

shatejas changed the title ~~[DO NOT MERGE][NEED INITIAL FEEDBACK]Introduces NativeEngineKNNQuery which executes ANN on rewrite~~ Introduces NativeEngineKNNQuery which executes ANN on rewrite Aug 1, 2024

shatejas force-pushed the poc/rewrite branch from 5d9cfcc to 6f0d4b3 Compare August 1, 2024 20:31

shatejas requested review from navneet1v, jmazanec15 and Vikasht34 August 1, 2024 20:37

jmazanec15 reviewed Aug 1, 2024

View reviewed changes

shatejas force-pushed the poc/rewrite branch 5 times, most recently from 5ec1eb6 to ff64081 Compare August 1, 2024 22:52

shatejas requested a review from jmazanec15 August 1, 2024 22:54

jmazanec15 previously approved these changes Aug 1, 2024

View reviewed changes

shatejas force-pushed the poc/rewrite branch from b4b5233 to 99fc4f7 Compare August 3, 2024 00:14

Vikasht34 approved these changes Aug 3, 2024

View reviewed changes

navneet1v reviewed Aug 5, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

navneet1v reviewed Aug 5, 2024

View reviewed changes

src/main/java/org/opensearch/knn/index/KNNSettings.java Show resolved Hide resolved

vibrantvarun reviewed Aug 5, 2024

View reviewed changes

shatejas force-pushed the poc/rewrite branch from 99fc4f7 to 4f97f5b Compare August 5, 2024 22:01

shatejas requested review from navneet1v, Vikasht34 and vibrantvarun August 5, 2024 22:19

Vikasht34 approved these changes Aug 5, 2024

View reviewed changes

jmazanec15 reviewed Aug 6, 2024

View reviewed changes

vibrantvarun approved these changes Aug 6, 2024

View reviewed changes

Introduces NativeEngineKNNQuery which executes ANN on rewrite

649f46b

Signed-off-by: Tejas Shah <[email protected]>

shatejas force-pushed the poc/rewrite branch from 1a9d418 to 649f46b Compare August 6, 2024 17:21

shatejas requested a review from jmazanec15 August 6, 2024 17:22

jmazanec15 approved these changes Aug 6, 2024

View reviewed changes

navneet1v approved these changes Aug 6, 2024

View reviewed changes

navneet1v merged commit df7627c into opensearch-project:main Aug 6, 2024
27 of 29 checks passed

navneet1v added backport 2.x v2.17.0 Enhancements Increases software capabilities beyond original client specifications Refactoring Improve the design, structure, and implementation while preserving its functionality labels Aug 6, 2024

shatejas added a commit to shatejas/k-NN that referenced this pull request Aug 8, 2024

Introduces NativeEngineKNNQuery which executes ANN on rewrite (opense…

368ed00

…arch-project#1877) Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit df7627c)

shatejas mentioned this pull request Aug 8, 2024

[Backport 2.x] Introduces NativeEngineKNNQuery which executes ANN on rewrite (#1877) #1943

Merged

5 tasks

shatejas added a commit to shatejas/k-NN that referenced this pull request Aug 8, 2024

Introduces NativeEngineKNNQuery which executes ANN on rewrite (opense…

41d3eea

…arch-project#1877) Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit df7627c)

junqiu-lei pushed a commit that referenced this pull request Aug 9, 2024

Introduces NativeEngineKNNQuery which executes ANN on rewrite (#1877) (…

20039d0

…#1943) Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit df7627c)

shatejas deleted the poc/rewrite branch August 12, 2024 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces NativeEngineKNNQuery which executes ANN on rewrite #1877

Introduces NativeEngineKNNQuery which executes ANN on rewrite #1877

shatejas commented Jul 25, 2024 •

edited

Loading

navneet1v commented Jul 25, 2024 •

edited

Loading

navneet1v left a comment

jmazanec15 left a comment

jmazanec15 left a comment

Vikasht34 left a comment

vibrantvarun Aug 5, 2024

shatejas Aug 5, 2024

navneet1v commented Aug 5, 2024

navneet1v commented Aug 5, 2024

jmazanec15 left a comment

jmazanec15 Aug 6, 2024

jmazanec15 left a comment

opensearch-trigger-bot bot commented Aug 6, 2024

Introduces NativeEngineKNNQuery which executes ANN on rewrite #1877

Introduces NativeEngineKNNQuery which executes ANN on rewrite #1877

Conversation

shatejas commented Jul 25, 2024 • edited Loading

Description

Related Issues

Check List

navneet1v commented Jul 25, 2024 • edited Loading

navneet1v left a comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

Vikasht34 left a comment

Choose a reason for hiding this comment

vibrantvarun Aug 5, 2024

Choose a reason for hiding this comment

shatejas Aug 5, 2024

Choose a reason for hiding this comment

navneet1v commented Aug 5, 2024

navneet1v commented Aug 5, 2024

jmazanec15 left a comment

Choose a reason for hiding this comment

jmazanec15 Aug 6, 2024

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 6, 2024

shatejas commented Jul 25, 2024 •

edited

Loading

navneet1v commented Jul 25, 2024 •

edited

Loading