From ca39eeb1255e41f8f24c5b7940cb381a1f39e072 Mon Sep 17 00:00:00 2001 From: Dooyong Kim Date: Tue, 3 Dec 2024 16:29:41 -0800 Subject: [PATCH] Fixed a bug that does not get a space type from index setting when it is empty for compatibility. Signed-off-by: Dooyong Kim --- CHANGELOG.md | 1 + .../knn/index/engine/SpaceTypeResolver.java | 20 +++-- .../index/mapper/KNNVectorFieldMapper.java | 16 ++-- .../mapper/KNNVectorFieldMapperUtil.java | 4 +- .../plugin/rest/RestTrainModelHandler.java | 1 + .../index/engine/SpaceTypeResolverTests.java | 90 +++++++++++++------ 6 files changed, 88 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03a8d7974..e093da016 100644 --- 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 index a12ffbc7b..dbfec33d0 100644 --- 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 index beceadde5..2a8a2b3ee 100644 --- 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 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 index b3727f2ef..607bc476a 100644 --- 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 index 4380310c3..71ac87c79 100644 --- 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 index 99fc98c9e..71e3336d2 100644 --- 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 ) ); }