Add support for search using the "fields" parameter with knn_vector field #2314

e-emoto · 2024-12-07T03:39:16Z

Description

This change implements valueFetcher in KNNVectorFieldType so search can support using the "fields" parameter when used with the knn_vector field type. ITs for this change were added to OpenSearchIT after the change was tested manually.

Manual Testing Output

curl -X PUT "localhost:9200/test_index" -H 'Content-Type: application/json' -d' 
{
  "settings" : {
    "number_of_shards" : 1,
    "number_of_replicas" : 0,
    "index.knn": true
  },
  "mappings": {
       "properties": {
       "test_field1": {
           "type": "knn_vector",
           "dimension": 2,
           "method": {
               "name":"hnsw",
               "engine":"nmslib",
               "space_type": "l2",
               "parameters":{
                 "m":20,
                 "ef_construction": 23
               }
           }
      },"test_field2": {
           "type": "knn_vector",
           "dimension": 3,
           "method": {
               "name":"hnsw",
               "engine":"nmslib",
               "space_type": "l2",
               "parameters":{
                 "m":20,
                 "ef_construction": 23
               }
           }
      },"test_field3": {
           "type": "text"
      }
   }
  }
}
'

curl -X PUT "localhost:9200/_bulk" -H 'Content-Type: application/json' -d'
{ "index": { "_index": "test_index" } }
{ "test_field1": [1.5, 5.5], "test_field2": [1.5, 5.5, 3.5], "test_field3": "text 1"}
{ "index": { "_index": "test_index" } }
{ "test_field1": [2.5, 3.5], "test_field2": [2.5, 3.5, 7.5], "test_field3": "text 2"}
{ "index": { "_index": "test_index" } }
{ "test_field1": [4.5, 5.5], "test_field2": [4.5, 5.5, 1.5]}
{ "index": { "_index": "test_index" } }
{ "test_field1": [1.5, 5.5], "test_field2": [1.5, 5.5, 2.5]}
{ "index": { "_index": "test_index" } }
{ "test_field3": "text 3"}
{ "index": { "_index": "test_index" } }
{ "test_field3": "text 4"}
'

curl -X GET "localhost:9200/test_index/_search?pretty" -H 'Content-Type: application/json' -d'
{
  "fields": ["test_field1"],
  "query": {
    "knn": {
      "test_field1": {
        "vector": [2.0, 4.0],
        "k": 10
      }
    }
  }
}
'

{
  "took" : 4,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 4,
      "relation" : "eq"
    },
    "max_score" : 0.6666667,
    "hits" : [
      {
        "_index" : "test_index",
        "_id" : "6lOiwpMB0RzOL9-Fc3K5",
        "_score" : 0.6666667,
        "_source" : {
          "test_field1" : [
            2.5,
            3.5
          ],
          "test_field2" : [
            2.5,
            3.5,
            7.5
          ],
          "test_field3" : "text 2"
        },
        "fields" : {
          "test_field1" : [
            2.5,
            3.5
          ]
        }
      },
      {
        "_index" : "test_index",
        "_id" : "6VOiwpMB0RzOL9-Fc3K5",
        "_score" : 0.2857143,
        "_source" : {
          "test_field1" : [
            1.5,
            5.5
          ],
          "test_field2" : [
            1.5,
            5.5,
            3.5
          ],
          "test_field3" : "text 1"
        },
        "fields" : {
          "test_field1" : [
            1.5,
            5.5
          ]
        }
      },
      {
        "_index" : "test_index",
        "_id" : "7FOiwpMB0RzOL9-Fc3K5",
        "_score" : 0.2857143,
        "_source" : {
          "test_field1" : [
            1.5,
            5.5
          ],
          "test_field2" : [
            1.5,
            5.5,
            2.5
          ]
        },
        "fields" : {
          "test_field1" : [
            1.5,
            5.5
          ]
        }
      },
      {
        "_index" : "test_index",
        "_id" : "61OiwpMB0RzOL9-Fc3K5",
        "_score" : 0.10526316,
        "_source" : {
          "test_field1" : [
            4.5,
            5.5
          ],
          "test_field2" : [
            4.5,
            5.5,
            1.5
          ]
        },
        "fields" : {
          "test_field1" : [
            4.5,
            5.5
          ]
        }
      }
    ]
  }
}

Related Issues

Resolves #1633

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.

Signed-off-by: Ethan Emoto <[email protected]>

navneet1v · 2024-12-08T04:27:02Z

@e-emoto please fix the failing CIs

Signed-off-by: Ethan Emoto <[email protected]>

src/test/java/org/opensearch/knn/index/OpenSearchIT.java

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldType.java

Signed-off-by: Ethan Emoto <[email protected]>

VijayanB · 2024-12-10T03:56:02Z

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldType.java

@@ -51,7 +52,12 @@ public KNNVectorFieldType(String name, Map<String, String> metadata, VectorDataT

    @Override
    public ValueFetcher valueFetcher(QueryShardContext context, SearchLookup searchLookup, String format) {
-        throw new UnsupportedOperationException("KNN Vector do not support fields search");


What does this format variable do? Do we have to add any check for that?

Also do you see problem with backward compatibility? Does this work for index created before 2.19?

What does this format variable do? Do we have to add any check for that?

I'm not sure exactly what it does, in my testing it was always null when valueFetcher was called for KNNVectorFieldType.

Also do you see problem with backward compatibility? Does this work for index created before 2.19?

I don't see any reason why it would have issues with backward compatibility, but how would I test this to confirm it?

Since this is change in query path, I don't see any reason why BWC will fail.

src/test/java/org/opensearch/knn/index/OpenSearchIT.java

Signed-off-by: Ethan Emoto <[email protected]>

navneet1v · 2024-12-12T08:14:18Z

src/test/java/org/opensearch/knn/index/OpenSearchIT.java

+        XContentBuilder builder = XContentFactory.jsonBuilder()
+            .startObject()
+            .field("fields", new String[] { "*" })
+            .startObject("query")
+            .startObject("match_all")


why we are using a match all query here? Since a query is a query and it will solve the purpose, I would recommend having both match_all and a k-NN query to ensure that all cases are covered.

navneet1v · 2024-12-12T08:19:03Z

ITs for this change were added to OpenSearchIT after the change was tested manually.

can you add the manual testing result on the PR?

navneet1v · 2024-12-12T08:14:41Z

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldType.java

@@ -51,7 +52,12 @@ public KNNVectorFieldType(String name, Map<String, String> metadata, VectorDataT

    @Override
    public ValueFetcher valueFetcher(QueryShardContext context, SearchLookup searchLookup, String format) {
-        throw new UnsupportedOperationException("KNN Vector do not support fields search");
+        return new ArraySourceValueFetcher(name(), context) {
+            @Override


Please add a UT for this.

navneet1v · 2024-12-12T08:15:33Z

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldType.java

@@ -51,7 +52,12 @@ public KNNVectorFieldType(String name, Map<String, String> metadata, VectorDataT

    @Override
    public ValueFetcher valueFetcher(QueryShardContext context, SearchLookup searchLookup, String format) {
-        throw new UnsupportedOperationException("KNN Vector do not support fields search");


Since this is change in query path, I don't see any reason why BWC will fail.

navneet1v · 2024-12-12T08:25:10Z

src/test/java/org/opensearch/knn/index/OpenSearchIT.java

@@ -900,6 +901,136 @@ public void testKNNIndex_whenBuildVectorGraphThresholdIsProvidedEndToEnd_thenBui
        deleteKNNIndex(indexName);
    }

+    public void testKNNIndexSearchFieldsParameter() throws Exception {


Also lets add an IT test where some the docs don't have vector field and some have vector field in it.

Signed-off-by: Ethan Emoto <[email protected]>

jmazanec15 · 2025-01-08T22:35:59Z

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldType.java

+        return new ArraySourceValueFetcher(name(), context) {
+            @Override
+            protected Object parseSourceValue(Object value) {
+                if (value instanceof ArrayList) {


For this, I am not sure what is exactly expected from this method. Im thinking maybe we follow what the completion mapper did and if its not a list, just wrap it in a list: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/mapper/CompletionFieldMapper.java#L394-L398.

@shatejas @navneet1v what do you guys think?

Add support for fields parameter with knn_vector field

b49e6b2

Signed-off-by: Ethan Emoto <[email protected]>

e-emoto requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan, luyuncheng and shatejas as code owners December 7, 2024 03:39

e-emoto and others added 2 commits December 9, 2024 10:07

Fix spotless formatting

064fdbc

Signed-off-by: Ethan Emoto <[email protected]>

Update CHANGELOG.md

57c41e5

Signed-off-by: Ethan Emoto <[email protected]>

martin-gaievski reviewed Dec 9, 2024

View reviewed changes

src/test/java/org/opensearch/knn/index/OpenSearchIT.java Outdated Show resolved Hide resolved

martin-gaievski reviewed Dec 9, 2024

View reviewed changes

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

Fix import statements

0dc9f1b

Signed-off-by: Ethan Emoto <[email protected]>

VijayanB reviewed Dec 10, 2024

View reviewed changes

src/test/java/org/opensearch/knn/index/OpenSearchIT.java Outdated Show resolved Hide resolved

VijayanB reviewed Dec 10, 2024

View reviewed changes

src/test/java/org/opensearch/knn/index/OpenSearchIT.java Outdated Show resolved Hide resolved

Adjust/fix tests based on comments

c595c3d

Signed-off-by: Ethan Emoto <[email protected]>

navneet1v reviewed Dec 12, 2024

View reviewed changes

e-emoto and others added 3 commits December 13, 2024 10:49

Add more to tests based on comments

d8c10f9

Signed-off-by: Ethan Emoto <[email protected]>

Apply spotlessJava

1c26853

Signed-off-by: Ethan Emoto <[email protected]>

Merge branch 'main' into fields-parameter-support

2afb483

Signed-off-by: Ethan Emoto <[email protected]>

e-emoto requested a review from martin-gaievski December 27, 2024 21:25

e-emoto requested review from navneet1v and VijayanB December 27, 2024 21:25

Modify parseSourceValue for valueFetcher

28225e0

Signed-off-by: Ethan Emoto <[email protected]>

e-emoto requested a review from 0ctopus13prime as a code owner January 8, 2025 21:51

Merge branch 'main' into fields-parameter-support

c4094b0

Signed-off-by: Ethan Emoto <[email protected]>

jmazanec15 reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for search using the "fields" parameter with knn_vector field #2314

Add support for search using the "fields" parameter with knn_vector field #2314

e-emoto commented Dec 7, 2024 •

edited

Loading

navneet1v commented Dec 8, 2024

VijayanB Dec 10, 2024

VijayanB Dec 10, 2024

e-emoto Dec 11, 2024

navneet1v Dec 12, 2024

navneet1v Dec 12, 2024

navneet1v commented Dec 12, 2024

navneet1v Dec 12, 2024

navneet1v Dec 12, 2024

navneet1v Dec 12, 2024

jmazanec15 Jan 8, 2025

Add support for search using the "fields" parameter with knn_vector field #2314

Are you sure you want to change the base?

Add support for search using the "fields" parameter with knn_vector field #2314

Conversation

e-emoto commented Dec 7, 2024 • edited Loading

Description

Manual Testing Output

Related Issues

Check List

navneet1v commented Dec 8, 2024

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

navneet1v commented Dec 12, 2024

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

e-emoto commented Dec 7, 2024 •

edited

Loading