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/IndexingIT.java

@@ -5,9 +5,13 @@
 
 package org.opensearch.knn.bwc;
 
+import org.junit.Assert;
 import org.opensearch.common.xcontent.XContentFactory;
 import org.opensearch.knn.index.SpaceType;
 
+import java.io.IOException;
+import java.util.Map;
+
 import static org.opensearch.knn.TestUtils.KNN_ALGO_PARAM_EF_CONSTRUCTION_MIN_VALUE;
 import static org.opensearch.knn.TestUtils.KNN_ALGO_PARAM_M_MIN_VALUE;
 import static org.opensearch.knn.TestUtils.KNN_VECTOR;
@@ -16,8 +20,13 @@
 import static org.opensearch.knn.TestUtils.VECTOR_TYPE;
 import static org.opensearch.knn.common.KNNConstants.DIMENSION;
 import static org.opensearch.knn.common.KNNConstants.FAISS_NAME;
+import static org.opensearch.knn.common.KNNConstants.KNN_ENGINE;
 import static org.opensearch.knn.common.KNNConstants.KNN_METHOD;
 import static org.opensearch.knn.common.KNNConstants.METHOD_HNSW;
+import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION;
+import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_EF_SEARCH;
+import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_M;
+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;
 
@@ -28,6 +37,7 @@ public class IndexingIT extends AbstractRestartUpgradeTestCase {
     private static final int K = 5;
     private static final int M = 50;
     private static final int EF_CONSTRUCTION = 1024;
+    private static final int EF_SEARCH = 200;
     private static final int NUM_DOCS = 10;
     private static int QUERY_COUNT = 0;
 
@@ -78,20 +88,35 @@ public void testKNNIndexDefaultMethodFieldMapping() throws Exception {
     }
 
     // Custom Method Field Mapping
-    // space_type : "inner_product", engine : "faiss", m : 50, ef_construction : 1024
+    // space_type : "inner_product", engine : "faiss", m : 50, ef_construction : 1024, ef_search : 200
     public void testKNNIndexCustomMethodFieldMapping() throws Exception {
         if (isRunningAgainstOldCluster()) {
             createKnnIndex(
                 testIndex,
                 getKNNDefaultIndexSettings(),
-                createKNNIndexCustomMethodFieldMapping(TEST_FIELD, DIMENSIONS, SpaceType.INNER_PRODUCT, FAISS_NAME, M, EF_CONSTRUCTION)
+                createKNNIndexCustomMethodFieldMapping(TEST_FIELD, DIMENSIONS, SpaceType.INNER_PRODUCT, FAISS_NAME, M, EF_CONSTRUCTION, EF_SEARCH)
             );
             addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, DOC_ID, NUM_DOCS);
         } else {
+            validateCustomMethodFieldMappingAfterUpgrade();
             validateKNNIndexingOnUpgrade();
         }
     }
 
+    private void validateCustomMethodFieldMappingAfterUpgrade() throws IOException {
+        final Map<String, Object> indexMappings = getIndexMappingAsMap(testIndex);
+        final Map<String, Object> properties = (Map<String, Object>) indexMappings.get(PROPERTIES);
+        final Map<String,Object> knnMethod = ((Map<String,Object>)((Map<String, Object>) properties.get(TEST_FIELD)).get(KNN_METHOD));
+        final Map<String,Object> methodParameters = (Map<String, Object>)knnMethod.get(PARAMETERS);
+
+        Assert.assertEquals(METHOD_HNSW, knnMethod.get(NAME));
+        Assert.assertEquals(SpaceType.INNER_PRODUCT.getValue(), knnMethod.get(METHOD_PARAMETER_SPACE_TYPE));
+        Assert.assertEquals(FAISS_NAME, knnMethod.get(KNN_ENGINE));
+        Assert.assertEquals(EF_CONSTRUCTION, ((Integer) methodParameters.get(METHOD_PARAMETER_EF_CONSTRUCTION)).intValue());
+        Assert.assertEquals(EF_SEARCH, ((Integer) methodParameters.get(METHOD_PARAMETER_EF_SEARCH)).intValue());
+        Assert.assertEquals(M, ((Integer) methodParameters.get(METHOD_PARAMETER_M)).intValue());
+    }
+
     // test null parameters
     public void testNullParametersOnUpgrade() throws Exception {
         if (isRunningAgainstOldCluster()) {

src/main/java/org/opensearch/knn/common/KNNConstants.java

@@ -69,6 +69,7 @@ public class KNNConstants {
     // nmslib specific constants
     public static final String NMSLIB_NAME = "nmslib";
     public static final String SPACE_TYPE = "spaceType"; // used as field info key
+    public static final String VERSION = "version"; // used as field info key
     public static final String HNSW_ALGO_M = "M";
     public static final String HNSW_ALGO_EF_CONSTRUCTION = "efConstruction";
     public static final String HNSW_ALGO_EF_SEARCH = "efSearch";

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

@@ -0,0 +1,48 @@
+/*
+ * 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_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/KNNMethod.java

@@ -63,7 +63,7 @@ public ValidationException validate(KNNMethodContext knnMethodContext) {
             );
         }
 
-        ValidationException methodValidation = methodComponent.validate(knnMethodContext.getMethodComponent());
+        ValidationException methodValidation = methodComponent.validate(knnMethodContext.getMethodComponentContext());
         if (methodValidation != null) {
             errorMessages.addAll(methodValidation.validationErrors());
         }
@@ -84,7 +84,7 @@ public ValidationException validate(KNNMethodContext knnMethodContext) {
      * @return true if training is required; false otherwise
      */
     public boolean isTrainingRequired(KNNMethodContext knnMethodContext) {
-        return methodComponent.isTrainingRequired(knnMethodContext.getMethodComponent());
+        return methodComponent.isTrainingRequired(knnMethodContext.getMethodComponentContext());
     }
 
     /**
@@ -95,7 +95,7 @@ public boolean isTrainingRequired(KNNMethodContext knnMethodContext) {
      * @return estimate overhead in KB
      */
     public int estimateOverheadInKB(KNNMethodContext knnMethodContext, int dimension) {
-        return methodComponent.estimateOverheadInKB(knnMethodContext.getMethodComponent(), dimension);
+        return methodComponent.estimateOverheadInKB(knnMethodContext.getMethodComponentContext(), dimension);
     }
 
     /**
@@ -105,7 +105,7 @@ public int estimateOverheadInKB(KNNMethodContext knnMethodContext, int dimension
      * @return KNNMethod as a map
      */
     public Map<String, Object> getAsMap(KNNMethodContext knnMethodContext) {
-        Map<String, Object> parameterMap = new HashMap<>(methodComponent.getAsMap(knnMethodContext.getMethodComponent()));
+        Map<String, Object> parameterMap = new HashMap<>(methodComponent.getAsMap(knnMethodContext.getMethodComponentContext()));
         parameterMap.put(KNNConstants.SPACE_TYPE, knnMethodContext.getSpaceType().getValue());
         return parameterMap;
     }

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

@@ -11,9 +11,9 @@
 
 package org.opensearch.knn.index;
 
-import lombok.AllArgsConstructor;
 import lombok.Getter;
 import lombok.NonNull;
+import lombok.RequiredArgsConstructor;
 import org.opensearch.common.ValidationException;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
@@ -41,7 +41,7 @@
  * KNNMethodContext will contain the information necessary to produce a library index from an Opensearch mapping.
  * It will encompass all parameters necessary to build the index.
  */
-@AllArgsConstructor
+@RequiredArgsConstructor
 @Getter
 public class KNNMethodContext implements ToXContentFragment, Writeable {
 
@@ -63,7 +63,7 @@ public static synchronized KNNMethodContext getDefault() {
     @NonNull
     private final SpaceType spaceType;
     @NonNull
-    private final MethodComponentContext methodComponent;
+    private final MethodComponentContext methodComponentContext;
 
     /**
      * Constructor from stream.
@@ -74,7 +74,7 @@ public static synchronized KNNMethodContext getDefault() {
     public KNNMethodContext(StreamInput in) throws IOException {
         this.knnEngine = KNNEngine.getEngine(in.readString());
         this.spaceType = SpaceType.getSpace(in.readString());
-        this.methodComponent = new MethodComponentContext(in);
+        this.methodComponentContext = new MethodComponentContext(in);
     }
 
     /**
@@ -198,7 +198,7 @@ public static KNNMethodContext parse(Object in) {
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.field(KNN_ENGINE, knnEngine.getName());
         builder.field(METHOD_PARAMETER_SPACE_TYPE, spaceType.getValue());
-        builder = methodComponent.toXContent(builder, params);
+        builder = methodComponentContext.toXContent(builder, params);
         return builder;
     }
 
@@ -211,20 +211,20 @@ public boolean equals(Object obj) {
         EqualsBuilder equalsBuilder = new EqualsBuilder();
         equalsBuilder.append(knnEngine, other.knnEngine);
         equalsBuilder.append(spaceType, other.spaceType);
-        equalsBuilder.append(methodComponent, other.methodComponent);
+        equalsBuilder.append(methodComponentContext, other.methodComponentContext);
 
         return equalsBuilder.isEquals();
     }
 
     @Override
     public int hashCode() {
-        return new HashCodeBuilder().append(knnEngine).append(spaceType).append(methodComponent).toHashCode();
+        return new HashCodeBuilder().append(knnEngine).append(spaceType).append(methodComponentContext).toHashCode();
     }
 
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         out.writeString(knnEngine.getName());
         out.writeString(spaceType.getValue());
-        this.methodComponent.writeTo(out);
+        this.methodComponentContext.writeTo(out);
     }
 }

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

@@ -8,6 +8,7 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.opensearch.OpenSearchParseException;
+import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
 import org.opensearch.action.admin.cluster.settings.ClusterUpdateSettingsResponse;
@@ -21,6 +22,7 @@
 import org.opensearch.index.IndexModule;
 import org.opensearch.knn.index.memory.NativeMemoryCacheManager;
 import org.opensearch.knn.index.memory.NativeMemoryCacheManagerDto;
+import org.opensearch.knn.index.util.IndexHyperParametersUtil;
 import org.opensearch.monitor.jvm.JvmInfo;
 import org.opensearch.monitor.os.OsProbe;
 
@@ -80,8 +82,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_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
@@ -447,11 +449,12 @@ public void onFailure(Exception e) {
      * @return efSearch value
      */
     public static int getEfSearchParam(String index) {
-        return KNNSettings.state().clusterService.state()
-            .getMetadata()
-            .index(index)
-            .getSettings()
-            .getAsInt(KNNSettings.KNN_ALGO_PARAM_EF_SEARCH, 512);
+        final IndexMetadata indexMetadata = KNNSettings.state().clusterService.state().getMetadata().index(index);
+        return indexMetadata.getSettings()
+            .getAsInt(
+                KNNSettings.KNN_ALGO_PARAM_EF_SEARCH,
+                IndexHyperParametersUtil.getHNSWEFSearchValue(indexMetadata.getCreationVersion())
+            );
     }
 
     public void setClusterService(ClusterService clusterService) {

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

@@ -12,9 +12,11 @@
 package org.opensearch.knn.index;
 
 import lombok.Getter;
+import org.opensearch.Version;
 import org.opensearch.common.TriFunction;
 import org.opensearch.common.ValidationException;
 import org.opensearch.knn.common.KNNConstants;
+import org.opensearch.knn.index.util.IndexHyperParametersUtil;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -296,11 +298,24 @@ public static Map<String, Object> getParameterMapWithDefaultsAdded(
     ) {
         Map<String, Object> parametersWithDefaultsMap = new HashMap<>();
         Map<String, Object> userProvidedParametersMap = methodComponentContext.getParameters();
+        Version indexCreationVersion = methodComponentContext.getIndexVersion();
         for (Parameter<?> parameter : methodComponent.getParameters().values()) {
             if (methodComponentContext.getParameters().containsKey(parameter.getName())) {
                 parametersWithDefaultsMap.put(parameter.getName(), userProvidedParametersMap.get(parameter.getName()));
             } else {
-                parametersWithDefaultsMap.put(parameter.getName(), parameter.getDefaultValue());
+                // Pick up new values here. Make sure that index settings is passed till here,
+                // Version.fromId(Integer.parseInt((String)context.indexSettings.settings.get("index.version.created")))
+                if (parameter.getName().equals(KNNConstants.METHOD_PARAMETER_EF_SEARCH)) {
+                    parametersWithDefaultsMap.put(parameter.getName(), IndexHyperParametersUtil.getHNSWEFSearchValue(indexCreationVersion));
+                } else if (parameter.getName().equals(KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION)) {
+                    parametersWithDefaultsMap.put(
+                        parameter.getName(),
+                        IndexHyperParametersUtil.getHNSWEFConstructionValue(indexCreationVersion)
+                    );
+                } else {
+                    parametersWithDefaultsMap.put(parameter.getName(), parameter.getDefaultValue());
+                }
+
             }
         }
 

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

@@ -11,8 +11,10 @@
 
 package org.opensearch.knn.index;
 
-import lombok.AllArgsConstructor;
 import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+import lombok.Setter;
+import org.opensearch.Version;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.common.io.stream.Writeable;
@@ -36,13 +38,18 @@
  *
  * Each component is composed of a name and a map of parameters.
  */
-@AllArgsConstructor
+@RequiredArgsConstructor
 public class MethodComponentContext implements ToXContentFragment, Writeable {
 
     @Getter
     private final String name;
     private final Map<String, Object> parameters;
 
+    // Need a code over here to make sure that we are reading and writing in stream this optional index version.
+    @Getter
+    @Setter
+    private Version indexVersion;
+
     /**
      * Constructor from stream.
      *

src/main/java/org/opensearch/knn/index/codec/BasePerFieldKnnVectorsFormat.java

@@ -47,7 +47,7 @@ public KnnVectorsFormat getKnnVectorsFormatForField(final String field) {
                 String.format("Cannot read field type for field [%s] because mapper service is not available", field)
             )
         ).fieldType(field);
-        var params = type.getKnnMethodContext().getMethodComponent().getParameters();
+        var params = type.getKnnMethodContext().getMethodComponentContext().getParameters();
         int maxConnections = getMaxConnections(params);
         int beamWidth = getBeamWidth(params);
         log.debug(