Fixing the BWC and rolling upgrade test after 2.18 release

Signed-off-by: Navneet Verma <[email protected]>

.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

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 {

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)

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.

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<KNNEngine> CUSTOM_SEGMENT_FILE_ENGINES = ImmutableSet.of(KNNEngine.NMSLIB, KNNEngine.FAISS);
     private static final Set<KNNEngine> ENGINES_SUPPORTING_FILTERS = ImmutableSet.of(KNNEngine.LUCENE, KNNEngine.FAISS);

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;

src/main/java/org/opensearch/knn/index/query/KNNWeight.java

@@ -251,7 +251,7 @@ private Map<Integer, Float> 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);

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

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

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());

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();

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)

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();

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);

src/main/java/org/opensearch/knn/index/codec/KNN990Codec/UnitTestCodec.java → 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;
 

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))
         );

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)
         );
     }

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
      */