Allow validation for non knn index only after 2.17.0

Before 2.17.0, non knn index are allowed to create knn_vector fields with model id and method params, however this is not allowed from 2.17.0. Hence, to maintain backward compatibility we will not add validation for any indices created before 2.17.0. Signed-off-by: Vijayan Balasubramanian <[email protected]>
opensearch-project · Dec 7, 2024 · 5ede283 · 5ede283
1 parent 9276c77
commit 5ede283
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Introduced a writing layer in native engines where relies on the writing interface to process IO. (#2241)[https://github.com/opensearch-project/k-NN/pull/2241]
 ### Bug Fixes
 * Fixing the bug when a segment has no vector field present for disk based vector search (#2282)[https://github.com/opensearch-project/k-NN/pull/2282]
+* Allow validation for non knn index only after 2.17.0 (#2315)[https://github.com/opensearch-project/k-NN/pull/2315]
 ### Infrastructure
 * Updated C++ version in JNI from c++11 to c++17 [#2259](https://github.com/opensearch-project/k-NN/pull/2259)
 * Upgrade bytebuddy and objenesis version to match OpenSearch core and, update github ci runner for macos [#2279](https://github.com/opensearch-project/k-NN/pull/2279)

diff --git a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java
@@ -262,7 +262,10 @@ public KNNVectorFieldMapper build(BuilderContext context) {
                 );
             }
 
-            if (originalParameters.getResolvedKnnMethodContext() == null) {
+            // return FlatVectorFieldMapper only for indices that are created on or after 2.17.0, for others, use either LuceneFieldMapper
+            // or
+            // MethodFieldMapper to maintain backwards compatibility
+            if (originalParameters.getResolvedKnnMethodContext() == null && context.indexCreatedVersion().onOrAfter(Version.V_2_17_0)) {
                 return FlatVectorFieldMapper.createFieldMapper(
                     buildFullName(context),
                     name,
@@ -375,8 +378,8 @@ public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserCont
                 );
             }
 
-            // Check for flat configuration
-            if (isKNNDisabled(parserContext.getSettings())) {
+            // Check for flat configuration and validate only if index is created after 2.17
+            if (isKNNDisabled(parserContext.getSettings()) && parserContext.indexVersionCreated().onOrAfter(Version.V_2_17_0)) {
                 validateFromFlat(builder);
             } else if (builder.modelId.get() != null) {
                 validateFromModel(builder);

diff --git a/src/test/java/org/opensearch/knn/index/OpenSearchIT.java b/src/test/java/org/opensearch/knn/index/OpenSearchIT.java
@@ -814,6 +814,30 @@ public void testKNNIndex_whenBuildVectorDataStructureIsLessThanDocCount_thenBuil
         deleteKNNIndex(indexName);
     }
 
+    public void testCreateNonKNNIndex_withKNNModelID_throwsException() throws Exception {
+        Settings settings = Settings.builder().put(createKNNDefaultScriptScoreSettings()).build();
+        ResponseException ex = expectThrows(
+            ResponseException.class,
+            () -> createKnnIndex(INDEX_NAME, settings, createKnnIndexMapping(FIELD_NAME, "random-model-id"))
+        );
+        String expMessage = "Cannot set modelId or method parameters when index.knn setting is false";
+        assertThat(EntityUtils.toString(ex.getResponse().getEntity()), containsString(expMessage));
+    }
+
+    public void testCreateNonKNNIndex_withKNNMethodParams_throwsException() throws Exception {
+        Settings settings = Settings.builder().put(createKNNDefaultScriptScoreSettings()).build();
+        ResponseException ex = expectThrows(
+            ResponseException.class,
+            () -> createKnnIndex(
+                INDEX_NAME,
+                settings,
+                createKnnIndexMapping(FIELD_NAME, 2, "hnsw", KNNEngine.FAISS.getName(), SpaceType.DEFAULT.getValue(), false)
+            )
+        );
+        String expMessage = "Cannot set modelId or method parameters when index.knn setting is false";
+        assertThat(EntityUtils.toString(ex.getResponse().getEntity()), containsString(expMessage));
+    }
+
     /*
       For this testcase, we will create index with setting build_vector_data_structure_threshold as -1, then index few documents, perform knn search,
       then, confirm hits because of exact search though there are no graph. In next step, update setting to 0, force merge segment to 1, perform knn search and confirm expected

diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java
@@ -396,6 +396,22 @@ protected String createKnnIndexMapping(String fieldName, Integer dimensions) thr
             .toString();
     }
 
+    /**
+     * Utility to create a Knn Index Mapping with model id
+     */
+    protected String createKnnIndexMapping(String fieldName, String modelId) throws IOException {
+        return XContentFactory.jsonBuilder()
+            .startObject()
+            .startObject("properties")
+            .startObject(fieldName)
+            .field("type", "knn_vector")
+            .field("model_id", modelId)
+            .endObject()
+            .endObject()
+            .endObject()
+            .toString();
+    }
+
     protected String createKnnIndexMapping(final String fieldName, final Integer dimensions, final VectorDataType vectorDataType)
         throws IOException {
         return XContentFactory.jsonBuilder()