Changed the default values for ef_search and ef_constrction for enabl…

…ing better indexing and search latency for vector search

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

qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexSettingIT.java

@@ -0,0 +1,43 @@
+/*
+ * 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.
+ */
+
+package org.opensearch.knn.bwc;
+
+import org.junit.Assert;
+import org.opensearch.Version;
+import org.opensearch.knn.index.KNNSettings;
+
+public class IndexSettingIT extends AbstractRestartUpgradeTestCase {
+    private static final String TEST_FIELD = "test-field";
+    private static final int DIMENSIONS = 5;
+
+    public void testOldIndexSettingsPersistedAfterUpgrade() throws Exception {
+        if(isRunningAgainstOldCluster()) {
+            // create index with Old Values
+            createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS));
+            int old_ef_search = Integer.parseInt(getIndexSettingByName(testIndex,
+                    KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, true));
+            Assert.assertEquals(KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH.intValue(), old_ef_search);
+        } else {
+            int old_ef_search = Integer.parseInt(getIndexSettingByName(testIndex,
+                    KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, true));
+            Assert.assertEquals(KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH.intValue(), old_ef_search);
+            deleteKNNIndex(testIndex);
+        }
+    }
+
+//    private void assertEfSearchOldDefaultValue(String indexName) {
+//        if (Version.fromString(getBWCVersion().get()).onOrAfter(Version.V_2_10_0)) {
+//
+//        }
+//
+//    }
+}

src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java

@@ -0,0 +1,47 @@
+/*
+ * 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.
+ */
+
+package org.opensearch.knn.index;
+
+import lombok.NoArgsConstructor;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.index.shard.IndexSettingProvider;
+
+@NoArgsConstructor
+public class KNNIndexSettingProvider implements IndexSettingProvider {
+
+    private static final Settings EF_SEARCH_SETTING = Settings.builder().put(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH,
+            KNNSettings.INDEX_KNN_NEW_DEFAULT_ALGO_PARAM_EF_SEARCH).build();
+
+    /**
+     * Returns explicitly set default index {@link Settings} for the given index. This should not
+     * return null.
+     *
+     * @param indexName {@link String} name of the index
+     * @param isDataStreamIndex boolean: index is a datastream index or not
+     * @param templateAndRequestSettings {@link Settings}
+     */
+    @Override
+    public Settings getAdditionalIndexSettings(String indexName, boolean isDataStreamIndex, Settings templateAndRequestSettings) {
+        if(isKNNIndex(templateAndRequestSettings) && isEfSearchNotSetByUser(templateAndRequestSettings)) {
+            return EF_SEARCH_SETTING;
+        }
+        return IndexSettingProvider.super.getAdditionalIndexSettings(indexName, isDataStreamIndex, templateAndRequestSettings);
+    }
+
+    private boolean isKNNIndex(Settings templateAndRequestSettings) {
+        return templateAndRequestSettings.getAsBoolean(KNNSettings.KNN_INDEX, false);
+    }
+
+    private boolean isEfSearchNotSetByUser(Settings templateAndRequestSettings) {
+        return templateAndRequestSettings.hasValue(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH) == false;
+    }
+}

src/main/java/org/opensearch/knn/index/KNNSettings.java

@@ -81,7 +81,8 @@ public class KNNSettings {
     public static final String INDEX_KNN_DEFAULT_SPACE_TYPE = "l2";
     public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_M = 16;
     public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH = 512;
-    public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION = 512;
+    public static final Integer INDEX_KNN_NEW_DEFAULT_ALGO_PARAM_EF_SEARCH = 100;
+    public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION = 100;
     public static final Integer KNN_DEFAULT_ALGO_PARAM_INDEX_THREAD_QTY = 1;
     public static final Integer KNN_DEFAULT_CIRCUIT_BREAKER_UNSET_PERCENTAGE = 75;
     public static final Integer KNN_DEFAULT_MODEL_CACHE_SIZE_LIMIT_PERCENTAGE = 10; // By default, set aside 10% of the JVM for the limit
@@ -451,7 +452,7 @@ public static int getEfSearchParam(String index) {
             .getMetadata()
             .index(index)
             .getSettings()
-            .getAsInt(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, 512);
+            .getAsInt(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH);
     }
 
     public void setClusterService(ClusterService clusterService) {

src/main/java/org/opensearch/knn/jni/JNIService.java

@@ -11,6 +11,7 @@
 
 package org.opensearch.knn.jni;
 
+import lombok.extern.log4j.Log4j2;
 import org.apache.commons.lang.ArrayUtils;
 import org.opensearch.knn.index.query.KNNQueryResult;
 import org.opensearch.knn.index.util.KNNEngine;
@@ -20,6 +21,7 @@
 /**
  * Service to distribute requests to the proper engine jni service
  */
+@Log4j2
 public class JNIService {
 
     /**
@@ -32,6 +34,7 @@ public class JNIService {
      * @param engineName name of engine to build index for
      */
     public static void createIndex(int[] ids, float[][] data, String indexPath, Map<String, Object> parameters, String engineName) {
+        log.error("index path {}, has the parameters : {}, engine: {}", indexPath, parameters, engineName);
         if (KNNEngine.NMSLIB.getName().equals(engineName)) {
             NmslibService.createIndex(ids, data, indexPath, parameters);
             return;

src/main/java/org/opensearch/knn/plugin/KNNPlugin.java

@@ -11,9 +11,11 @@
 import org.opensearch.core.action.ActionResponse;
 import org.opensearch.index.codec.CodecServiceFactory;
 import org.opensearch.index.engine.EngineFactory;
+import org.opensearch.index.shard.IndexSettingProvider;
 import org.opensearch.indices.SystemIndexDescriptor;
 import org.opensearch.knn.index.KNNCircuitBreaker;
 import org.opensearch.knn.index.KNNClusterUtil;
+import org.opensearch.knn.index.KNNIndexSettingProvider;
 import org.opensearch.knn.index.query.KNNQueryBuilder;
 import org.opensearch.knn.index.KNNSettings;
 import org.opensearch.knn.index.mapper.KNNVectorFieldMapper;
@@ -361,4 +363,14 @@ public Settings additionalSettings() {
             .collect(Collectors.toList());
         return Settings.builder().putList(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS.getKey(), finalSettings).build();
     }
+
+    /**
+     * An {@link IndexSettingProvider} allows hooking in to parts of an index
+     * lifecycle to provide explicit default settings for newly created indices. Rather than changing
+     * the default values for an index-level setting, these act as though the setting has been set
+     * explicitly, but still allow the setting to be overridden by a template or creation request body.
+     */
+    public Collection<IndexSettingProvider> getAdditionalIndexSettingProviders() {
+        return List.of(new KNNIndexSettingProvider());
+    }
 }

src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java

@@ -14,6 +14,7 @@
 import org.opensearch.common.xcontent.XContentHelper;
 import org.opensearch.core.xcontent.DeprecationHandler;
 import org.opensearch.core.xcontent.MediaType;
+import org.opensearch.core.xcontent.MediaTypeRegistry;
 import org.opensearch.core.xcontent.NamedXContentRegistry;
 import org.opensearch.core.xcontent.XContentParser;
 import org.opensearch.index.query.MatchAllQueryBuilder;
@@ -49,6 +50,7 @@
 import javax.management.remote.JMXServiceURL;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -632,11 +634,26 @@ protected int getTotalGraphsInCache() throws IOException {
      * Get specific Index setting value from response
      */
     protected String getIndexSettingByName(String indexName, String settingName) throws IOException {
-        @SuppressWarnings("unchecked")
-        Map<String, Object> settings = (Map<String, Object>) ((Map<String, Object>) getIndexSettings(indexName).get(indexName)).get(
-            "settings"
-        );
-        return (String) settings.get(settingName);
+        return getIndexSettingByName(indexName, settingName, false);
+    }
+
+    protected String getIndexSettingByName(String indexName, String settingName, boolean includeDefaults) throws IOException {
+        Request request = new Request("GET", "/" + indexName + "/_settings");
+        if(includeDefaults) {
+            request.addParameter("include_defaults", "true");
+        }
+        request.addParameter("flat_settings", "true");
+        Response response = client().performRequest(request);
+        try (InputStream is = response.getEntity().getContent()) {
+            Map<String, Object> settings = XContentHelper.convertToMap(MediaTypeRegistry.JSON.xContent(), is, true);
+            Map<String, Object> defaultSettings = new HashMap<>();
+            if(includeDefaults) {
+                defaultSettings = (Map<String, Object>) settings.get("defaults");
+            }
+            Map<String, Object> userSettings = (Map<String, Object>)settings.get("settings");
+            return (String)(userSettings.get(settingName) == null ? defaultSettings.get(settingName) :
+                    userSettings.get(settingName));
+        }
     }
 
     protected void createModelSystemIndex() throws IOException {