diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a2697fc9..e199e76f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), * Release query vector memory after execution (#2346)[https://github.com/opensearch-project/k-NN/pull/2346] * Fix shard level rescoring disabled setting flag (#2352)[https://github.com/opensearch-project/k-NN/pull/2352] * Fix filter rewrite logic which was resulting in getting inconsistent / incorrect results for cases where filter was getting rewritten for shards (#2359)[https://github.com/opensearch-project/k-NN/pull/2359] +* Fixing it to retrieve space_type from index setting when both method and top level don't have the value. [#2374](https://github.com/opensearch-project/k-NN/pull/2374) ### 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/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java index edccada44..f7eea9ef1 100644 --- a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java +++ b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java @@ -263,14 +263,14 @@ public void testKNNIndexBinaryForceMerge() throws Exception { } // Custom Legacy Field Mapping - // space_type : "linf", engine : "nmslib", m : 2, ef_construction : 2 + // space_type : "innerproduct", engine : "nmslib", m : 2, ef_construction : 2 public void testKNNIndexCustomLegacyFieldMapping() throws Exception { // When the cluster is in old version, create a KNN index with custom legacy field mapping settings // and add documents into that index if (isRunningAgainstOldCluster()) { Settings.Builder indexMappingSettings = createKNNIndexCustomLegacyFieldMappingIndexSettingsBuilder( - SpaceType.LINF, + SpaceType.INNER_PRODUCT, KNN_ALGO_PARAM_M_MIN_VALUE, KNN_ALGO_PARAM_EF_CONSTRUCTION_MIN_VALUE ); diff --git a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/ScriptScoringIT.java b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/ScriptScoringIT.java index df530776d..1c4a69c76 100644 --- a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/ScriptScoringIT.java +++ b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/ScriptScoringIT.java @@ -65,22 +65,6 @@ public void testKNNL1ScriptScore() throws Exception { } } - // KNN script scoring for space_type "linf" - public void testKNNLinfScriptScore() throws Exception { - if (isRunningAgainstOldCluster()) { - createKnnIndex(testIndex, createKNNDefaultScriptScoreSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS)); - addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, DOC_ID, NUM_DOCS); - } else { - QUERY_COUNT = NUM_DOCS; - DOC_ID = NUM_DOCS; - validateKNNScriptScoreSearch(testIndex, TEST_FIELD, DIMENSIONS, QUERY_COUNT, K, SpaceType.LINF); - addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, DOC_ID, NUM_DOCS); - QUERY_COUNT = QUERY_COUNT + NUM_DOCS; - validateKNNScriptScoreSearch(testIndex, TEST_FIELD, DIMENSIONS, QUERY_COUNT, K, SpaceType.LINF); - deleteKNNIndex(testIndex); - } - } - // KNN script scoring for space_type "innerproduct" public void testKNNInnerProductScriptScore() throws Exception { if (isRunningAgainstOldCluster()) { diff --git a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/WarmupIT.java b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/WarmupIT.java index 64c55f6fd..3daf8c0a1 100644 --- a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/WarmupIT.java +++ b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/WarmupIT.java @@ -45,14 +45,14 @@ public void testKNNWarmupDefaultLegacyFieldMapping() throws Exception { } // Custom Legacy Field Mapping - // space_type : "linf", engine : "nmslib", m : 2, ef_construction : 2 + // space_type : "innerproduct", engine : "nmslib", m : 2, ef_construction : 2 public void testKNNWarmupCustomLegacyFieldMapping() throws Exception { // When the cluster is in old version, create a KNN index with custom legacy field mapping settings // and add documents into that index if (isRunningAgainstOldCluster()) { Settings.Builder indexMappingSettings = createKNNIndexCustomLegacyFieldMappingIndexSettingsBuilder( - SpaceType.LINF, + SpaceType.INNER_PRODUCT, KNN_ALGO_PARAM_M_MIN_VALUE, KNN_ALGO_PARAM_EF_CONSTRUCTION_MIN_VALUE ); 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..808bd39b8 100644 --- a/src/main/java/org/opensearch/knn/index/engine/SpaceTypeResolver.java +++ b/src/main/java/org/opensearch/knn/index/engine/SpaceTypeResolver.java @@ -6,7 +6,9 @@ package org.opensearch.knn.index.engine; import org.apache.logging.log4j.util.Strings; +import org.opensearch.common.settings.Settings; import org.opensearch.index.mapper.MapperParsingException; +import org.opensearch.knn.index.KNNSettings; import org.opensearch.knn.index.SpaceType; import org.opensearch.knn.index.VectorDataType; @@ -25,24 +27,35 @@ public final class SpaceTypeResolver { private SpaceTypeResolver() {} /** - * Resolves space type from configuration details. It is guaranteed not to return either null or - * {@link SpaceType#UNDEFINED} + * Resolves space type from configuration details. It is guaranteed not to return null. + * When space is not in either method and top level, UNDEFINED will be returned. + * This can happen when it is defined at index level which is deprecated and no longer allowed in the future. + * In this case, UNDEFINED will be returned. * - * @param knnMethodContext Method context - * @param vectorDataType Vectordatatype + * @param knnMethodContext Method context * @param topLevelSpaceTypeString Alternative top-level space type * @return {@link SpaceType} for the method */ public SpaceType resolveSpaceType( final KNNMethodContext knnMethodContext, - final VectorDataType vectorDataType, - final String topLevelSpaceTypeString + final String topLevelSpaceTypeString, + final Settings indexSettings, + final VectorDataType vectorDataType ) { SpaceType methodSpaceType = getSpaceTypeFromMethodContext(knnMethodContext); SpaceType topLevelSpaceType = getSpaceTypeFromString(topLevelSpaceTypeString); + // If we failed to find space type from both method context and top level + // 1. We try to get it from index setting, which is a relic of legacy. + // 2. Otherwise, we return a default one. if (isSpaceTypeConfigured(methodSpaceType) == false && isSpaceTypeConfigured(topLevelSpaceType) == false) { - return getSpaceTypeFromVectorDataType(vectorDataType); + if (indexSettings != null) { + final String spaceType = indexSettings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey()); + if (spaceType != null) { + return SpaceType.getSpace(spaceType); + } + } + return getDefaultSpaceType(vectorDataType); } if (isSpaceTypeConfigured(methodSpaceType) == false) { @@ -67,6 +80,13 @@ public SpaceType resolveSpaceType( ); } + public static SpaceType getDefaultSpaceType(final VectorDataType vectorDataType) { + if (vectorDataType == VectorDataType.BINARY) { + return SpaceType.DEFAULT_BINARY; + } + return SpaceType.DEFAULT; + } + private SpaceType getSpaceTypeFromMethodContext(final KNNMethodContext knnMethodContext) { if (knnMethodContext == null) { return SpaceType.UNDEFINED; @@ -75,13 +95,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; 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 67f3efa5b..c17abc2e1 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -390,13 +390,20 @@ 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() + builder.topLevelSpaceType.get(), + parserContext.getSettings(), + builder.vectorDataType.get() ); + + // Set space type to the original knnMethodContext. Since the resolved one can be UNDEFINED, + // we must wrap it and pick up the default when it is UNDEFINED. setSpaceType(builder.originalParameters.getKnnMethodContext(), resolvedSpaceType); validateSpaceType(builder); + + // Resolve method component. For the legacy case where space type can be configured at index level, + // it first tries to use the given one then tries to get it from index setting when the space type is UNDEFINED. resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType); validateFromKNNMethod(builder); } 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..a9ca56d4b 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java @@ -146,7 +146,7 @@ static boolean useLuceneKNNVectorsFormat(final Version indexCreatedVersion) { return indexCreatedVersion.onOrAfter(Version.V_2_17_0); } - private static SpaceType getSpaceType(final Settings indexSettings) { + public static SpaceType getSpaceType(final Settings indexSettings) { String spaceType = indexSettings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey()); if (spaceType == null) { spaceType = KNNSettings.INDEX_KNN_DEFAULT_SPACE_TYPE; @@ -196,15 +196,11 @@ private static int getEfConstruction(Settings indexSettings, Version indexVersio static KNNMethodContext createKNNMethodContextFromLegacy( Settings indexSettings, Version indexCreatedVersion, - SpaceType topLevelSpaceType + SpaceType resolvedSpaceType ) { - // 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); return new KNNMethodContext( KNNEngine.NMSLIB, - finalSpaceToSet, + resolvedSpaceType, new MethodComponentContext( METHOD_HNSW, Map.of( 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 837bb3f43..3c835af60 100644 --- a/src/main/java/org/opensearch/knn/plugin/rest/RestTrainModelHandler.java +++ b/src/main/java/org/opensearch/knn/plugin/rest/RestTrainModelHandler.java @@ -149,6 +149,11 @@ && ensureSpaceTypeNotSet(topLevelSpaceType)) { vectorDataType = VectorDataType.DEFAULT; } + if ((knnMethodContext == null || knnMethodContext.getSpaceType() == SpaceType.UNDEFINED) + && topLevelSpaceType == SpaceType.UNDEFINED) { + topLevelSpaceType = SpaceTypeResolver.getDefaultSpaceType(vectorDataType); + } + ensureIfSetThenEquals( MODE_PARAMETER, mode, @@ -161,8 +166,9 @@ && ensureSpaceTypeNotSet(topLevelSpaceType)) { ); SpaceType resolvedSpaceType = SpaceTypeResolver.INSTANCE.resolveSpaceType( knnMethodContext, - vectorDataType, - topLevelSpaceType.getValue() + topLevelSpaceType.getValue(), + null, + vectorDataType ); setSpaceType(knnMethodContext, resolvedSpaceType); TrainingModelRequest trainingModelRequest = new TrainingModelRequest( diff --git a/src/test/java/org/opensearch/knn/index/KNNESSettingsTestIT.java b/src/test/java/org/opensearch/knn/index/KNNESSettingsTestIT.java index 1253ed0e2..3687c261d 100644 --- a/src/test/java/org/opensearch/knn/index/KNNESSettingsTestIT.java +++ b/src/test/java/org/opensearch/knn/index/KNNESSettingsTestIT.java @@ -5,25 +5,91 @@ package org.opensearch.knn.index; -import org.opensearch.knn.KNNRestTestCase; -import org.opensearch.knn.index.query.KNNQueryBuilder; -import org.opensearch.knn.plugin.stats.StatNames; +import org.apache.hc.core5.http.ParseException; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.opensearch.client.Response; import org.opensearch.client.ResponseException; import org.opensearch.common.settings.Settings; +import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.rest.RestStatus; +import org.opensearch.knn.KNNRestTestCase; +import org.opensearch.knn.KNNResult; +import org.opensearch.knn.index.query.KNNQueryBuilder; +import org.opensearch.knn.plugin.stats.StatNames; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; +import java.util.List; import java.util.Map; +import java.util.Set; import static org.hamcrest.Matchers.containsString; +import static org.opensearch.knn.TestUtils.KNN_VECTOR; +import static org.opensearch.knn.TestUtils.PROPERTIES; +import static org.opensearch.knn.TestUtils.VECTOR_TYPE; +import static org.opensearch.knn.common.KNNConstants.DIMENSION; public class KNNESSettingsTestIT extends KNNRestTestCase { public static final int ALWAYS_BUILD_GRAPH = 0; + public void testKNNLegacySpaceTypeIndexingTest() throws IOException, ParseException { + // Configure space_type at index level. This is deprecated and will be removed in the future. + final Settings indexSettings = Settings.builder() + .put("index.knn", true) + .put("knn.algo_param.ef_search", 100) + .put("knn.space_type", SpaceType.INNER_PRODUCT.getValue()) + .build(); + + // This mapping does not have method. + final String testField = "knn_field"; + final String testIndex = "knn_index"; + final String mapping = XContentFactory.jsonBuilder() + .startObject() + .startObject(PROPERTIES) + .startObject(testField) + .field(VECTOR_TYPE, KNN_VECTOR) + .field(DIMENSION, 2) + .endObject() + .endObject() + .endObject() + .toString(); + + createKnnIndex(testIndex, indexSettings, mapping); + + // Ingest data. + float[] queryVector = new float[] { 2, 3 }; + final int k = 2; + + float[][] vectorData = new float[5][2]; + vectorData[0] = new float[] { 11.7f, 2.7f }; // distance=31.5 + vectorData[1] = new float[] { 20.9f, 3.9f }; // distance=53.5 <- answer + vectorData[2] = new float[] { 3.77f, 4.22f }; // distance=20.2 + vectorData[3] = new float[] { 15, 6 }; // distance=48 <- answer + vectorData[4] = new float[] { 4.7f, 5.9f }; // distance=27.1 + + bulkAddKnnDocs(testIndex, testField, vectorData, vectorData.length); + flushIndex(testIndex); + + // Send a query and verify scores are correct. + Response searchResponse = searchKNNIndex( + testIndex, + KNNQueryBuilder.builder().k(k).fieldName(testField).vector(queryVector).build(), + k + ); + + List results = parseSearchResponse(EntityUtils.toString(searchResponse.getEntity()), testField); + + assertEquals(k, results.size()); + Set docIds = new HashSet<>(); + for (KNNResult result : results) { + docIds.add(result.getDocId()); + } + assertEquals(new HashSet<>(Arrays.asList("1", "3")), docIds); + } + /** * KNN Index writes should be blocked when the plugin disabled * @throws Exception Exception from test 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..946dd7786 100644 --- a/src/test/java/org/opensearch/knn/index/engine/SpaceTypeResolverTests.java +++ b/src/test/java/org/opensearch/knn/index/engine/SpaceTypeResolverTests.java @@ -6,35 +6,202 @@ package org.opensearch.knn.index.engine; import lombok.SneakyThrows; +import org.opensearch.common.settings.Settings; import org.opensearch.index.mapper.MapperParsingException; import org.opensearch.knn.KNNTestCase; +import org.opensearch.knn.index.KNNSettings; import org.opensearch.knn.index.SpaceType; import org.opensearch.knn.index.VectorDataType; +import static org.opensearch.Version.CURRENT; +import static org.opensearch.knn.index.KNNSettings.KNN_INDEX; +import static org.opensearch.knn.index.KNNSettings.KNN_SPACE_TYPE; + public class SpaceTypeResolverTests extends KNNTestCase { private static final SpaceTypeResolver SPACE_TYPE_RESOLVER = SpaceTypeResolver.INSTANCE; + private static final Settings DONT_CARE_SETTINGS = null; + private static final VectorDataType DONT_CARE_VECTOR_DATA = null; - 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, "")); + private void assertResolveSpaceType( + KNNMethodContext knnMethodContext, + String topLevelSpaceTypeString, + Settings indexSettings, + VectorDataType vectorDataType, + SpaceType expectedSpaceType + ) { assertEquals( - SpaceType.DEFAULT, - SPACE_TYPE_RESOLVER.resolveSpaceType( - new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY), - VectorDataType.FLOAT, - "" - ) + expectedSpaceType, + SPACE_TYPE_RESOLVER.resolveSpaceType(knnMethodContext, topLevelSpaceTypeString, indexSettings, vectorDataType) ); - 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, - "" - ) + } + + public void testResolveSpaceType_whenNoConfigProvided_thenFallbackToVectorDataType() { + final Settings emptySettings = Settings.builder().put(settings(CURRENT).build()).put(KNN_INDEX, true).build(); + final Settings settings = Settings.builder() + .put(settings(CURRENT).build()) + .put(KNN_SPACE_TYPE, SpaceType.L2.getValue()) + .put(KNN_INDEX, true) + .build(); + + final KNNMethodContext methodContext = new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.COSINESIMIL, MethodComponentContext.EMPTY); + final KNNMethodContext emptyMethodContext = new KNNMethodContext( + KNNEngine.DEFAULT, + SpaceType.UNDEFINED, + MethodComponentContext.EMPTY + ); + final KNNMethodContext nullMethodContext = null; + + assertResolveSpaceType( + methodContext, + SpaceType.COSINESIMIL.getValue(), + settings, + VectorDataType.BYTE, + methodContext.getSpaceType() + ); + assertResolveSpaceType( + methodContext, + SpaceType.COSINESIMIL.getValue(), + settings, + VectorDataType.FLOAT, + methodContext.getSpaceType() + ); + assertResolveSpaceType( + methodContext, + SpaceType.COSINESIMIL.getValue(), + settings, + VectorDataType.BINARY, + methodContext.getSpaceType() + ); + assertResolveSpaceType( + methodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.BYTE, + methodContext.getSpaceType() + ); + assertResolveSpaceType( + methodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.FLOAT, + methodContext.getSpaceType() + ); + assertResolveSpaceType( + methodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.BINARY, + methodContext.getSpaceType() + ); + assertResolveSpaceType(methodContext, "", settings, VectorDataType.BYTE, methodContext.getSpaceType()); + assertResolveSpaceType(methodContext, "", settings, VectorDataType.FLOAT, methodContext.getSpaceType()); + assertResolveSpaceType(methodContext, "", settings, VectorDataType.BINARY, methodContext.getSpaceType()); + assertResolveSpaceType(methodContext, "", emptySettings, VectorDataType.BYTE, methodContext.getSpaceType()); + assertResolveSpaceType(methodContext, "", emptySettings, VectorDataType.FLOAT, methodContext.getSpaceType()); + assertResolveSpaceType(methodContext, "", emptySettings, VectorDataType.BINARY, methodContext.getSpaceType()); + assertResolveSpaceType(emptyMethodContext, SpaceType.COSINESIMIL.getValue(), settings, VectorDataType.BYTE, SpaceType.COSINESIMIL); + assertResolveSpaceType(emptyMethodContext, SpaceType.COSINESIMIL.getValue(), settings, VectorDataType.FLOAT, SpaceType.COSINESIMIL); + assertResolveSpaceType( + emptyMethodContext, + SpaceType.COSINESIMIL.getValue(), + settings, + VectorDataType.BINARY, + SpaceType.COSINESIMIL + ); + assertResolveSpaceType( + emptyMethodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.BYTE, + SpaceType.COSINESIMIL + ); + assertResolveSpaceType( + emptyMethodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.FLOAT, + SpaceType.COSINESIMIL + ); + assertResolveSpaceType( + emptyMethodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.BINARY, + SpaceType.COSINESIMIL + ); + assertResolveSpaceType( + emptyMethodContext, + "", + settings, + VectorDataType.BYTE, + SpaceType.getSpace(settings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey())) ); + assertResolveSpaceType( + emptyMethodContext, + "", + settings, + VectorDataType.FLOAT, + SpaceType.getSpace(settings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey())) + ); + assertResolveSpaceType( + emptyMethodContext, + "", + settings, + VectorDataType.BINARY, + SpaceType.getSpace(settings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey())) + ); + assertResolveSpaceType(emptyMethodContext, "", emptySettings, VectorDataType.BYTE, SpaceType.DEFAULT); + assertResolveSpaceType(emptyMethodContext, "", emptySettings, VectorDataType.FLOAT, SpaceType.DEFAULT); + assertResolveSpaceType(emptyMethodContext, "", emptySettings, VectorDataType.BINARY, SpaceType.DEFAULT_BINARY); + assertResolveSpaceType(nullMethodContext, SpaceType.COSINESIMIL.getValue(), settings, VectorDataType.BYTE, SpaceType.COSINESIMIL); + assertResolveSpaceType(nullMethodContext, SpaceType.COSINESIMIL.getValue(), settings, VectorDataType.FLOAT, SpaceType.COSINESIMIL); + assertResolveSpaceType(nullMethodContext, SpaceType.COSINESIMIL.getValue(), settings, VectorDataType.BINARY, SpaceType.COSINESIMIL); + assertResolveSpaceType( + nullMethodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.BYTE, + SpaceType.COSINESIMIL + ); + assertResolveSpaceType( + nullMethodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.FLOAT, + SpaceType.COSINESIMIL + ); + assertResolveSpaceType( + nullMethodContext, + SpaceType.COSINESIMIL.getValue(), + emptySettings, + VectorDataType.BINARY, + SpaceType.COSINESIMIL + ); + assertResolveSpaceType( + nullMethodContext, + "", + settings, + VectorDataType.BYTE, + SpaceType.getSpace(settings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey())) + ); + assertResolveSpaceType( + nullMethodContext, + "", + settings, + VectorDataType.FLOAT, + SpaceType.getSpace(settings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey())) + ); + assertResolveSpaceType( + nullMethodContext, + "", + settings, + VectorDataType.BINARY, + SpaceType.getSpace(settings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey())) + ); + assertResolveSpaceType(nullMethodContext, "", emptySettings, VectorDataType.BYTE, SpaceType.DEFAULT); + assertResolveSpaceType(nullMethodContext, "", emptySettings, VectorDataType.FLOAT, SpaceType.DEFAULT); + assertResolveSpaceType(nullMethodContext, "", emptySettings, VectorDataType.BINARY, SpaceType.DEFAULT_BINARY); } @SneakyThrows @@ -43,40 +210,69 @@ public void testResolveSpaceType_whenMethodSpaceTypeAndTopLevelSpecified_thenThr MapperParsingException.class, () -> SPACE_TYPE_RESOLVER.resolveSpaceType( new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.L2, MethodComponentContext.EMPTY), - VectorDataType.FLOAT, - SpaceType.INNER_PRODUCT.getValue() + SpaceType.INNER_PRODUCT.getValue(), + DONT_CARE_SETTINGS, + DONT_CARE_VECTOR_DATA ) ); assertEquals( SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType( new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY), - VectorDataType.FLOAT, - SpaceType.DEFAULT.getValue() + SpaceType.DEFAULT.getValue(), + DONT_CARE_SETTINGS, + DONT_CARE_VECTOR_DATA ) ); assertEquals( SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType( new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY), - VectorDataType.FLOAT, - SpaceType.UNDEFINED.getValue() + SpaceType.UNDEFINED.getValue(), + DONT_CARE_SETTINGS, + DONT_CARE_VECTOR_DATA + ) + ); + assertEquals( + SpaceType.DEFAULT, + SPACE_TYPE_RESOLVER.resolveSpaceType( + new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY), + SpaceType.DEFAULT.getValue(), + DONT_CARE_SETTINGS, + DONT_CARE_VECTOR_DATA ) ); + + final Settings settings = Settings.builder().put(settings(CURRENT).build()).put(KNN_INDEX, true).build(); + + // method (undefined) -> top level (undefined) -> settings (undefined) -> Default Space Type assertEquals( SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType( new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY), - VectorDataType.FLOAT, - SpaceType.DEFAULT.getValue() + SpaceType.UNDEFINED.getValue(), + settings, + VectorDataType.BYTE ) ); + assertEquals( SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType( new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY), - VectorDataType.FLOAT, - SpaceType.UNDEFINED.getValue() + SpaceType.UNDEFINED.getValue(), + settings, + VectorDataType.FLOAT + ) + ); + + assertEquals( + SpaceType.DEFAULT_BINARY, + SPACE_TYPE_RESOLVER.resolveSpaceType( + new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY), + SpaceType.UNDEFINED.getValue(), + settings, + VectorDataType.BINARY ) ); } @@ -87,13 +283,11 @@ public void testResolveSpaceType_whenSpaceTypeSpecifiedOnce_thenReturnValue() { SpaceType.L1, SPACE_TYPE_RESOLVER.resolveSpaceType( new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.L1, MethodComponentContext.EMPTY), - VectorDataType.FLOAT, - "" + "", + null, + null ) ); - assertEquals( - SpaceType.INNER_PRODUCT, - SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.FLOAT, SpaceType.INNER_PRODUCT.getValue()) - ); + assertEquals(SpaceType.INNER_PRODUCT, SPACE_TYPE_RESOLVER.resolveSpaceType(null, SpaceType.INNER_PRODUCT.getValue(), null, null)); } } diff --git a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java index 9e637be9b..49b15a0f4 100644 --- a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java +++ b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java @@ -399,6 +399,45 @@ public void testBuilder_build_fromModel() { assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().isEmpty()); } + public void testSpaceType_build_fromLegacy() throws IOException { + // Check legacy is picked up if model context and method context are not set + ModelDao modelDao = mock(ModelDao.class); + int m = 17; + int efConstruction = 17; + KNNVectorFieldMapper.TypeParser typeParser = new KNNVectorFieldMapper.TypeParser(() -> modelDao); + + XContentBuilder xContentBuilder = XContentFactory.jsonBuilder() + .startObject() + .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) + .field(DIMENSION_FIELD_NAME, 12) + .endObject(); + + Settings settings = Settings.builder() + .put(settings(CURRENT).build()) + .put(KNNSettings.KNN_ALGO_PARAM_M, m) + .put(KNNSettings.KNN_ALGO_PARAM_EF_CONSTRUCTION, efConstruction) + .put(KNNSettings.KNN_SPACE_TYPE, SpaceType.INNER_PRODUCT.getValue()) + .put(KNN_INDEX, true) + .build(); + + KNNVectorFieldMapper.Builder builder = (KNNVectorFieldMapper.Builder) typeParser.parse( + "test-field-name-1", + xContentBuilderToMap(xContentBuilder), + buildLegacyParserContext("test", settings, Version.V_2_15_0) + ); + + // Setup settings + Mapper.BuilderContext builderContext = new Mapper.BuilderContext(settings, new ContentPath()); + KNNVectorFieldMapper knnVectorFieldMapper = builder.build(builderContext); + assertTrue(knnVectorFieldMapper instanceof MethodFieldMapper); + assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().isPresent()); + assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); + assertEquals( + SpaceType.INNER_PRODUCT, + knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().get().getSpaceType() + ); + } + public void testBuilder_build_fromLegacy() throws IOException { // Check legacy is picked up if model context and method context are not set ModelDao modelDao = mock(ModelDao.class); @@ -2090,6 +2129,14 @@ public IndexMetadata buildIndexMetaData(String indexName, Settings settings) { } public Mapper.TypeParser.ParserContext buildParserContext(String indexName, Settings settings) { + return dobuildParserContext(indexName, settings, CURRENT); + } + + public Mapper.TypeParser.ParserContext buildLegacyParserContext(String indexName, Settings settings, Version version) { + return dobuildParserContext(indexName, settings, version); + } + + public Mapper.TypeParser.ParserContext dobuildParserContext(String indexName, Settings settings, Version version) { IndexSettings indexSettings = new IndexSettings( buildIndexMetaData(indexName, settings), Settings.EMPTY, @@ -2104,7 +2151,7 @@ public Mapper.TypeParser.ParserContext buildParserContext(String indexName, Sett null, mapperService, type -> new KNNVectorFieldMapper.TypeParser(() -> mockModelDao), - CURRENT, + version, null, null, null diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index 896674a18..b43256336 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -1398,6 +1398,11 @@ public void validateKNNSearch(String testIndex, String testField, int dimension, ); List results = parseSearchResponse(EntityUtils.toString(searchResponse.getEntity()), testField); + // TMP + for (KNNResult r : results) { + System.out.println("+++++++++++++++++++ id:" + r.getDocId() + ", score=" + r.getScore()); + } + // TMP assertEquals(k, results.size()); for (int i = 0; i < k; i++) {