From 5d98552629c0fe6e615dfad4ae8505d3a9f3bd95 Mon Sep 17 00:00:00 2001 From: Vijayan Balasubramanian Date: Thu, 24 Oct 2024 14:12:56 -0700 Subject: [PATCH] Update default engine to FAISS (#2235) Since faiss supports more features than nmslib, and, we had seen data points that there are more number of vector search users are interesed in faiss, we will be updating default engine to be faiss. This will benefit users who preffered to use defaults while working with vector search. Signed-off-by: Vijayan Balasubramanian --- .../org/opensearch/knn/bwc/IndexingIT.java | 17 ++++ .../opensearch-knn.release-notes-2.18.0.0.md | 1 + .../knn/index/engine/EngineResolver.java | 2 +- .../knn/index/engine/KNNEngine.java | 2 +- .../index/mapper/KNNVectorFieldMapper.java | 9 +- .../opensearch/knn/index/query/KNNWeight.java | 2 +- .../opensearch/knn/KNNSingleNodeTestCase.java | 18 ++++ .../knn/index/KNNIndexShardTests.java | 3 +- .../org/opensearch/knn/index/NmslibIT.java | 5 ++ .../knn/index/VectorDataTypeIT.java | 23 ----- .../KNN80DocValuesConsumerTests.java | 1 + .../knn/index/codec/KNNCodecTestCase.java | 2 +- .../knn/index/engine/EngineResolverTests.java | 6 +- .../knn/index/engine/KNNEngineTests.java | 4 + .../mapper/KNNVectorFieldMapperTests.java | 85 ++++++++++--------- .../knn/index/query/KNNWeightTests.java | 9 +- .../opensearch/knn/jni/JNIServiceTests.java | 72 ++++++++-------- .../transport/GetModelResponseTests.java | 9 +- .../transport/TrainingModelRequestTests.java | 6 +- 19 files changed, 163 insertions(+), 113 deletions(-) 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 bf4c8f7ec..c344581fe 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 @@ -61,6 +61,23 @@ public void testKNNIndexDefaultLegacyFieldMapping() throws Exception { } } + // ensure that index is created using legacy mapping in old cluster, and, then docs are added, and, new docs are added + // in new cluster, search should return docs when it was indexed during old cluster and new cluster. + public void testKNNIndexDefaultEngine() throws Exception { + waitForClusterHealthGreen(NODES_BWC_CLUSTER); + if (isRunningAgainstOldCluster()) { + createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS)); + addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, DOC_ID, 5); + // Flush to ensure that index is not re-indexed when node comes back up + flush(testIndex, true); + } else { + validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, 5, 5); + addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, 5, 5); + validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, 10, 10); + deleteKNNIndex(testIndex); + } + } + // Ensure that when segments created with old mapping are forcemerged in new cluster, they // succeed public void testKNNIndexDefaultLegacyFieldMappingForceMerge() throws Exception { diff --git a/release-notes/opensearch-knn.release-notes-2.18.0.0.md b/release-notes/opensearch-knn.release-notes-2.18.0.0.md index 9e85fd9ca..844605a8e 100644 --- a/release-notes/opensearch-knn.release-notes-2.18.0.0.md +++ b/release-notes/opensearch-knn.release-notes-2.18.0.0.md @@ -15,6 +15,7 @@ Compatible with OpenSearch 2.18.0 * Add CompressionLevel Calculation for PQ [#2200](https://github.com/opensearch-project/k-NN/pull/2200) * Remove FSDirectory dependency from native engine constructing side and deprecated FileWatcher [#2182](https://github.com/opensearch-project/k-NN/pull/2182) * Update approximate_threshold to 15K documents [#2229](https://github.com/opensearch-project/k-NN/pull/2229) +* Update default engine to FAISS [#2221](https://github.com/opensearch-project/k-NN/pull/2221) ### Bug Fixes * Add DocValuesProducers for releasing memory when close index [#1946](https://github.com/opensearch-project/k-NN/pull/1946) * KNN80DocValues should only be considered for BinaryDocValues fields [#2147](https://github.com/opensearch-project/k-NN/pull/2147) diff --git a/src/main/java/org/opensearch/knn/index/engine/EngineResolver.java b/src/main/java/org/opensearch/knn/index/engine/EngineResolver.java index daae361e4..d52c21c4c 100644 --- a/src/main/java/org/opensearch/knn/index/engine/EngineResolver.java +++ b/src/main/java/org/opensearch/knn/index/engine/EngineResolver.java @@ -49,7 +49,7 @@ public KNNEngine resolveEngine( // For 1x, we need to default to faiss if mode is provided and use nmslib otherwise if (CompressionLevel.isConfigured(compressionLevel) == false || compressionLevel == CompressionLevel.x1) { - return mode == Mode.ON_DISK ? KNNEngine.FAISS : KNNEngine.DEFAULT; + return mode == Mode.ON_DISK ? KNNEngine.FAISS : KNNEngine.NMSLIB; } // Lucene is only engine that supports 4x - so we have to default to it here. diff --git a/src/main/java/org/opensearch/knn/index/engine/KNNEngine.java b/src/main/java/org/opensearch/knn/index/engine/KNNEngine.java index 80b9f32a6..1e560a11b 100644 --- a/src/main/java/org/opensearch/knn/index/engine/KNNEngine.java +++ b/src/main/java/org/opensearch/knn/index/engine/KNNEngine.java @@ -29,7 +29,7 @@ public enum KNNEngine implements KNNLibrary { FAISS(FAISS_NAME, Faiss.INSTANCE), LUCENE(LUCENE_NAME, Lucene.INSTANCE); - public static final KNNEngine DEFAULT = NMSLIB; + public static final KNNEngine DEFAULT = FAISS; private static final Set CUSTOM_SEGMENT_FILE_ENGINES = ImmutableSet.of(KNNEngine.NMSLIB, KNNEngine.FAISS); private static final Set ENGINES_SUPPORTING_FILTERS = ImmutableSet.of(KNNEngine.LUCENE, KNNEngine.FAISS); 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 18d4f7b64..beceadde5 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -500,8 +500,7 @@ private void resolveKNNMethodComponents( .build() ); - // If the original parameters are from legacy - if (builder.originalParameters.isLegacyMapping()) { + if (useKNNMethodContextFromLegacy(builder, parserContext)) { // Then create KNNMethodContext to be used from the legacy index settings builder.originalParameters.setResolvedKnnMethodContext( createKNNMethodContextFromLegacy(parserContext.getSettings(), parserContext.indexVersionCreated(), resolvedSpaceType) @@ -550,6 +549,12 @@ private void setEngine(final KNNMethodContext knnMethodContext, KNNEngine knnEng } } + static boolean useKNNMethodContextFromLegacy(Builder builder, Mapper.TypeParser.ParserContext parserContext) { + // If the original parameters are from legacy, and it is created on or before 2_17_2 since default is changed to + // FAISS starting 2_18, which doesn't support accepting algo params from index settings + return parserContext.indexVersionCreated().onOrBefore(Version.V_2_17_2) && builder.originalParameters.isLegacyMapping(); + } + // We store the version of the index with the mapper as different version of Opensearch has different default // values of KNN engine Algorithms hyperparameters. protected Version indexCreatedVersion; diff --git a/src/main/java/org/opensearch/knn/index/query/KNNWeight.java b/src/main/java/org/opensearch/knn/index/query/KNNWeight.java index b5b2a5d22..04c2ce587 100644 --- a/src/main/java/org/opensearch/knn/index/query/KNNWeight.java +++ b/src/main/java/org/opensearch/knn/index/query/KNNWeight.java @@ -251,7 +251,7 @@ private Map doANNSearch( spaceType = modelMetadata.getSpaceType(); vectorDataType = modelMetadata.getVectorDataType(); } else { - String engineName = fieldInfo.attributes().getOrDefault(KNN_ENGINE, KNNEngine.NMSLIB.getName()); + String engineName = fieldInfo.attributes().getOrDefault(KNN_ENGINE, KNNEngine.DEFAULT.getName()); knnEngine = KNNEngine.getEngine(engineName); String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, SpaceType.L2.getValue()); spaceType = SpaceType.getSpace(spaceTypeName); diff --git a/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java b/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java index 587e80e5d..7bfce5b94 100644 --- a/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java +++ b/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java @@ -16,6 +16,7 @@ import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.knn.index.KNNSettings; +import org.opensearch.knn.index.engine.KNNEngine; import org.opensearch.knn.index.query.KNNQueryBuilder; import org.opensearch.knn.index.memory.NativeMemoryCacheManager; import org.opensearch.knn.index.memory.NativeMemoryLoadStrategy; @@ -50,6 +51,7 @@ import static org.mockito.Mockito.when; import static org.opensearch.knn.common.KNNConstants.DIMENSION; import static org.opensearch.knn.common.KNNConstants.KNN_ENGINE; +import static org.opensearch.knn.common.KNNConstants.METHOD_HNSW; import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_SPACE_TYPE; import static org.opensearch.knn.common.KNNConstants.MODEL_BLOB_PARAMETER; import static org.opensearch.knn.common.KNNConstants.MODEL_DESCRIPTION; @@ -109,6 +111,22 @@ protected void createKnnIndexMapping(String indexName, String fieldName, Integer /** * Create simple k-NN mapping with engine */ + protected void createKnnIndexMapping(String indexName, String fieldName, Integer dimensions, KNNEngine engine) throws IOException { + PutMappingRequest request = new PutMappingRequest(indexName); + XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().startObject().startObject("properties"); + xContentBuilder.startObject(fieldName); + xContentBuilder.field("type", "knn_vector").field("dimension", dimensions.toString()); + xContentBuilder.startObject("method"); + xContentBuilder.field("name", METHOD_HNSW); + xContentBuilder.field(KNN_ENGINE, engine.getName()); + xContentBuilder.endObject(); + xContentBuilder.endObject(); + xContentBuilder.endObject(); + xContentBuilder.endObject(); + request.source(xContentBuilder); + OpenSearchAssertions.assertAcked(client().admin().indices().putMapping(request).actionGet()); + } + protected void updateIndexSetting(String indexName, Settings setting) { UpdateSettingsRequest request = new UpdateSettingsRequest(setting, indexName); OpenSearchAssertions.assertAcked(client().admin().indices().updateSettings(request).actionGet()); diff --git a/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java b/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java index c25d2a390..d01c3a0b4 100644 --- a/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java +++ b/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java @@ -19,6 +19,7 @@ import org.opensearch.index.IndexService; import org.opensearch.index.engine.Engine; import org.opensearch.index.shard.IndexShard; +import org.opensearch.knn.index.engine.KNNEngine; import org.opensearch.knn.index.memory.NativeMemoryCacheManager; import java.io.IOException; @@ -108,7 +109,7 @@ public void testWarmup_shardNotPresentInCache() throws InterruptedException, Exe public void testGetAllEngineFileContexts() throws IOException, ExecutionException, InterruptedException { IndexService indexService = createKNNIndex(testIndexName); - createKnnIndexMapping(testIndexName, testFieldName, dimensions); + createKnnIndexMapping(testIndexName, testFieldName, dimensions, KNNEngine.NMSLIB); updateIndexSetting(testIndexName, Settings.builder().put(KNNSettings.INDEX_KNN_ADVANCED_APPROXIMATE_THRESHOLD, 0).build()); IndexShard indexShard = indexService.iterator().next(); diff --git a/src/test/java/org/opensearch/knn/index/NmslibIT.java b/src/test/java/org/opensearch/knn/index/NmslibIT.java index 495fc55ba..23d35d39e 100644 --- a/src/test/java/org/opensearch/knn/index/NmslibIT.java +++ b/src/test/java/org/opensearch/knn/index/NmslibIT.java @@ -36,6 +36,7 @@ import java.util.TreeMap; import static org.hamcrest.Matchers.containsString; +import static org.opensearch.knn.common.KNNConstants.KNN_ENGINE; public class NmslibIT extends KNNRestTestCase { @@ -281,6 +282,7 @@ public void testCreateIndexWithValidAlgoParams_mapping() { .field("dimension", 2) .startObject(KNNConstants.KNN_METHOD) .field(KNNConstants.METHOD_PARAMETER_SPACE_TYPE, spaceType) + .field(KNN_ENGINE, KNNEngine.NMSLIB.getName()) .field(KNNConstants.NAME, KNNConstants.METHOD_HNSW) .startObject(KNNConstants.PARAMETERS) .field(KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION, efConstruction) @@ -336,6 +338,7 @@ public void testCreateIndexWithValidAlgoParams_mappingAndSettings() { .field("dimension", 2) .startObject(KNNConstants.KNN_METHOD) .field(KNNConstants.METHOD_PARAMETER_SPACE_TYPE, spaceType1) + .field(KNN_ENGINE, KNNEngine.NMSLIB.getName()) .field(KNNConstants.NAME, KNNConstants.METHOD_HNSW) .startObject(KNNConstants.PARAMETERS) .field(KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION, efConstruction1) @@ -363,6 +366,7 @@ public void testCreateIndexWithValidAlgoParams_mappingAndSettings() { .field("dimension", 2) .startObject(KNNConstants.KNN_METHOD) .field(KNNConstants.METHOD_PARAMETER_SPACE_TYPE, spaceType1) + .field(KNN_ENGINE, KNNEngine.NMSLIB.getName()) .field(KNNConstants.NAME, KNNConstants.METHOD_HNSW) .startObject(KNNConstants.PARAMETERS) .field(KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION, efConstruction1) @@ -375,6 +379,7 @@ public void testCreateIndexWithValidAlgoParams_mappingAndSettings() { .field("dimension", 2) .startObject(KNNConstants.KNN_METHOD) .field(KNNConstants.METHOD_PARAMETER_SPACE_TYPE, spaceType2) + .field(KNN_ENGINE, KNNEngine.NMSLIB.getName()) .field(KNNConstants.NAME, KNNConstants.METHOD_HNSW) .startObject(KNNConstants.PARAMETERS) .field(KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION, efConstruction2) diff --git a/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java b/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java index fcfb192ca..2e68339d4 100644 --- a/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java +++ b/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java @@ -263,29 +263,6 @@ public void testByteVectorDataTypeWithNmslibEngine() { assertTrue(ex.getMessage().contains("is not supported for vector data type")); } - @SneakyThrows - public void testByteVectorDataTypeWithLegacyFieldMapperKnnIndexSetting() { - // Create an index with byte vector data_type and index.knn as true without setting KnnMethodContext, - // which should throw an exception because the LegacyFieldMapper will use NMSLIB engine and byte data_type - // is not supported for NMSLIB engine. - XContentBuilder builder = XContentFactory.jsonBuilder() - .startObject() - .startObject(PROPERTIES_FIELD) - .startObject(FIELD_NAME) - .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) - .field(DIMENSION, 2) - .field(VECTOR_DATA_TYPE_FIELD, VectorDataType.BYTE.getValue()) - .endObject() - .endObject() - .endObject(); - - String mapping = builder.toString(); - - ResponseException ex = expectThrows(ResponseException.class, () -> createKnnIndex(INDEX_NAME, mapping)); - assertTrue(ex.getMessage(), ex.getMessage().contains("is not supported for vector data type")); - - } - public void testDocValuesWithByteVectorDataTypeLuceneEngine() throws Exception { createKnnIndexMappingWithLuceneEngine(2, SpaceType.L2, VectorDataType.BYTE.getValue()); ingestL2ByteTestData(); diff --git a/src/test/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumerTests.java b/src/test/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumerTests.java index e1f16006d..c11bc765f 100644 --- a/src/test/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumerTests.java +++ b/src/test/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumerTests.java @@ -279,6 +279,7 @@ public void testAddKNNBinaryField_fromScratch_nmslibLegacy() throws IOException .addAttribute(KNNConstants.HNSW_ALGO_EF_CONSTRUCTION, "512") .addAttribute(KNNConstants.HNSW_ALGO_M, "16") .addAttribute(KNNConstants.SPACE_TYPE, spaceType.getValue()) + .addAttribute(KNNConstants.KNN_ENGINE, knnEngine.getName()) .build() }; FieldInfos fieldInfos = new FieldInfos(fieldInfoArray); diff --git a/src/test/java/org/opensearch/knn/index/codec/KNNCodecTestCase.java b/src/test/java/org/opensearch/knn/index/codec/KNNCodecTestCase.java index e6fcb643d..53614c7c4 100644 --- a/src/test/java/org/opensearch/knn/index/codec/KNNCodecTestCase.java +++ b/src/test/java/org/opensearch/knn/index/codec/KNNCodecTestCase.java @@ -103,7 +103,7 @@ public class KNNCodecTestCase extends KNNTestCase { .vectorDataType(VectorDataType.DEFAULT) .build(); KNNMethodContext knnMethodContext = new KNNMethodContext( - KNNEngine.DEFAULT, + KNNEngine.NMSLIB, SpaceType.DEFAULT, new MethodComponentContext(METHOD_HNSW, ImmutableMap.of(METHOD_PARAMETER_M, 16, METHOD_PARAMETER_EF_CONSTRUCTION, 512)) ); diff --git a/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java b/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java index df195883a..291f0c671 100644 --- a/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java +++ b/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java @@ -41,10 +41,10 @@ public void testResolveEngine_whenModeAndCompressionAreFalse_thenDefault() { ); } - public void testResolveEngine_whenModeSpecifiedAndCompressionIsNotSpecified_thenDefault() { + public void testResolveEngine_whenModeSpecifiedAndCompressionIsNotSpecified_thenNMSLIB() { assertEquals(KNNEngine.DEFAULT, ENGINE_RESOLVER.resolveEngine(KNNMethodConfigContext.builder().build(), null, false)); assertEquals( - KNNEngine.DEFAULT, + KNNEngine.NMSLIB, ENGINE_RESOLVER.resolveEngine( KNNMethodConfigContext.builder().mode(Mode.IN_MEMORY).build(), new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY, false), @@ -63,7 +63,7 @@ public void testResolveEngine_whenCompressionIs1x_thenEngineBasedOnMode() { ) ); assertEquals( - KNNEngine.DEFAULT, + KNNEngine.NMSLIB, ENGINE_RESOLVER.resolveEngine(KNNMethodConfigContext.builder().compressionLevel(CompressionLevel.x1).build(), null, false) ); } diff --git a/src/test/java/org/opensearch/knn/index/engine/KNNEngineTests.java b/src/test/java/org/opensearch/knn/index/engine/KNNEngineTests.java index 5a6ed8e52..3f356999c 100644 --- a/src/test/java/org/opensearch/knn/index/engine/KNNEngineTests.java +++ b/src/test/java/org/opensearch/knn/index/engine/KNNEngineTests.java @@ -25,6 +25,10 @@ public void testDelegateLibraryFunctions() { assertEquals(Lucene.INSTANCE.getVersion(), KNNEngine.LUCENE.getVersion()); } + public void testGetDefaultEngine_thenReturnFAISS() { + assertEquals(KNNEngine.FAISS, KNNEngine.DEFAULT); + } + /** * Test name getter */ 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 98bbf42ca..714723a8e 100644 --- a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java +++ b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java @@ -65,6 +65,7 @@ import java.util.Optional; import java.util.stream.Collectors; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -146,7 +147,7 @@ public void testTypeParser_build_fromKnnMethodContext() throws IOException { // Check that knnMethodContext takes precedent over both model and legacy ModelDao modelDao = mock(ModelDao.class); - SpaceType spaceType = SpaceType.COSINESIMIL; + SpaceType spaceType = SpaceType.DEFAULT; int mRight = 17; int mWrong = 71; @@ -284,36 +285,43 @@ public void testTypeParser_withDifferentSpaceTypeCombinations_thenSuccess() thro ); assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); - // if space type is provided and legacy mappings is hit - xContentBuilder = XContentFactory.jsonBuilder() - .startObject() - .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) - .field(DIMENSION_FIELD_NAME, TEST_DIMENSION) - .field(KNNConstants.TOP_LEVEL_PARAMETER_SPACE_TYPE, topLevelSpaceType.getValue()) - .endObject(); - builder = (KNNVectorFieldMapper.Builder) typeParser.parse( - "test-field-name-1", - xContentBuilderToMap(xContentBuilder), - buildParserContext("test", settings) - ); + // mock useKNNMethodContextFromLegacy to simulate index ix created before 2_18 + try (MockedStatic utilMockedStatic = Mockito.mockStatic(KNNVectorFieldMapper.class)) { + utilMockedStatic.when(() -> KNNVectorFieldMapper.useKNNMethodContextFromLegacy(any(), any())).thenReturn(true); + // if space type is provided and legacy mappings is hit + xContentBuilder = XContentFactory.jsonBuilder() + .startObject() + .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) + .field(DIMENSION_FIELD_NAME, TEST_DIMENSION) + .field(KNNConstants.TOP_LEVEL_PARAMETER_SPACE_TYPE, topLevelSpaceType.getValue()) + .endObject(); + builder = (KNNVectorFieldMapper.Builder) typeParser.parse( + "test-field-name-1", + xContentBuilderToMap(xContentBuilder), + buildParserContext("test", settings) + ); - builderContext = new Mapper.BuilderContext(settings, new ContentPath()); - knnVectorFieldMapper = builder.build(builderContext); - assertTrue(knnVectorFieldMapper instanceof MethodFieldMapper); - assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().isPresent()); - assertEquals(topLevelSpaceType, knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().get().getSpaceType()); - // this check ensures that legacy mapping is hit, as in legacy mapping we pick M from index settings - assertEquals( - mForSetting, - knnVectorFieldMapper.fieldType() - .getKnnMappingConfig() - .getKnnMethodContext() - .get() - .getMethodComponentContext() - .getParameters() - .get(METHOD_PARAMETER_M) - ); - assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); + builderContext = new Mapper.BuilderContext(settings, new ContentPath()); + knnVectorFieldMapper = builder.build(builderContext); + assertTrue(knnVectorFieldMapper instanceof MethodFieldMapper); + assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().isPresent()); + assertEquals( + topLevelSpaceType, + knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().get().getSpaceType() + ); + // this check ensures that legacy mapping is hit, as in legacy mapping we pick M from index settings + assertEquals( + mForSetting, + knnVectorFieldMapper.fieldType() + .getKnnMappingConfig() + .getKnnMethodContext() + .get() + .getMethodComponentContext() + .getParameters() + .get(METHOD_PARAMETER_M) + ); + assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); + } } public void testTypeParser_withSpaceTypeAndMode_thenSuccess() throws IOException { @@ -1614,7 +1622,7 @@ public void testBuilder_whenBinaryWithLegacyKNNDisabled_thenValid() { assertTrue(knnVectorFieldMapper instanceof FlatVectorFieldMapper); } - public void testTypeParser_whenBinaryWithLegacyKNNEnabled_thenException() throws IOException { + public void testTypeParser_whenBinaryWithLegacyKNNEnabled_thenValid() throws IOException { // Check legacy is picked up if model context and method context are not set ModelDao modelDao = mock(ModelDao.class); KNNVectorFieldMapper.TypeParser typeParser = new KNNVectorFieldMapper.TypeParser(() -> modelDao); @@ -1631,11 +1639,12 @@ public void testTypeParser_whenBinaryWithLegacyKNNEnabled_thenException() throws .field(VECTOR_DATA_TYPE_FIELD, VectorDataType.BINARY.getValue()) .endObject(); - Exception ex = expectThrows(Exception.class, () -> { - typeParser.parse(fieldName, xContentBuilderToMap(xContentBuilder), buildParserContext(indexName, settings)); - }); - - assertTrue(ex.getMessage(), ex.getMessage().contains("does not support space type")); + final KNNVectorFieldMapper.Builder builder = (KNNVectorFieldMapper.Builder) typeParser.parse( + fieldName, + xContentBuilderToMap(xContentBuilder), + buildParserContext(indexName, settings) + ); + assertEquals(SpaceType.HAMMING, builder.getOriginalParameters().getResolvedKnnMethodContext().getSpaceType()); } public void testBuild_whenInvalidCharsInFieldName_thenThrowException() { @@ -1661,7 +1670,7 @@ public void testTypeParser_whenModeAndCompressionAreSet_thenHandle() throws IOEx ModelDao modelDao = mock(ModelDao.class); KNNVectorFieldMapper.TypeParser typeParser = new KNNVectorFieldMapper.TypeParser(() -> modelDao); - // Default to nmslib and ensure legacy is in use + // Default to faiss and ensure legacy is in use XContentBuilder xContentBuilder = XContentFactory.jsonBuilder() .startObject() .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) @@ -1676,7 +1685,7 @@ public void testTypeParser_whenModeAndCompressionAreSet_thenHandle() throws IOEx assertTrue(builder.getOriginalParameters().isLegacyMapping()); validateBuilderAfterParsing( builder, - KNNEngine.NMSLIB, + KNNEngine.DEFAULT, SpaceType.L2, VectorDataType.FLOAT, CompressionLevel.x1, diff --git a/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java b/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java index 482e7e0cb..511895026 100644 --- a/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java +++ b/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java @@ -108,6 +108,7 @@ public class KNNWeightTests extends KNNTestCase { private static final int K = 5; private static final Set SEGMENT_FILES_NMSLIB = Set.of("_0.cfe", "_0_2011_target_field.hnswc"); private static final Set SEGMENT_FILES_FAISS = Set.of("_0.cfe", "_0_2011_target_field.faissc"); + private static final Set SEGMENT_FILES_DEFAULT = SEGMENT_FILES_FAISS; private static final Set SEGMENT_MULTI_FIELD_FILES_FAISS = Set.of( "_0.cfe", "_0_2011_target_field.faissc", @@ -174,7 +175,11 @@ public void tearDownAfterTest() { @SneakyThrows public void testQueryResultScoreNmslib() { for (SpaceType space : List.of(SpaceType.L2, SpaceType.L1, SpaceType.COSINESIMIL, SpaceType.INNER_PRODUCT, SpaceType.LINF)) { - testQueryScore(space::scoreTranslation, SEGMENT_FILES_NMSLIB, Map.of(SPACE_TYPE, space.getValue())); + testQueryScore( + space::scoreTranslation, + SEGMENT_FILES_NMSLIB, + Map.of(SPACE_TYPE, space.getValue(), KNN_ENGINE, KNNEngine.NMSLIB.getName()) + ); } } @@ -398,7 +403,7 @@ public void testEmptyQueryResults() { Map.of(), Sort.RELEVANCE ); - segmentInfo.setFiles(SEGMENT_FILES_NMSLIB); + segmentInfo.setFiles(SEGMENT_FILES_DEFAULT); final SegmentCommitInfo segmentCommitInfo = new SegmentCommitInfo(segmentInfo, 0, 0, 0, 0, 0, new byte[StringHelper.ID_LENGTH]); when(reader.getSegmentInfo()).thenReturn(segmentCommitInfo); diff --git a/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java b/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java index f6d118092..2a5d6cbc6 100644 --- a/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java +++ b/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java @@ -64,6 +64,7 @@ import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_SPACE_TYPE; import static org.opensearch.knn.common.KNNConstants.NAME; import static org.opensearch.knn.common.KNNConstants.PARAMETERS; +import static org.opensearch.knn.index.engine.KNNEngine.NMSLIB; public class JNIServiceTests extends KNNTestCase { static final int FP16_MAX = 65504; @@ -118,7 +119,7 @@ public void testCreateIndex_nmslib_invalid_noSpaceType() { testData.indexData.getDimension(), "something", Collections.emptyMap(), - KNNEngine.NMSLIB + NMSLIB ) ); } @@ -137,7 +138,7 @@ public void testCreateIndex_nmslib_invalid_vectorDocIDMismatch() throws IOExcept vectors1[0].length, tmpFile1.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ) ); @@ -153,7 +154,7 @@ public void testCreateIndex_nmslib_invalid_vectorDocIDMismatch() throws IOExcept vectors2[0].length, tmpFile2.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ) ); } @@ -172,7 +173,7 @@ public void testCreateIndex_nmslib_invalid_nullArgument() throws IOException { 0, tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ) ); @@ -184,7 +185,7 @@ public void testCreateIndex_nmslib_invalid_nullArgument() throws IOException { 0, tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ) ); @@ -196,13 +197,13 @@ public void testCreateIndex_nmslib_invalid_nullArgument() throws IOException { 0, null, ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ) ); expectThrows( Exception.class, - () -> TestUtils.createIndex(docIds, memoryAddress, 0, tmpFile.toAbsolutePath().toString(), null, KNNEngine.NMSLIB) + () -> TestUtils.createIndex(docIds, memoryAddress, 0, tmpFile.toAbsolutePath().toString(), null, NMSLIB) ); expectThrows( @@ -232,7 +233,7 @@ public void testCreateIndex_nmslib_invalid_badSpace() throws IOException { vectors[0].length, tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, "invalid"), - KNNEngine.NMSLIB + NMSLIB ) ); } @@ -258,7 +259,7 @@ public void testCreateIndex_nmslib_invalid_badParameterType() throws IOException vectors[0].length, tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue(), KNNConstants.PARAMETERS, parametersMap), - KNNEngine.NMSLIB + NMSLIB ) ); } @@ -278,7 +279,7 @@ public void testCreateIndex_nmslib_valid() throws IOException { testData.indexData.getDimension(), tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, spaceType.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertTrue(tmpFile.toFile().length() > 0); @@ -297,7 +298,7 @@ public void testCreateIndex_nmslib_valid() throws IOException { KNNConstants.METHOD_PARAMETER_M, 12 ), - KNNEngine.NMSLIB + NMSLIB ); assertTrue(tmpFile.toFile().length() > 0); } @@ -698,20 +699,17 @@ public void testLoadIndex_invalidEngine() { } public void testLoadIndex_nmslib_invalid_badSpaceType() { - expectThrows( - Exception.class, - () -> JNIService.loadIndex("test", ImmutableMap.of(KNNConstants.SPACE_TYPE, "invalid"), KNNEngine.NMSLIB) - ); + expectThrows(Exception.class, () -> JNIService.loadIndex("test", ImmutableMap.of(KNNConstants.SPACE_TYPE, "invalid"), NMSLIB)); } public void testLoadIndex_nmslib_invalid_noSpaceType() { - expectThrows(Exception.class, () -> JNIService.loadIndex("test", Collections.emptyMap(), KNNEngine.NMSLIB)); + expectThrows(Exception.class, () -> JNIService.loadIndex("test", Collections.emptyMap(), NMSLIB)); } public void testLoadIndex_nmslib_invalid_fileDoesNotExist() { expectThrows( Exception.class, - () -> JNIService.loadIndex("invalid", ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), KNNEngine.NMSLIB) + () -> JNIService.loadIndex("invalid", ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), NMSLIB) ); } @@ -722,7 +720,7 @@ public void testLoadIndex_nmslib_invalid_badFile() throws IOException { () -> JNIService.loadIndex( tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ) ); } @@ -737,14 +735,14 @@ public void testLoadIndex_nmslib_valid() throws IOException { testData.indexData.getDimension(), tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertTrue(tmpFile.toFile().length() > 0); long pointer = JNIService.loadIndex( tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertNotEquals(0, pointer); } @@ -757,8 +755,8 @@ public void testLoadIndex_nmslib_valid_with_stream() throws IOException { testData.loadDataToMemoryAddress(), testData.indexData.getDimension(), tmpFile.toAbsolutePath().toString(), - ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue(), KNN_ENGINE, NMSLIB), + NMSLIB ); assertTrue(tmpFile.toFile().length() > 0); @@ -767,7 +765,7 @@ public void testLoadIndex_nmslib_valid_with_stream() throws IOException { long pointer = JNIService.loadIndex( new IndexInputWithBuffer(indexInput), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertNotEquals(0, pointer); } @@ -815,7 +813,7 @@ public void testQueryIndex_invalidEngine() { public void testQueryIndex_nmslib_invalid_badPointer() { - expectThrows(Exception.class, () -> JNIService.queryIndex(0L, new float[] {}, 0, null, KNNEngine.NMSLIB, null, 0, null)); + expectThrows(Exception.class, () -> JNIService.queryIndex(0L, new float[] {}, 0, null, NMSLIB, null, 0, null)); } public void testQueryIndex_nmslib_invalid_nullQueryVector() throws IOException { @@ -828,18 +826,18 @@ public void testQueryIndex_nmslib_invalid_nullQueryVector() throws IOException { testData.indexData.getDimension(), tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertTrue(tmpFile.toFile().length() > 0); long pointer = JNIService.loadIndex( tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertNotEquals(0, pointer); - expectThrows(Exception.class, () -> JNIService.queryIndex(pointer, null, 10, null, KNNEngine.NMSLIB, null, 0, null)); + expectThrows(Exception.class, () -> JNIService.queryIndex(pointer, null, 10, null, NMSLIB, null, 0, null)); } public void testQueryIndex_nmslib_valid() throws IOException { @@ -858,19 +856,19 @@ public void testQueryIndex_nmslib_valid() throws IOException { testData.indexData.getDimension(), tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, spaceType.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertTrue(tmpFile.toFile().length() > 0); long pointer = JNIService.loadIndex( tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, spaceType.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertNotEquals(0, pointer); for (float[] query : testData.queries) { - KNNQueryResult[] results = JNIService.queryIndex(pointer, query, k, null, KNNEngine.NMSLIB, null, 0, null); + KNNQueryResult[] results = JNIService.queryIndex(pointer, query, k, null, NMSLIB, null, 0, null); assertEquals(k, results.length); } } @@ -1284,18 +1282,18 @@ public void testFree_nmslib_valid() throws IOException { testData.indexData.getDimension(), tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertTrue(tmpFile.toFile().length() > 0); long pointer = JNIService.loadIndex( tmpFile.toAbsolutePath().toString(), ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), - KNNEngine.NMSLIB + NMSLIB ); assertNotEquals(0, pointer); - JNIService.free(pointer, KNNEngine.NMSLIB); + JNIService.free(pointer, NMSLIB); } public void testFree_faiss_valid() throws IOException { @@ -1550,7 +1548,7 @@ public void testIndexLoad_whenStateIsShared_thenSucceed() { @SneakyThrows public void testIsIndexIVFPQL2() { long dummyAddress = 0; - assertFalse(JNIService.isSharedIndexStateRequired(dummyAddress, KNNEngine.NMSLIB)); + assertFalse(JNIService.isSharedIndexStateRequired(dummyAddress, NMSLIB)); String faissIVFPQL2Index = createFaissIVFPQIndex(16, 16, 4, SpaceType.L2); long faissIVFPQL2Address = JNIService.loadIndex(faissIVFPQL2Index, Collections.emptyMap(), KNNEngine.FAISS); @@ -1571,9 +1569,9 @@ public void testIsIndexIVFPQL2() { @SneakyThrows public void testFunctionsUnsupportedForEngine_whenEngineUnsupported_thenThrowIllegalArgumentException() { int dummyAddress = 0; - expectThrows(IllegalArgumentException.class, () -> JNIService.initSharedIndexState(dummyAddress, KNNEngine.NMSLIB)); - expectThrows(IllegalArgumentException.class, () -> JNIService.setSharedIndexState(dummyAddress, dummyAddress, KNNEngine.NMSLIB)); - expectThrows(IllegalArgumentException.class, () -> JNIService.freeSharedIndexState(dummyAddress, KNNEngine.NMSLIB)); + expectThrows(IllegalArgumentException.class, () -> JNIService.initSharedIndexState(dummyAddress, NMSLIB)); + expectThrows(IllegalArgumentException.class, () -> JNIService.setSharedIndexState(dummyAddress, dummyAddress, NMSLIB)); + expectThrows(IllegalArgumentException.class, () -> JNIService.freeSharedIndexState(dummyAddress, NMSLIB)); } private void assertQueryResultsMatch(float[][] testQueries, int k, List indexAddresses) { diff --git a/src/test/java/org/opensearch/knn/plugin/transport/GetModelResponseTests.java b/src/test/java/org/opensearch/knn/plugin/transport/GetModelResponseTests.java index 3bf5f302c..771b07edd 100644 --- a/src/test/java/org/opensearch/knn/plugin/transport/GetModelResponseTests.java +++ b/src/test/java/org/opensearch/knn/plugin/transport/GetModelResponseTests.java @@ -30,6 +30,7 @@ import org.opensearch.knn.indices.ModelState; import java.io.IOException; +import java.util.Locale; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; @@ -76,7 +77,9 @@ public void testXContent() throws IOException { Model model = new Model(getModelMetadata(ModelState.CREATED), testModelBlob, modelId); GetModelResponse getModelResponse = new GetModelResponse(model); String expectedResponseString = String.format( - "{\"model_id\":\"test-model\",\"model_blob\":\"aGVsbG8=\",\"state\":\"created\",\"timestamp\":\"2021-03-27 10:15:30 AM +05:30\",\"description\":\"test model\",\"error\":\"\",\"space_type\":\"l2\",\"dimension\":4,\"engine\":\"nmslib\",\"training_node_assignment\":\"\",\"model_definition\":{\"name\":\"\",\"parameters\":{}},\"data_type\":\"float\",\"model_version\":\"%s\"}", + Locale.ROOT, + "{\"model_id\":\"test-model\",\"model_blob\":\"aGVsbG8=\",\"state\":\"created\",\"timestamp\":\"2021-03-27 10:15:30 AM +05:30\",\"description\":\"test model\",\"error\":\"\",\"space_type\":\"l2\",\"dimension\":4,\"engine\":\"%s\",\"training_node_assignment\":\"\",\"model_definition\":{\"name\":\"\",\"parameters\":{}},\"data_type\":\"float\",\"model_version\":\"%s\"}", + KNNEngine.DEFAULT.getName(), Version.CURRENT.toString() ); XContentBuilder xContentBuilder = MediaTypeRegistry.contentBuilder(XContentType.JSON); @@ -94,7 +97,9 @@ public void testXContentWithNoModelBlob() throws IOException { Model model = new Model(getModelMetadata(ModelState.FAILED), null, modelId); GetModelResponse getModelResponse = new GetModelResponse(model); String expectedResponseString = String.format( - "{\"model_id\":\"test-model\",\"model_blob\":\"\",\"state\":\"failed\",\"timestamp\":\"2021-03-27 10:15:30 AM +05:30\",\"description\":\"test model\",\"error\":\"\",\"space_type\":\"l2\",\"dimension\":4,\"engine\":\"nmslib\",\"training_node_assignment\":\"\",\"model_definition\":{\"name\":\"\",\"parameters\":{}},\"data_type\":\"float\",\"model_version\":\"%s\"}", + Locale.ROOT, + "{\"model_id\":\"test-model\",\"model_blob\":\"\",\"state\":\"failed\",\"timestamp\":\"2021-03-27 10:15:30 AM +05:30\",\"description\":\"test model\",\"error\":\"\",\"space_type\":\"l2\",\"dimension\":4,\"engine\":\"%s\",\"training_node_assignment\":\"\",\"model_definition\":{\"name\":\"\",\"parameters\":{}},\"data_type\":\"float\",\"model_version\":\"%s\"}", + KNNEngine.DEFAULT.getName(), Version.CURRENT.toString() ); XContentBuilder xContentBuilder = MediaTypeRegistry.contentBuilder(XContentType.JSON); diff --git a/src/test/java/org/opensearch/knn/plugin/transport/TrainingModelRequestTests.java b/src/test/java/org/opensearch/knn/plugin/transport/TrainingModelRequestTests.java index 2c423e0ef..6fd399434 100644 --- a/src/test/java/org/opensearch/knn/plugin/transport/TrainingModelRequestTests.java +++ b/src/test/java/org/opensearch/knn/plugin/transport/TrainingModelRequestTests.java @@ -46,6 +46,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.opensearch.knn.common.KNNConstants.METHOD_HNSW; public class TrainingModelRequestTests extends KNNTestCase { @@ -290,11 +291,14 @@ public void testValidation_invalid_invalidMethodContext() { String trainingIndex = "test-training-index"; String trainingField = "test-training-field"; + MethodComponentContext methodComponentContext = new MethodComponentContext(METHOD_HNSW, Collections.emptyMap()); + final KNNMethodContext knnMethodContext = new KNNMethodContext(KNNEngine.NMSLIB, SpaceType.DEFAULT, methodComponentContext); + ValidationException validationException = expectThrows( ValidationException.class, () -> new TrainingModelRequest( modelId, - getDefaultKNNMethodContext(), + knnMethodContext, dimension, trainingIndex, trainingField,