From 967bf726a9f4f6ca1b19c7c0f9f98396c577ae94 Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Thu, 7 Nov 2024 12:58:08 -0800 Subject: [PATCH] Fixing the BWC and rolling upgrade test after 2.18 release Signed-off-by: Navneet Verma --- ...backwards_compatibility_tests_workflow.yml | 4 +- .../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 +- .../services/org.apache.lucene.codecs.Codec | 1 - .../org.apache.lucene.codecs.Codec | 11 +++ .../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 - .../codec/KNN990Codec/UnitTestCodec.java | 4 - .../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 | 2 +- .../transport/GetModelResponseTests.java | 9 +- .../transport/TrainingModelRequestTests.java | 6 +- 23 files changed, 90 insertions(+), 136 deletions(-) create mode 100644 src/test/META-INF.services/org.apache.lucene.codecs.Codec rename src/{main => test}/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java (84%) diff --git a/.github/workflows/backwards_compatibility_tests_workflow.yml b/.github/workflows/backwards_compatibility_tests_workflow.yml index a0e4530b13..5a90d58529 100644 --- a/.github/workflows/backwards_compatibility_tests_workflow.yml +++ b/.github/workflows/backwards_compatibility_tests_workflow.yml @@ -35,7 +35,7 @@ jobs: matrix: java: [ 21 ] os: [ubuntu-latest] - bwc_version : [ "2.0.1", "2.1.0", "2.2.1", "2.3.0", "2.4.1", "2.5.0", "2.6.0", "2.7.0", "2.8.0", "2.9.0", "2.10.0", "2.11.0", "2.12.0", "2.13.0", "2.14.0", "2.15.0", "2.16.0", "2.17.0", "2.18.0-SNAPSHOT"] + bwc_version : [ "2.0.1", "2.1.0", "2.2.1", "2.3.0", "2.4.1", "2.5.0", "2.6.0", "2.7.0", "2.8.0", "2.9.0", "2.10.0", "2.11.0", "2.12.0", "2.13.0", "2.14.0", "2.15.0", "2.16.0", "2.17.0", "2.18.0", "2.19.0-SNAPSHOT"] opensearch_version : [ "3.0.0-SNAPSHOT" ] exclude: - os: windows-latest @@ -124,7 +124,7 @@ jobs: matrix: java: [ 21 ] os: [ubuntu-latest] - bwc_version: [ "2.18.0-SNAPSHOT" ] + bwc_version: [ "2.19.0-SNAPSHOT" ] opensearch_version: [ "3.0.0-SNAPSHOT" ] name: k-NN Rolling-Upgrade BWC Tests 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 b212d844f7..7d2b3807a1 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,23 +61,6 @@ public void testKNNIndexDefaultLegacyFieldMapping() throws Exception { } } - // ensure that index is created using legacy mapping in old cluster, and, then, add docs to both old and new cluster. - // when search is requested on new cluster it should return all docs irrespective of 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 844605a8e9..9e85fd9caf 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,7 +15,6 @@ 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 d52c21c4ca..daae361e4a 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.NMSLIB; + return mode == Mode.ON_DISK ? KNNEngine.FAISS : KNNEngine.DEFAULT; } // 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 1e560a11ba..80b9f32a64 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 = FAISS; + public static final KNNEngine DEFAULT = NMSLIB; 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 beceadde50..18d4f7b64b 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -500,7 +500,8 @@ private void resolveKNNMethodComponents( .build() ); - if (useKNNMethodContextFromLegacy(builder, parserContext)) { + // If the original parameters are from legacy + if (builder.originalParameters.isLegacyMapping()) { // Then create KNNMethodContext to be used from the legacy index settings builder.originalParameters.setResolvedKnnMethodContext( createKNNMethodContextFromLegacy(parserContext.getSettings(), parserContext.indexVersionCreated(), resolvedSpaceType) @@ -549,12 +550,6 @@ 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 04c2ce587e..b5b2a5d22f 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.DEFAULT.getName()); + String engineName = fieldInfo.attributes().getOrDefault(KNN_ENGINE, KNNEngine.NMSLIB.getName()); knnEngine = KNNEngine.getEngine(engineName); String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, SpaceType.L2.getValue()); spaceType = SpaceType.getSpace(spaceTypeName); diff --git a/src/main/resources/META-INF/services/org.apache.lucene.codecs.Codec b/src/main/resources/META-INF/services/org.apache.lucene.codecs.Codec index 7a8916981e..0ac054339d 100644 --- a/src/main/resources/META-INF/services/org.apache.lucene.codecs.Codec +++ b/src/main/resources/META-INF/services/org.apache.lucene.codecs.Codec @@ -8,4 +8,3 @@ org.opensearch.knn.index.codec.KNN940Codec.KNN940Codec org.opensearch.knn.index.codec.KNN950Codec.KNN950Codec org.opensearch.knn.index.codec.KNN990Codec.KNN990Codec org.opensearch.knn.index.codec.KNN9120Codec.KNN9120Codec -org.opensearch.knn.index.codec.KNN990Codec.UnitTestCodec diff --git a/src/test/META-INF.services/org.apache.lucene.codecs.Codec b/src/test/META-INF.services/org.apache.lucene.codecs.Codec new file mode 100644 index 0000000000..ef1b36298f --- /dev/null +++ b/src/test/META-INF.services/org.apache.lucene.codecs.Codec @@ -0,0 +1,11 @@ +# +# SPDX-License-Identifier: Apache-2.0 +# +# The OpenSearch Contributors require contributions made to +# this file be licensed under the Apache-2.0 license or a +# compatible open source license. +# +# Modifications Copyright OpenSearch Contributors. See +# GitHub history for details. +# +org.opensearch.knn.index.codec.KNN990Codec.UnitTestCodec diff --git a/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java b/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java index 7bfce5b94a..587e80e5d6 100644 --- a/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java +++ b/src/test/java/org/opensearch/knn/KNNSingleNodeTestCase.java @@ -16,7 +16,6 @@ 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; @@ -51,7 +50,6 @@ 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; @@ -111,22 +109,6 @@ 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 d01c3a0b48..c25d2a3900 100644 --- a/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java +++ b/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java @@ -19,7 +19,6 @@ 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; @@ -109,7 +108,7 @@ public void testWarmup_shardNotPresentInCache() throws InterruptedException, Exe public void testGetAllEngineFileContexts() throws IOException, ExecutionException, InterruptedException { IndexService indexService = createKNNIndex(testIndexName); - createKnnIndexMapping(testIndexName, testFieldName, dimensions, KNNEngine.NMSLIB); + createKnnIndexMapping(testIndexName, testFieldName, dimensions); 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 8ca436bf41..b342fdf3f9 100644 --- a/src/test/java/org/opensearch/knn/index/NmslibIT.java +++ b/src/test/java/org/opensearch/knn/index/NmslibIT.java @@ -36,7 +36,6 @@ import java.util.TreeMap; import static org.hamcrest.Matchers.containsString; -import static org.opensearch.knn.common.KNNConstants.KNN_ENGINE; public class NmslibIT extends KNNRestTestCase { @@ -282,7 +281,6 @@ 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) @@ -338,7 +336,6 @@ 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) @@ -366,7 +363,6 @@ 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) @@ -379,7 +375,6 @@ 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 6e0f954a7a..959d94e3ee 100644 --- a/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java +++ b/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java @@ -263,6 +263,29 @@ 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 c11bc765f1..e1f16006d6 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,7 +279,6 @@ 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/main/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java b/src/test/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java similarity index 84% rename from src/main/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java rename to src/test/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java index d651f410a0..46302292bc 100644 --- a/src/main/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java +++ b/src/test/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java @@ -16,10 +16,6 @@ import org.apache.lucene.codecs.perfield.PerFieldKnnVectorsFormat; import org.opensearch.knn.index.codec.KNNCodecVersion; -/** - * This codec is for testing. The reason for putting this codec here is SPI is only scanning the src folder and not - * able to pick this class if its in test folder. Don't use this codec outside of testing - */ public class UnitTestCodec extends FilterCodec { private static final Integer BUILD_GRAPH_ALWAYS = 0; 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 315693a653..2036e14aa4 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.NMSLIB, + KNNEngine.DEFAULT, 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 291f0c671a..df195883a6 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_thenNMSLIB() { + public void testResolveEngine_whenModeSpecifiedAndCompressionIsNotSpecified_thenDefault() { assertEquals(KNNEngine.DEFAULT, ENGINE_RESOLVER.resolveEngine(KNNMethodConfigContext.builder().build(), null, false)); assertEquals( - KNNEngine.NMSLIB, + KNNEngine.DEFAULT, 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.NMSLIB, + KNNEngine.DEFAULT, 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 3f356999ce..5a6ed8e52c 100644 --- a/src/test/java/org/opensearch/knn/index/engine/KNNEngineTests.java +++ b/src/test/java/org/opensearch/knn/index/engine/KNNEngineTests.java @@ -25,10 +25,6 @@ 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 714723a8e7..98bbf42ca3 100644 --- a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java +++ b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java @@ -65,7 +65,6 @@ 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; @@ -147,7 +146,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.DEFAULT; + SpaceType spaceType = SpaceType.COSINESIMIL; int mRight = 17; int mWrong = 71; @@ -285,43 +284,36 @@ public void testTypeParser_withDifferentSpaceTypeCombinations_thenSuccess() thro ); assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); - // 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) - ); + // 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 { @@ -1622,7 +1614,7 @@ public void testBuilder_whenBinaryWithLegacyKNNDisabled_thenValid() { assertTrue(knnVectorFieldMapper instanceof FlatVectorFieldMapper); } - public void testTypeParser_whenBinaryWithLegacyKNNEnabled_thenValid() throws IOException { + public void testTypeParser_whenBinaryWithLegacyKNNEnabled_thenException() 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); @@ -1639,12 +1631,11 @@ public void testTypeParser_whenBinaryWithLegacyKNNEnabled_thenValid() throws IOE .field(VECTOR_DATA_TYPE_FIELD, VectorDataType.BINARY.getValue()) .endObject(); - final KNNVectorFieldMapper.Builder builder = (KNNVectorFieldMapper.Builder) typeParser.parse( - fieldName, - xContentBuilderToMap(xContentBuilder), - buildParserContext(indexName, settings) - ); - assertEquals(SpaceType.HAMMING, builder.getOriginalParameters().getResolvedKnnMethodContext().getSpaceType()); + Exception ex = expectThrows(Exception.class, () -> { + typeParser.parse(fieldName, xContentBuilderToMap(xContentBuilder), buildParserContext(indexName, settings)); + }); + + assertTrue(ex.getMessage(), ex.getMessage().contains("does not support space type")); } public void testBuild_whenInvalidCharsInFieldName_thenThrowException() { @@ -1670,7 +1661,7 @@ public void testTypeParser_whenModeAndCompressionAreSet_thenHandle() throws IOEx ModelDao modelDao = mock(ModelDao.class); KNNVectorFieldMapper.TypeParser typeParser = new KNNVectorFieldMapper.TypeParser(() -> modelDao); - // Default to faiss and ensure legacy is in use + // Default to nmslib and ensure legacy is in use XContentBuilder xContentBuilder = XContentFactory.jsonBuilder() .startObject() .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) @@ -1685,7 +1676,7 @@ public void testTypeParser_whenModeAndCompressionAreSet_thenHandle() throws IOEx assertTrue(builder.getOriginalParameters().isLegacyMapping()); validateBuilderAfterParsing( builder, - KNNEngine.DEFAULT, + KNNEngine.NMSLIB, 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 5118950261..482e7e0cb9 100644 --- a/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java +++ b/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java @@ -108,7 +108,6 @@ 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", @@ -175,11 +174,7 @@ 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(), KNN_ENGINE, KNNEngine.NMSLIB.getName()) - ); + testQueryScore(space::scoreTranslation, SEGMENT_FILES_NMSLIB, Map.of(SPACE_TYPE, space.getValue())); } } @@ -403,7 +398,7 @@ public void testEmptyQueryResults() { Map.of(), Sort.RELEVANCE ); - segmentInfo.setFiles(SEGMENT_FILES_DEFAULT); + segmentInfo.setFiles(SEGMENT_FILES_NMSLIB); 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 e116ef3c60..53a78b3817 100644 --- a/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java +++ b/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java @@ -933,7 +933,7 @@ public void testLoadIndex_nmslib_valid_with_stream() throws IOException { testData.indexData.getDimension(), directory, indexFileName1, - ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue(), KNN_ENGINE, KNNEngine.NMSLIB), + ImmutableMap.of(KNNConstants.SPACE_TYPE, SpaceType.L2.getValue()), KNNEngine.NMSLIB ); assertTrue(directory.fileLength(indexFileName1) > 0); 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 e503f78357..49f75ab305 100644 --- a/src/test/java/org/opensearch/knn/plugin/transport/GetModelResponseTests.java +++ b/src/test/java/org/opensearch/knn/plugin/transport/GetModelResponseTests.java @@ -29,7 +29,6 @@ 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,9 +75,7 @@ public void testXContent() throws IOException { Model model = new Model(getModelMetadata(ModelState.CREATED), testModelBlob, modelId); GetModelResponse getModelResponse = new GetModelResponse(model); String expectedResponseString = String.format( - 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(), + "{\"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\"}", Version.CURRENT.toString() ); XContentBuilder xContentBuilder = XContentFactory.jsonBuilder(); @@ -96,9 +93,7 @@ public void testXContentWithNoModelBlob() throws IOException { Model model = new Model(getModelMetadata(ModelState.FAILED), null, modelId); GetModelResponse getModelResponse = new GetModelResponse(model); String expectedResponseString = String.format( - 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(), + "{\"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\"}", Version.CURRENT.toString() ); XContentBuilder xContentBuilder = XContentFactory.jsonBuilder(); 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 6fd3994349..2c423e0eff 100644 --- a/src/test/java/org/opensearch/knn/plugin/transport/TrainingModelRequestTests.java +++ b/src/test/java/org/opensearch/knn/plugin/transport/TrainingModelRequestTests.java @@ -46,7 +46,6 @@ 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 { @@ -291,14 +290,11 @@ 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, - knnMethodContext, + getDefaultKNNMethodContext(), dimension, trainingIndex, trainingField,