From d8fd941b3006b529060b05234bf1a5fff8a49515 Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Tue, 22 Aug 2023 22:56:48 -0700 Subject: [PATCH] Changed the default values for ef_search and ef_constrction for enabling better indexing and search latency for vector search Signed-off-by: Navneet Verma --- .../org/opensearch/knn/bwc/IndexingIT.java | 29 ++++++- .../opensearch/knn/common/KNNConstants.java | 1 + .../knn/index/KNNIndexSettingProvider.java | 48 +++++++++++ .../org/opensearch/knn/index/KNNMethod.java | 8 +- .../knn/index/KNNMethodContext.java | 16 ++-- .../org/opensearch/knn/index/KNNSettings.java | 17 ++-- .../opensearch/knn/index/MethodComponent.java | 17 +++- .../knn/index/MethodComponentContext.java | 11 ++- .../codec/BasePerFieldKnnVectorsFormat.java | 2 +- .../index/mapper/KNNVectorFieldMapper.java | 29 +++++-- .../knn/index/mapper/LegacyFieldMapper.java | 23 +++-- .../knn/index/mapper/LuceneFieldMapper.java | 4 +- .../knn/index/mapper/MethodFieldMapper.java | 12 ++- .../knn/index/mapper/ModelFieldMapper.java | 11 ++- .../knn/index/util/AbstractKNNLibrary.java | 10 ++- .../index/util/IndexHyperParametersUtil.java | 64 ++++++++++++++ .../knn/index/util/NativeLibrary.java | 2 +- .../org/opensearch/knn/jni/JNIService.java | 3 + .../org/opensearch/knn/plugin/KNNPlugin.java | 12 +++ .../opensearch/knn/index/IndexUtilTests.java | 2 + .../knn/index/KNNMethodContextTests.java | 18 ++-- .../KNN80DocValuesConsumerTests.java | 2 + .../mapper/KNNVectorFieldMapperTests.java | 21 ++--- .../opensearch/knn/index/util/FaissTests.java | 3 + .../opensearch/knn/jni/JNIServiceTests.java | 2 + .../org/opensearch/knn/KNNRestTestCase.java | 83 ++++++++++++------- 26 files changed, 354 insertions(+), 96 deletions(-) create mode 100644 src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java create mode 100644 src/main/java/org/opensearch/knn/index/util/IndexHyperParametersUtil.java 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 c8d0cac934..9a385ce97c 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 @@ -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 indexMappings = getIndexMappingAsMap(testIndex); + final Map properties = (Map) indexMappings.get(PROPERTIES); + final Map knnMethod = ((Map)((Map) properties.get(TEST_FIELD)).get(KNN_METHOD)); + final Map methodParameters = (Map)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()) { diff --git a/src/main/java/org/opensearch/knn/common/KNNConstants.java b/src/main/java/org/opensearch/knn/common/KNNConstants.java index 184067c359..7118750f7c 100644 --- a/src/main/java/org/opensearch/knn/common/KNNConstants.java +++ b/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"; diff --git a/src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java b/src/main/java/org/opensearch/knn/index/KNNIndexSettingProvider.java new file mode 100644 index 0000000000..fea202fcd6 --- /dev/null +++ b/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; + } +} diff --git a/src/main/java/org/opensearch/knn/index/KNNMethod.java b/src/main/java/org/opensearch/knn/index/KNNMethod.java index d46e8fccd0..2d3672d877 100644 --- a/src/main/java/org/opensearch/knn/index/KNNMethod.java +++ b/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 getAsMap(KNNMethodContext knnMethodContext) { - Map parameterMap = new HashMap<>(methodComponent.getAsMap(knnMethodContext.getMethodComponent())); + Map parameterMap = new HashMap<>(methodComponent.getAsMap(knnMethodContext.getMethodComponentContext())); parameterMap.put(KNNConstants.SPACE_TYPE, knnMethodContext.getSpaceType().getValue()); return parameterMap; } diff --git a/src/main/java/org/opensearch/knn/index/KNNMethodContext.java b/src/main/java/org/opensearch/knn/index/KNNMethodContext.java index 5f5a0f232a..15599f7df9 100644 --- a/src/main/java/org/opensearch/knn/index/KNNMethodContext.java +++ b/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); } } diff --git a/src/main/java/org/opensearch/knn/index/KNNSettings.java b/src/main/java/org/opensearch/knn/index/KNNSettings.java index 4356a06102..81a8f6f2e4 100644 --- a/src/main/java/org/opensearch/knn/index/KNNSettings.java +++ b/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) { diff --git a/src/main/java/org/opensearch/knn/index/MethodComponent.java b/src/main/java/org/opensearch/knn/index/MethodComponent.java index c904f15c3b..64603fb35e 100644 --- a/src/main/java/org/opensearch/knn/index/MethodComponent.java +++ b/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 getParameterMapWithDefaultsAdded( ) { Map parametersWithDefaultsMap = new HashMap<>(); Map 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()); + } + } } diff --git a/src/main/java/org/opensearch/knn/index/MethodComponentContext.java b/src/main/java/org/opensearch/knn/index/MethodComponentContext.java index cf10e9a646..7f413b44c7 100644 --- a/src/main/java/org/opensearch/knn/index/MethodComponentContext.java +++ b/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 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. * diff --git a/src/main/java/org/opensearch/knn/index/codec/BasePerFieldKnnVectorsFormat.java b/src/main/java/org/opensearch/knn/index/codec/BasePerFieldKnnVectorsFormat.java index d10ad98218..d1335e4b01 100644 --- a/src/main/java/org/opensearch/knn/index/codec/BasePerFieldKnnVectorsFormat.java +++ b/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( 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 346d4c2388..b876ca3cfd 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -7,6 +7,7 @@ import lombok.Getter; import lombok.extern.log4j.Log4j2; +import org.opensearch.Version; import org.opensearch.common.ValidationException; import org.opensearch.knn.common.KNNConstants; @@ -79,6 +80,10 @@ private static KNNVectorFieldMapper toType(FieldMapper in) { return (KNNVectorFieldMapper) in; } + // 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; + /** * Builder for KNNVectorFieldMapper. This class defines the set of parameters that can be applied to the knn_vector * field type @@ -139,7 +144,7 @@ public static class Builder extends ParametrizedFieldMapper.Builder { b.startObject(n); v.toXContent(b, ToXContent.EMPTY_PARAMS); b.endObject(); - }), m -> m.getMethodComponent().getName()).setValidator(v -> { + }), m -> m.getMethodComponentContext().getName()).setValidator(v -> { if (v == null) return; ValidationException validationException = null; @@ -167,9 +172,12 @@ public static class Builder extends ParametrizedFieldMapper.Builder { protected ModelDao modelDao; - public Builder(String name, ModelDao modelDao) { + protected Version indexCreatedVersion; + + public Builder(String name, ModelDao modelDao, Version indexCreatedVersion) { super(name); this.modelDao = modelDao; + this.indexCreatedVersion = indexCreatedVersion; } /** @@ -181,11 +189,12 @@ public Builder(String name, ModelDao modelDao) { * @param m m value of field * @param efConstruction efConstruction value of field */ - public Builder(String name, String spaceType, String m, String efConstruction) { + public Builder(String name, String spaceType, String m, String efConstruction, Version indexCreatedVersion) { super(name); this.spaceType = spaceType; this.m = m; this.efConstruction = efConstruction; + this.indexCreatedVersion = indexCreatedVersion; } @Override @@ -221,6 +230,7 @@ public KNNVectorFieldMapper build(BuilderContext context) { final Map metaValue = meta.getValue(); if (knnMethodContext != null) { + knnMethodContext.getMethodComponentContext().setIndexVersion(indexCreatedVersion); final KNNVectorFieldType mappedFieldType = new KNNVectorFieldType( buildFullName(context), metaValue, @@ -292,7 +302,7 @@ public KNNVectorFieldMapper build(BuilderContext context) { } if (this.efConstruction == null) { - this.efConstruction = LegacyFieldMapper.getEfConstruction(context.indexSettings()); + this.efConstruction = LegacyFieldMapper.getEfConstruction(context.indexSettings(), indexCreatedVersion); } // Validates and throws exception if index.knn is set to true in the index settings @@ -310,7 +320,8 @@ public KNNVectorFieldMapper build(BuilderContext context) { hasDocValues.get(), spaceType, m, - efConstruction + efConstruction, + indexCreatedVersion ); } @@ -346,7 +357,7 @@ public TypeParser(Supplier modelDaoSupplier) { @Override public Mapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { - Builder builder = new KNNVectorFieldMapper.Builder(name, modelDaoSupplier.get()); + Builder builder = new KNNVectorFieldMapper.Builder(name, modelDaoSupplier.get(), parserContext.indexVersionCreated()); builder.parse(name, parserContext, node); // All ignoreMalformed, boolean stored, - boolean hasDocValues + boolean hasDocValues, + Version indexCreatedVersion ) { super(simpleName, mappedFieldType, multiFields, copyTo); this.ignoreMalformed = ignoreMalformed; @@ -469,6 +481,7 @@ public KNNVectorFieldMapper( this.dimension = mappedFieldType.getDimension(); this.vectorDataType = mappedFieldType.getVectorDataType(); updateEngineStats(); + this.indexCreatedVersion = indexCreatedVersion; } public KNNVectorFieldMapper clone() { @@ -610,7 +623,7 @@ protected boolean docValuesByDefault() { @Override public ParametrizedFieldMapper.Builder getMergeBuilder() { - return new KNNVectorFieldMapper.Builder(simpleName(), modelDao).init(this); + return new KNNVectorFieldMapper.Builder(simpleName(), modelDao, indexCreatedVersion).init(this); } @Override diff --git a/src/main/java/org/opensearch/knn/index/mapper/LegacyFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/LegacyFieldMapper.java index 868aec3e67..88947e4824 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/LegacyFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/LegacyFieldMapper.java @@ -7,10 +7,12 @@ import lombok.extern.log4j.Log4j2; import org.apache.lucene.document.FieldType; +import org.opensearch.Version; import org.opensearch.common.Explicit; import org.opensearch.common.settings.Settings; import org.opensearch.index.mapper.ParametrizedFieldMapper; import org.opensearch.knn.index.KNNSettings; +import org.opensearch.knn.index.util.IndexHyperParametersUtil; import org.opensearch.knn.index.util.KNNEngine; import static org.opensearch.knn.common.KNNConstants.DIMENSION; @@ -47,13 +49,16 @@ public class LegacyFieldMapper extends KNNVectorFieldMapper { boolean hasDocValues, String spaceType, String m, - String efConstruction + String efConstruction, + Version indexCreatedVersion ) { - super(simpleName, mappedFieldType, multiFields, copyTo, ignoreMalformed, stored, hasDocValues); + super(simpleName, mappedFieldType, multiFields, copyTo, ignoreMalformed, stored, hasDocValues, indexCreatedVersion); this.spaceType = spaceType; this.m = m; this.efConstruction = efConstruction; + // as we are creating a new object we need to set this value again. + // this.indexCreatedVersion = indexCreatedVersion; this.fieldType = new FieldType(KNNVectorFieldMapper.Defaults.FIELD_TYPE); @@ -70,7 +75,9 @@ public class LegacyFieldMapper extends KNNVectorFieldMapper { @Override public ParametrizedFieldMapper.Builder getMergeBuilder() { - return new KNNVectorFieldMapper.Builder(simpleName(), this.spaceType, this.m, this.efConstruction).init(this); + return new KNNVectorFieldMapper.Builder(simpleName(), this.spaceType, this.m, this.efConstruction, this.indexCreatedVersion).init( + this + ); } static String getSpaceType(Settings indexSettings) { @@ -103,17 +110,19 @@ static String getM(Settings indexSettings) { return m; } - static String getEfConstruction(Settings indexSettings) { + static String getEfConstruction(Settings indexSettings, Version indexVersion) { String efConstruction = indexSettings.get(KNNSettings.INDEX_KNN_ALGO_PARAM_EF_CONSTRUCTION_SETTING.getKey()); if (efConstruction == null) { + String defaultEFConstructionValue = String.valueOf(IndexHyperParametersUtil.getHNSWEFConstructionValue(indexVersion)); log.info( String.format( - "[KNN] The setting \"%s\" was not set for the index. Likely caused by recent version upgrade. Setting the setting to the default value=%s", + "[KNN] The setting \"%s\" was not set for the index. Likely caused by recent version upgrade. " + + "Picking up default value for the index =%s", HNSW_ALGO_EF_CONSTRUCTION, - KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION + defaultEFConstructionValue ) ); - return String.valueOf(KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION); + return String.valueOf(IndexHyperParametersUtil.getHNSWEFConstructionValue(indexVersion)); } return efConstruction; } diff --git a/src/main/java/org/opensearch/knn/index/mapper/LuceneFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/LuceneFieldMapper.java index b28b93028e..5a517bf05e 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/LuceneFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/LuceneFieldMapper.java @@ -47,7 +47,8 @@ public class LuceneFieldMapper extends KNNVectorFieldMapper { input.getCopyTo(), input.getIgnoreMalformed(), input.isStored(), - input.isHasDocValues() + input.isHasDocValues(), + input.getKnnMethodContext().getMethodComponentContext().getIndexVersion() ); vectorDataType = input.getVectorDataType(); @@ -74,6 +75,7 @@ public class LuceneFieldMapper extends KNNVectorFieldMapper { } else { this.vectorFieldType = null; } + // this.indexCreatedVersion = input.getKnnMethodContext().getMethodComponentContext().getIndexVersion(); } @Override diff --git a/src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java index 1db710bad6..d202f39f43 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java @@ -34,7 +34,16 @@ public class MethodFieldMapper extends KNNVectorFieldMapper { KNNMethodContext knnMethodContext ) { - super(simpleName, mappedFieldType, multiFields, copyTo, ignoreMalformed, stored, hasDocValues); + super( + simpleName, + mappedFieldType, + multiFields, + copyTo, + ignoreMalformed, + stored, + hasDocValues, + knnMethodContext.getMethodComponentContext().getIndexVersion() + ); this.knnMethod = knnMethodContext; @@ -54,6 +63,7 @@ public class MethodFieldMapper extends KNNVectorFieldMapper { } catch (IOException ioe) { throw new RuntimeException(String.format("Unable to create KNNVectorFieldMapper: %s", ioe)); } + // this.indexCreatedVersion = knnMethodContext.getMethodComponentContext().getIndexVersion(); this.fieldType.freeze(); } diff --git a/src/main/java/org/opensearch/knn/index/mapper/ModelFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/ModelFieldMapper.java index 0fa1169374..c8cbaf0232 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/ModelFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/ModelFieldMapper.java @@ -31,7 +31,16 @@ public class ModelFieldMapper extends KNNVectorFieldMapper { ModelDao modelDao, String modelId ) { - super(simpleName, mappedFieldType, multiFields, copyTo, ignoreMalformed, stored, hasDocValues); + super( + simpleName, + mappedFieldType, + multiFields, + copyTo, + ignoreMalformed, + stored, + hasDocValues, + null + ); this.modelId = modelId; this.modelDao = modelDao; diff --git a/src/main/java/org/opensearch/knn/index/util/AbstractKNNLibrary.java b/src/main/java/org/opensearch/knn/index/util/AbstractKNNLibrary.java index c5a3d1d1e2..f97d188102 100644 --- a/src/main/java/org/opensearch/knn/index/util/AbstractKNNLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/AbstractKNNLibrary.java @@ -35,22 +35,24 @@ public KNNMethod getMethod(String methodName) { @Override public ValidationException validateMethod(KNNMethodContext knnMethodContext) { - String methodName = knnMethodContext.getMethodComponent().getName(); + String methodName = knnMethodContext.getMethodComponentContext().getName(); return getMethod(methodName).validate(knnMethodContext); } @Override public boolean isTrainingRequired(KNNMethodContext knnMethodContext) { - String methodName = knnMethodContext.getMethodComponent().getName(); + String methodName = knnMethodContext.getMethodComponentContext().getName(); return getMethod(methodName).isTrainingRequired(knnMethodContext); } @Override public Map getMethodAsMap(KNNMethodContext knnMethodContext) { - KNNMethod knnMethod = methods.get(knnMethodContext.getMethodComponent().getName()); + KNNMethod knnMethod = methods.get(knnMethodContext.getMethodComponentContext().getName()); if (knnMethod == null) { - throw new IllegalArgumentException(String.format("Invalid method name: %s", knnMethodContext.getMethodComponent().getName())); + throw new IllegalArgumentException( + String.format("Invalid method name: %s", knnMethodContext.getMethodComponentContext().getName()) + ); } return knnMethod.getAsMap(knnMethodContext); diff --git a/src/main/java/org/opensearch/knn/index/util/IndexHyperParametersUtil.java b/src/main/java/org/opensearch/knn/index/util/IndexHyperParametersUtil.java new file mode 100644 index 0000000000..45d85b3ac3 --- /dev/null +++ b/src/main/java/org/opensearch/knn/index/util/IndexHyperParametersUtil.java @@ -0,0 +1,64 @@ +/* + * 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.util; + +import lombok.NonNull; +import lombok.extern.log4j.Log4j2; +import org.opensearch.Version; +import org.opensearch.knn.index.KNNSettings; + +/** + * This class acts as an abstraction to get the default hyperparameter values for different parameters used in the + * Nearest Neighbor Algorithm across different version of Index. + */ +@Log4j2 +public class IndexHyperParametersUtil { + + private static final int INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION_OLD_VALUE = 512; + private static final int INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH_OLD_VALUE = 512; + + public static int getHNSWEFConstructionValue(@NonNull final Version indexVersion) { + if (indexVersion.before(Version.V_2_12_0)) { + log.info( + "Picking up old values of ef_construction : index version : {}, value: {}", + indexVersion, + INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION_OLD_VALUE + ); + return INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION_OLD_VALUE; + } else { + log.info( + "Picking up new values of ef_construction : index version : {}, value: {}", + indexVersion, + KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION + ); + return KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION; + } + } + + public static int getHNSWEFSearchValue(@NonNull final Version indexVersion) { + if (indexVersion.before(Version.V_2_12_0)) { + log.info( + "Picking up old values of ef_search : index version : {}, value: {}", + indexVersion, + INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH_OLD_VALUE + ); + return INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH_OLD_VALUE; + } else { + log.info( + "Picking up new values of ef_search : index version : {}, value: {}", + indexVersion, + KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH + ); + return KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH; + } + } +} diff --git a/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java b/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java index 896c2b8895..5e264ed12e 100644 --- a/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java @@ -62,7 +62,7 @@ public float score(float rawScore, SpaceType spaceType) { @Override public int estimateOverheadInKB(KNNMethodContext knnMethodContext, int dimension) { - String methodName = knnMethodContext.getMethodComponent().getName(); + String methodName = knnMethodContext.getMethodComponentContext().getName(); return getMethod(methodName).estimateOverheadInKB(knnMethodContext, dimension); } diff --git a/src/main/java/org/opensearch/knn/jni/JNIService.java b/src/main/java/org/opensearch/knn/jni/JNIService.java index f45fb0c736..7f3ec85f0b 100644 --- a/src/main/java/org/opensearch/knn/jni/JNIService.java +++ b/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 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; diff --git a/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java b/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java index 53e9f31052..d4b3a7601d 100644 --- a/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java +++ b/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_MMAP_EXTENSIONS.getKey(), combinedSettings).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 getAdditionalIndexSettingProviders() { +// return List.of(new KNNIndexSettingProvider()); +// } } diff --git a/src/test/java/org/opensearch/knn/index/IndexUtilTests.java b/src/test/java/org/opensearch/knn/index/IndexUtilTests.java index 7013ef261a..5216d9cd1f 100644 --- a/src/test/java/org/opensearch/knn/index/IndexUtilTests.java +++ b/src/test/java/org/opensearch/knn/index/IndexUtilTests.java @@ -12,6 +12,7 @@ package org.opensearch.knn.index; import com.google.common.collect.ImmutableMap; +import org.opensearch.Version; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.Metadata; @@ -54,6 +55,7 @@ public void testGetLoadParameters() { Settings settings = Settings.builder().loadFromMap(indexSettings).build(); IndexMetadata indexMetadata = mock(IndexMetadata.class); when(indexMetadata.getSettings()).thenReturn(settings); + when(indexMetadata.getCreationVersion()).thenReturn(Version.CURRENT); Metadata metadata = mock(Metadata.class); when(metadata.index(anyString())).thenReturn(indexMetadata); ClusterState clusterState = mock(ClusterState.class); diff --git a/src/test/java/org/opensearch/knn/index/KNNMethodContextTests.java b/src/test/java/org/opensearch/knn/index/KNNMethodContextTests.java index 0b12980aba..1330e8da06 100644 --- a/src/test/java/org/opensearch/knn/index/KNNMethodContextTests.java +++ b/src/test/java/org/opensearch/knn/index/KNNMethodContextTests.java @@ -66,7 +66,7 @@ public void testStreams() throws IOException { public void testGetMethodComponent() { MethodComponentContext methodComponent = new MethodComponentContext("test-method", Collections.emptyMap()); KNNMethodContext knnMethodContext = new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, methodComponent); - assertEquals(methodComponent, knnMethodContext.getMethodComponent()); + assertEquals(methodComponent, knnMethodContext.getMethodComponentContext()); } /** @@ -275,7 +275,7 @@ public void testParse_nullParameters() throws IOException { .endObject(); Map in = xContentBuilderToMap(xContentBuilder); KNNMethodContext knnMethodContext = KNNMethodContext.parse(in); - assertTrue(knnMethodContext.getMethodComponent().getParameters().isEmpty()); + assertTrue(knnMethodContext.getMethodComponentContext().getParameters().isEmpty()); } /** @@ -291,8 +291,8 @@ public void testParse_valid() throws IOException { assertEquals(KNNEngine.DEFAULT, knnMethodContext.getKnnEngine()); assertEquals(SpaceType.DEFAULT, knnMethodContext.getSpaceType()); - assertEquals(methodName, knnMethodContext.getMethodComponent().getName()); - assertTrue(knnMethodContext.getMethodComponent().getParameters().isEmpty()); + assertEquals(methodName, knnMethodContext.getMethodComponentContext().getName()); + assertTrue(knnMethodContext.getMethodComponentContext().getParameters().isEmpty()); // Method with parameters String methodParameterKey1 = "p-1"; @@ -311,8 +311,8 @@ public void testParse_valid() throws IOException { in = xContentBuilderToMap(xContentBuilder); knnMethodContext = KNNMethodContext.parse(in); - assertEquals(methodParameterValue1, knnMethodContext.getMethodComponent().getParameters().get(methodParameterKey1)); - assertEquals(methodParameterValue2, knnMethodContext.getMethodComponent().getParameters().get(methodParameterKey2)); + assertEquals(methodParameterValue1, knnMethodContext.getMethodComponentContext().getParameters().get(methodParameterKey1)); + assertEquals(methodParameterValue2, knnMethodContext.getMethodComponentContext().getParameters().get(methodParameterKey2)); // Method with parameter that is a method context paramet @@ -330,12 +330,12 @@ public void testParse_valid() throws IOException { in = xContentBuilderToMap(xContentBuilder); knnMethodContext = KNNMethodContext.parse(in); - assertTrue(knnMethodContext.getMethodComponent().getParameters().get(methodParameterKey1) instanceof MethodComponentContext); + assertTrue(knnMethodContext.getMethodComponentContext().getParameters().get(methodParameterKey1) instanceof MethodComponentContext); assertEquals( methodParameterValue1, - ((MethodComponentContext) knnMethodContext.getMethodComponent().getParameters().get(methodParameterKey1)).getName() + ((MethodComponentContext) knnMethodContext.getMethodComponentContext().getParameters().get(methodParameterKey1)).getName() ); - assertEquals(methodParameterValue2, knnMethodContext.getMethodComponent().getParameters().get(methodParameterKey2)); + assertEquals(methodParameterValue2, knnMethodContext.getMethodComponentContext().getParameters().get(methodParameterKey2)); } /** 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 6af83de87a..0572d1820f 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 @@ -18,6 +18,7 @@ import org.apache.lucene.store.IOContext; import org.junit.AfterClass; import org.junit.BeforeClass; +import org.opensearch.Version; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; @@ -283,6 +284,7 @@ public void testAddKNNBinaryField_fromScratch_faissCurrent() throws IOException spaceType, new MethodComponentContext(METHOD_HNSW, ImmutableMap.of(METHOD_PARAMETER_M, 16, METHOD_PARAMETER_EF_CONSTRUCTION, 512)) ); + knnMethodContext.getMethodComponentContext().setIndexVersion(Version.CURRENT); String parameterString = XContentFactory.jsonBuilder().map(knnEngine.getMethodAsMap(knnMethodContext)).toString(); 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 1f35987819..a21b3b4d25 100644 --- a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java +++ b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java @@ -93,7 +93,7 @@ public class KNNVectorFieldMapperTests extends KNNTestCase { public void testBuilder_getParameters() { String fieldName = "test-field-name"; ModelDao modelDao = mock(ModelDao.class); - KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder(fieldName, modelDao); + KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder(fieldName, modelDao, CURRENT); assertEquals(7, builder.getParameters().size()); List actualParams = builder.getParameters().stream().map(a -> a.name).collect(Collectors.toList()); @@ -104,7 +104,8 @@ public void testBuilder_getParameters() { public void testBuilder_build_fromKnnMethodContext() { // Check that knnMethodContext takes precedent over both model and legacy ModelDao modelDao = mock(ModelDao.class); - KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder("test-field-name-1", modelDao); + KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder("test-field-name-1", modelDao, + CURRENT); SpaceType spaceType = SpaceType.COSINESIMIL; int m = 17; @@ -141,7 +142,7 @@ public void testBuilder_build_fromKnnMethodContext() { public void testBuilder_build_fromModel() { // Check that modelContext takes precedent over legacy ModelDao modelDao = mock(ModelDao.class); - KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder("test-field-name-1", modelDao); + KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder("test-field-name-1", modelDao, CURRENT); SpaceType spaceType = SpaceType.COSINESIMIL; int m = 17; @@ -178,7 +179,7 @@ public void testBuilder_build_fromModel() { public void testBuilder_build_fromLegacy() { // Check legacy is picked up if model context and method context are not set ModelDao modelDao = mock(ModelDao.class); - KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder("test-field-name-1", modelDao); + KNNVectorFieldMapper.Builder builder = new KNNVectorFieldMapper.Builder("test-field-name-1", modelDao, CURRENT); SpaceType spaceType = SpaceType.COSINESIMIL; int m = 17; @@ -236,10 +237,10 @@ public void testBuilder_parse_fromKnnMethodContext_luceneEngine() throws IOExcep Mapper.BuilderContext builderContext = new Mapper.BuilderContext(settings, new ContentPath()); builder.build(builderContext); - assertEquals(METHOD_HNSW, builder.knnMethodContext.get().getMethodComponent().getName()); + assertEquals(METHOD_HNSW, builder.knnMethodContext.get().getMethodComponentContext().getName()); assertEquals( efConstruction, - builder.knnMethodContext.get().getMethodComponent().getParameters().get(METHOD_PARAMETER_EF_CONSTRUCTION) + builder.knnMethodContext.get().getMethodComponentContext().getParameters().get(METHOD_PARAMETER_EF_CONSTRUCTION) ); assertTrue(KNNEngine.LUCENE.isInitialized()); @@ -259,8 +260,8 @@ public void testBuilder_parse_fromKnnMethodContext_luceneEngine() throws IOExcep buildParserContext(indexName, settings) ); - assertEquals(METHOD_HNSW, builder.knnMethodContext.get().getMethodComponent().getName()); - assertTrue(builderEmptyParams.knnMethodContext.get().getMethodComponent().getParameters().isEmpty()); + assertEquals(METHOD_HNSW, builder.knnMethodContext.get().getMethodComponentContext().getName()); + assertTrue(builderEmptyParams.knnMethodContext.get().getMethodComponentContext().getParameters().isEmpty()); XContentBuilder xContentBuilderUnsupportedParam = XContentFactory.jsonBuilder() .startObject() @@ -484,10 +485,10 @@ public void testTypeParser_parse_fromKnnMethodContext() throws IOException { buildParserContext(indexName, settings) ); - assertEquals(METHOD_HNSW, builder.knnMethodContext.get().getMethodComponent().getName()); + assertEquals(METHOD_HNSW, builder.knnMethodContext.get().getMethodComponentContext().getName()); assertEquals( efConstruction, - builder.knnMethodContext.get().getMethodComponent().getParameters().get(METHOD_PARAMETER_EF_CONSTRUCTION) + builder.knnMethodContext.get().getMethodComponentContext().getParameters().get(METHOD_PARAMETER_EF_CONSTRUCTION) ); // Test invalid parameter diff --git a/src/test/java/org/opensearch/knn/index/util/FaissTests.java b/src/test/java/org/opensearch/knn/index/util/FaissTests.java index 99a1537805..0e7bc64820 100644 --- a/src/test/java/org/opensearch/knn/index/util/FaissTests.java +++ b/src/test/java/org/opensearch/knn/index/util/FaissTests.java @@ -5,6 +5,7 @@ package org.opensearch.knn.index.util; +import org.opensearch.Version; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.knn.KNNTestCase; @@ -48,6 +49,7 @@ public void testGetMethodAsMap_whenMethodIsHNSWFlat_thenCreateCorrectIndexDescri .endObject(); Map in = xContentBuilderToMap(xContentBuilder); KNNMethodContext knnMethodContext = KNNMethodContext.parse(in); + knnMethodContext.getMethodComponentContext().setIndexVersion(Version.CURRENT); Map map = Faiss.INSTANCE.getMethodAsMap(knnMethodContext); @@ -76,6 +78,7 @@ public void testGetMethodAsMap_whenMethodIsHNSWPQ_thenCreateCorrectIndexDescript .endObject(); Map in = xContentBuilderToMap(xContentBuilder); KNNMethodContext knnMethodContext = KNNMethodContext.parse(in); + knnMethodContext.getMethodComponentContext().setIndexVersion(Version.CURRENT); Map map = Faiss.INSTANCE.getMethodAsMap(knnMethodContext); diff --git a/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java b/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java index d1a5be741d..185f2953d5 100644 --- a/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java +++ b/src/test/java/org/opensearch/knn/jni/JNIServiceTests.java @@ -14,6 +14,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.junit.BeforeClass; +import org.opensearch.Version; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.knn.KNNTestCase; @@ -840,6 +841,7 @@ public void testTrain_whenConfigurationIsHNSWPQ_thenSucceed() throws IOException .endObject(); Map in = xContentBuilderToMap(xContentBuilder); KNNMethodContext knnMethodContext = KNNMethodContext.parse(in); + knnMethodContext.getMethodComponentContext().setIndexVersion(Version.CURRENT); Map parameters = KNNEngine.FAISS.getMethodAsMap(knnMethodContext); byte[] faissIndex = JNIService.trainIndex(parameters, 128, trainPointer, FAISS_NAME); diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index 678f490fa5..1fa9ed401e 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -15,6 +15,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; @@ -50,6 +51,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; @@ -66,30 +68,6 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; -import static org.opensearch.knn.common.KNNConstants.DIMENSION; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_M; -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_ENCODER_PARAMETER; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NLIST; -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; -import static org.opensearch.knn.common.KNNConstants.MODEL_ERROR; -import static org.opensearch.knn.common.KNNConstants.MODEL_ID; -import static org.opensearch.knn.common.KNNConstants.MODEL_INDEX_MAPPING_PATH; -import static org.opensearch.knn.common.KNNConstants.MODEL_INDEX_NAME; -import static org.opensearch.knn.common.KNNConstants.MODEL_STATE; -import static org.opensearch.knn.common.KNNConstants.MODEL_TIMESTAMP; -import static org.opensearch.knn.common.KNNConstants.TRAIN_FIELD_PARAMETER; -import static org.opensearch.knn.common.KNNConstants.TRAIN_INDEX_PARAMETER; -import static org.opensearch.knn.common.KNNConstants.NAME; -import static org.opensearch.knn.common.KNNConstants.METHOD_HNSW; -import static org.opensearch.knn.common.KNNConstants.PARAMETERS; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_M; - import static org.opensearch.knn.TestUtils.NUMBER_OF_REPLICAS; import static org.opensearch.knn.TestUtils.NUMBER_OF_SHARDS; import static org.opensearch.knn.TestUtils.INDEX_KNN; @@ -100,6 +78,7 @@ import static org.opensearch.knn.TestUtils.QUERY_VALUE; import static org.opensearch.knn.TestUtils.computeGroundTruthValues; +import static org.opensearch.knn.common.KNNConstants.*; import static org.opensearch.knn.index.SpaceType.L2; import static org.opensearch.knn.index.memory.NativeMemoryCacheManager.GRAPH_COUNT; import static org.opensearch.knn.index.util.KNNEngine.FAISS; @@ -634,11 +613,27 @@ protected int getTotalGraphsInCache() throws IOException { * Get specific Index setting value from response */ protected String getIndexSettingByName(String indexName, String settingName) throws IOException { - @SuppressWarnings("unchecked") - Map settings = (Map) ((Map) 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 settings = (Map) XContentHelper.convertToMap(MediaTypeRegistry.JSON.xContent(), is, true) + .get(indexName); + Map defaultSettings = new HashMap<>(); + if (includeDefaults) { + defaultSettings = (Map) settings.get("defaults"); + } + Map userSettings = (Map) settings.get("settings"); + // logger.error("Default settings: {}, index settings {}", defaultSettings, userSettings); + return (String) (userSettings.get(settingName) == null ? defaultSettings.get(settingName) : userSettings.get(settingName)); + } } protected void createModelSystemIndex() throws IOException { @@ -987,6 +982,36 @@ public String createKNNIndexCustomMethodFieldMapping( .toString(); } + public String createKNNIndexCustomMethodFieldMapping( + String fieldName, + Integer dimensions, + SpaceType spaceType, + String engine, + Integer m, + Integer ef_construction, + Integer ef_search) throws IOException { + return XContentFactory.jsonBuilder() + .startObject() + .startObject(PROPERTIES) + .startObject(fieldName) + .field(VECTOR_TYPE, KNN_VECTOR) + .field(DIMENSION, dimensions.toString()) + .startObject(KNN_METHOD) + .field(NAME, METHOD_HNSW) + .field(METHOD_PARAMETER_SPACE_TYPE, spaceType.getValue()) + .field(KNN_ENGINE, engine) + .startObject(PARAMETERS) + .field(METHOD_PARAMETER_EF_CONSTRUCTION, ef_construction) + .field(METHOD_PARAMETER_M, m) + .field(METHOD_PARAMETER_EF_SEARCH, ef_search) + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .toString(); + } + // Default KNN script score settings protected Settings createKNNDefaultScriptScoreSettings() { return Settings.builder().put(NUMBER_OF_SHARDS, 1).put(NUMBER_OF_REPLICAS, 0).put(INDEX_KNN, false).build();