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,