Adds rescore parameter to KNNQuery #1969

jmazanec15 · 2024-08-15T18:27:06Z

Description

Adds rescore parameter to knn query. With this commit, the rescore is a no-op. The functionality and validation will be added in a later commit.

The valid options look like:

"query": {
  "knn": {
    "target_field": {
      "vector": [0, 0],
      "k": 100,
      "rescore": true
    }
  }
}

"query": {
  "knn": {
    "target_field": {
      "vector": [0, 0],
      "k": 100,
      "rescore": {}
    }
  }
}

  "query": {
    "knn": {
      "target_field": {
        "vector": [0, 0],
        "k": 100,
        "rescore": {
            "oversample_factor": 1.2
        }
      }
    }
  }

Will update changelog once feature is completed

Related Issues

Related to #1779

Check List

Commits are signed per the DCO using --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.

Adds rescore parameter to knn query. With this commit, the rescore is a no-op. The functionality and validation will be added in a later commit. Signed-off-by: John Mazanec <[email protected]>

src/main/java/org/opensearch/knn/common/KNNConstants.java

src/main/java/org/opensearch/knn/index/query/parser/RescoreParser.java

navneet1v · 2024-08-15T19:41:30Z

src/main/java/org/opensearch/knn/index/query/rescore/RescoreContext.java

+public final class RescoreContext {
+
+    public static final float DEFAULT_OVERSAMPLE_FACTOR = 1.0f;
+    public static final float MAX_OVERSAMPLE_FACTOR = 5.0f;


@jmazanec15 will 5 be good enough may be 100? I know nothing is good enough. but I was thinking with 100 as max and a cap of 10K on the k*oversampling factor should cover most of the cases.

Thoughts?

Sure I can increase. Let me update

src/main/java/org/opensearch/knn/index/query/rescore/RescoreContext.java

shatejas · 2024-08-15T19:38:41Z

src/main/java/org/opensearch/knn/index/query/parser/KNNQueryBuilderParser.java

@@ -110,6 +124,10 @@ public static KNNQueryBuilder.Builder streamInput(StreamInput in, Function<Strin
            builder.methodParameters(MethodParametersParser.streamInput(in, IndexUtil::isClusterOnOrAfterMinRequiredVersion));
        }

+        if (minClusterVersionCheck.apply(RESCORE_PARAMETER)) {


I think this will affect neural, just a heads up

shatejas · 2024-08-15T19:41:33Z

src/main/java/org/opensearch/knn/index/util/IndexUtil.java

@@ -48,6 +48,8 @@ public class IndexUtil {
    private static final Version MINIMAL_SUPPORTED_VERSION_FOR_RADIAL_SEARCH = Version.V_2_14_0;
    private static final Version MINIMAL_SUPPORTED_VERSION_FOR_METHOD_PARAMETERS = Version.V_2_16_0;
    private static final Version MINIMAL_SUPPORTED_VERSION_FOR_MODEL_VECTOR_DATA_TYPE = Version.V_2_16_0;
+    // TODO: Will update once 2.17 backport change is merged
+    private static final Version MINIMAL_RESCORE_FEATURE = Version.V_3_0_0;


nit: Look around if there is a need to have this in neural plugin

shatejas · 2024-08-15T19:44:24Z

src/test/java/org/opensearch/knn/index/query/KNNQueryBuilderTests.java

@@ -776,19 +777,23 @@ public void testDoToQuery_InvalidZeroByteVector() {

    public void testSerialization() throws Exception {


nit: Does KNNQueryBuilderValidParamsTests and KNNQueryBuilderInvalidParamsTests need an update?

Ill add some basic ones, but do most of testing in the Rescore validation tests

src/main/java/org/opensearch/knn/index/query/rescore/RescoreContext.java

Signed-off-by: John Mazanec <[email protected]>

ryanbogan

LGTM

shatejas · 2024-08-15T21:26:09Z

src/test/java/org/opensearch/knn/integ/QueryParseIT.java

+import static org.opensearch.knn.index.query.parser.RescoreParser.RESCORE_OVERSAMPLE_PARAMETER;
+import static org.opensearch.knn.index.query.parser.RescoreParser.RESCORE_PARAMETER;
+
+public class QueryParseIT extends KNNRestTestCase {


nit: https://github.com/opensearch-project/k-NN/blob/main/src/test/java/org/opensearch/knn/index/query/InvalidSearchQueryIT.java

Seems like a duplicate in a way, maybe added a todo on one of them to merge for later

Yes, that probably makes sense. Let me make a note on this and come back with it in another PR, with some other test cleanup.

Vikasht34

LGTM !!

Adds rescore parameter to knn query. With this commit, the rescore is a no-op. The functionality and validation will be added in a later commit. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 9db7058)

* Adds rescore parameter to KNNQuery (#1969) Adds rescore parameter to knn query. With this commit, the rescore is a no-op. The functionality and validation will be added in a later commit. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 9db7058) * fix version Signed-off-by: John Mazanec <[email protected]> --------- Signed-off-by: John Mazanec <[email protected]> Co-authored-by: John Mazanec <[email protected]>

Adds rescore parameter to knn query. With this commit, the rescore is a no-op. The functionality and validation will be added in a later commit. Signed-off-by: John Mazanec <[email protected]> Signed-off-by: Akash Shankaran <[email protected]>

jmazanec15 added backport 2.x skip-changelog labels Aug 15, 2024

jmazanec15 requested review from heemin32, navneet1v, VijayanB, vamshin, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners August 15, 2024 18:27

Adds rescore parameter to KNNQuery

dc8e049

Adds rescore parameter to knn query. With this commit, the rescore is a no-op. The functionality and validation will be added in a later commit. Signed-off-by: John Mazanec <[email protected]>

jmazanec15 force-pushed the rescore-interface branch from c34cde1 to dc8e049 Compare August 15, 2024 18:45

navneet1v reviewed Aug 15, 2024

View reviewed changes

shatejas reviewed Aug 15, 2024

View reviewed changes

naveentatikonda previously approved these changes Aug 15, 2024

View reviewed changes

Address comments

20210a2

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 dismissed naveentatikonda’s stale review via 20210a2 August 15, 2024 20:33

ryanbogan approved these changes Aug 15, 2024

View reviewed changes

shatejas reviewed Aug 15, 2024

View reviewed changes

shatejas approved these changes Aug 15, 2024

View reviewed changes

jmazanec15 requested review from naveentatikonda and navneet1v August 15, 2024 21:27

Vikasht34 approved these changes Aug 15, 2024

View reviewed changes

naveentatikonda approved these changes Aug 15, 2024

View reviewed changes

jmazanec15 merged commit 9db7058 into opensearch-project:main Aug 15, 2024
31 checks passed

opensearch-trigger-bot bot mentioned this pull request Aug 15, 2024

[Backport 2.x] Adds rescore parameter to KNNQuery #1974

Merged

jmazanec15 mentioned this pull request Aug 21, 2024

Add support for qframework from interface #1994

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds rescore parameter to KNNQuery #1969

Adds rescore parameter to KNNQuery #1969

jmazanec15 commented Aug 15, 2024

navneet1v Aug 15, 2024

jmazanec15 Aug 15, 2024

shatejas Aug 15, 2024

shatejas Aug 15, 2024

shatejas Aug 15, 2024

jmazanec15 Aug 15, 2024

ryanbogan left a comment

shatejas Aug 15, 2024

jmazanec15 Aug 15, 2024

Vikasht34 left a comment

		@@ -776,19 +777,23 @@ public void testDoToQuery_InvalidZeroByteVector() {

		public void testSerialization() throws Exception {

Adds rescore parameter to KNNQuery #1969

Adds rescore parameter to KNNQuery #1969

Conversation

jmazanec15 commented Aug 15, 2024

Description

Related Issues

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbogan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vikasht34 left a comment

Choose a reason for hiding this comment