Fixed a bug that does not get a space type from index setting when it…

… is empty for compatibility. Signed-off-by: Dooyong Kim <[email protected]>
opensearch-project · Dec 4, 2024 · ca39eeb · ca39eeb
1 parent 9276c77
commit ca39eeb
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 44 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]
+* Fixing it to retrieve space_type from index setting when both method and top level don't have the value. [#2309](https://github.com/opensearch-project/k-NN/pull/2309)
 ### 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/engine/SpaceTypeResolver.java b/src/main/java/org/opensearch/knn/index/engine/SpaceTypeResolver.java
@@ -42,7 +42,7 @@ public SpaceType resolveSpaceType(
         SpaceType topLevelSpaceType = getSpaceTypeFromString(topLevelSpaceTypeString);
 
         if (isSpaceTypeConfigured(methodSpaceType) == false && isSpaceTypeConfigured(topLevelSpaceType) == false) {
-            return getSpaceTypeFromVectorDataType(vectorDataType);
+            return SpaceType.UNDEFINED;
         }
 
         if (isSpaceTypeConfigured(methodSpaceType) == false) {
@@ -75,13 +75,6 @@ private SpaceType getSpaceTypeFromMethodContext(final KNNMethodContext knnMethod
         return knnMethodContext.getSpaceType();
     }
 
-    private SpaceType getSpaceTypeFromVectorDataType(final VectorDataType vectorDataType) {
-        if (vectorDataType == VectorDataType.BINARY) {
-            return SpaceType.DEFAULT_BINARY;
-        }
-        return SpaceType.DEFAULT;
-    }
-
     private SpaceType getSpaceTypeFromString(final String spaceType) {
         if (Strings.isEmpty(spaceType)) {
             return SpaceType.UNDEFINED;
@@ -93,4 +86,15 @@ private SpaceType getSpaceTypeFromString(final String spaceType) {
     private boolean isSpaceTypeConfigured(final SpaceType spaceType) {
         return spaceType != null && spaceType != SpaceType.UNDEFINED;
     }
+
+    public SpaceType pickDefaultSpaceTypeWhenEmpty(SpaceType resolvedSpaceType, VectorDataType vectorDataType) {
+        if (resolvedSpaceType != SpaceType.UNDEFINED) {
+            return resolvedSpaceType;
+        } else {
+            if (vectorDataType == VectorDataType.BINARY) {
+                return SpaceType.DEFAULT_BINARY;
+            }
+            return SpaceType.DEFAULT;
+        }
+    }
 }
diff --git a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java
@@ -387,14 +387,17 @@ public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserCont
                 // If the original knnMethodContext is not null, resolve its space type and engine from the rest of the
                 // configuration. This is consistent with the existing behavior for space type in 2.16 where we modify the
                 // parsed value
-                SpaceType resolvedSpaceType = SpaceTypeResolver.INSTANCE.resolveSpaceType(
+                final SpaceType resolvedSpaceType = SpaceTypeResolver.INSTANCE.resolveSpaceType(
                     builder.originalParameters.getKnnMethodContext(),
                     builder.vectorDataType.get(),
                     builder.topLevelSpaceType.get()
                 );
-                setSpaceType(builder.originalParameters.getKnnMethodContext(), resolvedSpaceType);
+                setSpaceType(
+                    builder.originalParameters.getKnnMethodContext(),
+                    SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, builder.vectorDataType.get())
+                );
                 validateSpaceType(builder);
-                resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType);
+                resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType, builder.vectorDataType.get());
                 validateFromKNNMethod(builder);
             }
 
@@ -485,9 +488,10 @@ private void validateCompressionAndModeNotSet(KNNVectorFieldMapper.Builder build
         }
 
         private void resolveKNNMethodComponents(
-            KNNVectorFieldMapper.Builder builder,
+            Builder builder,
             ParserContext parserContext,
-            SpaceType resolvedSpaceType
+            SpaceType resolvedSpaceType,
+            VectorDataType vectorDataType
         ) {
             // Setup the initial configuration that is used to help resolve parameters.
             builder.setKnnMethodConfigContext(
@@ -520,7 +524,7 @@ private void resolveKNNMethodComponents(
                 builder.originalParameters.getResolvedKnnMethodContext(),
                 builder.knnMethodConfigContext,
                 false,
-                resolvedSpaceType
+                SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, vectorDataType)
             );
 
             // The original parameters stores both the resolveMethodContext as well as the original provided by the

diff --git a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java
@@ -199,9 +199,7 @@ static KNNMethodContext createKNNMethodContextFromLegacy(
         SpaceType topLevelSpaceType
     ) {
         // If top level spaceType is set then use that spaceType otherwise default to spaceType from index-settings
-        final SpaceType finalSpaceToSet = topLevelSpaceType != SpaceType.UNDEFINED
-            ? topLevelSpaceType
-            : KNNVectorFieldMapperUtil.getSpaceType(indexSettings);
+        final SpaceType finalSpaceToSet = topLevelSpaceType != SpaceType.UNDEFINED ? topLevelSpaceType : getSpaceType(indexSettings);
         return new KNNMethodContext(
             KNNEngine.NMSLIB,
             finalSpaceToSet,

diff --git a/src/main/java/org/opensearch/knn/plugin/rest/RestTrainModelHandler.java b/src/main/java/org/opensearch/knn/plugin/rest/RestTrainModelHandler.java
@@ -166,6 +166,7 @@ && ensureSpaceTypeNotSet(topLevelSpaceType)) {
             vectorDataType,
             topLevelSpaceType.getValue()
         );
+        resolvedSpaceType = SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, vectorDataType);
         setSpaceType(knnMethodContext, resolvedSpaceType);
         TrainingModelRequest trainingModelRequest = new TrainingModelRequest(
             modelId,

diff --git a/src/test/java/org/opensearch/knn/index/engine/SpaceTypeResolverTests.java b/src/test/java/org/opensearch/knn/index/engine/SpaceTypeResolverTests.java
@@ -16,23 +16,47 @@ public class SpaceTypeResolverTests extends KNNTestCase {
     private static final SpaceTypeResolver SPACE_TYPE_RESOLVER = SpaceTypeResolver.INSTANCE;
 
     public void testResolveSpaceType_whenNoConfigProvided_thenFallbackToVectorDataType() {
-        assertEquals(SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.FLOAT, ""));
-        assertEquals(SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BYTE, ""));
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                ""
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.FLOAT, ""),
+                VectorDataType.FLOAT
+            )
+        );
+        assertEquals(
+            SpaceType.DEFAULT,
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BYTE, ""),
+                VectorDataType.FLOAT
+            )
+        );
+        assertEquals(
+            SpaceType.DEFAULT,
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    ""
+                ),
+                VectorDataType.FLOAT
             )
         );
-        assertEquals(SpaceType.DEFAULT_BINARY, SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BINARY, ""));
         assertEquals(
             SpaceType.DEFAULT_BINARY,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.BINARY,
-                ""
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BINARY, ""),
+                VectorDataType.BINARY
+            )
+        );
+        assertEquals(
+            SpaceType.DEFAULT_BINARY,
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.BINARY,
+                    ""
+                ),
+                VectorDataType.BINARY
             )
         );
     }
@@ -49,34 +73,46 @@ public void testResolveSpaceType_whenMethodSpaceTypeAndTopLevelSpecified_thenThr
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.DEFAULT.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.DEFAULT.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.UNDEFINED.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.UNDEFINED.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.DEFAULT.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.DEFAULT.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.UNDEFINED.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.UNDEFINED.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
     }