From 67e70e7c89d0f88a490e613063ff1abc36da70f9 Mon Sep 17 00:00:00 2001 From: John Mazanec Date: Wed, 6 Jul 2022 20:52:40 -0700 Subject: [PATCH 1/5] Refactor library and engine structure Refactors library and engine structure to be less coupled. Pulls classes and interfaces out into individual files. Additionally, renames package from util to engine. Signed-off-by: John Mazanec --- .../org/opensearch/knn/index/util/Faiss.java | 366 +++++++++++ .../opensearch/knn/index/util/KNNEngine.java | 4 +- .../opensearch/knn/index/util/KNNLibrary.java | 574 +----------------- .../opensearch/knn/index/util/LibVersion.java | 19 + .../knn/index/util/NativeLibrary.java | 136 +++++ .../org/opensearch/knn/index/util/Nmslib.java | 101 +++ .../opensearch/knn/index/util/FaissTests.java | 74 +++ .../knn/index/util/KNNEngineTests.java | 6 +- ...raryTests.java => NativeLibraryTests.java} | 75 +-- 9 files changed, 717 insertions(+), 638 deletions(-) create mode 100644 src/main/java/org/opensearch/knn/index/util/Faiss.java create mode 100644 src/main/java/org/opensearch/knn/index/util/LibVersion.java create mode 100644 src/main/java/org/opensearch/knn/index/util/NativeLibrary.java create mode 100644 src/main/java/org/opensearch/knn/index/util/Nmslib.java create mode 100644 src/test/java/org/opensearch/knn/index/util/FaissTests.java rename src/test/java/org/opensearch/knn/index/util/{KNNLibraryTests.java => NativeLibraryTests.java} (73%) diff --git a/src/main/java/org/opensearch/knn/index/util/Faiss.java b/src/main/java/org/opensearch/knn/index/util/Faiss.java new file mode 100644 index 000000000..f7ac1ffaf --- /dev/null +++ b/src/main/java/org/opensearch/knn/index/util/Faiss.java @@ -0,0 +1,366 @@ +/* + * 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 com.google.common.collect.ImmutableMap; +import org.opensearch.knn.common.KNNConstants; +import org.opensearch.knn.index.KNNMethod; +import org.opensearch.knn.index.KNNSettings; +import org.opensearch.knn.index.MethodComponent; +import org.opensearch.knn.index.MethodComponentContext; +import org.opensearch.knn.index.Parameter; +import org.opensearch.knn.index.SpaceType; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Function; + +import static org.opensearch.knn.common.KNNConstants.BYTES_PER_KILOBYTES; +import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_COUNT_DEFAULT; +import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_COUNT_LIMIT; +import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE; +import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE_DEFAULT; +import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE_LIMIT; +import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_M; +import static org.opensearch.knn.common.KNNConstants.FAISS_HNSW_DESCRIPTION; +import static org.opensearch.knn.common.KNNConstants.FAISS_IVF_DESCRIPTION; +import static org.opensearch.knn.common.KNNConstants.FAISS_PQ_DESCRIPTION; +import static org.opensearch.knn.common.KNNConstants.METHOD_ENCODER_PARAMETER; +import static org.opensearch.knn.common.KNNConstants.METHOD_HNSW; +import static org.opensearch.knn.common.KNNConstants.METHOD_IVF; +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_NLIST; +import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NLIST_DEFAULT; +import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NLIST_LIMIT; +import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NPROBES; +import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NPROBES_DEFAULT; +import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NPROBES_LIMIT; +import static org.opensearch.knn.common.KNNConstants.NAME; +import static org.opensearch.knn.common.KNNConstants.PARAMETERS; + +/** + * Implements NativeLibrary for the faiss native library + */ +class Faiss extends NativeLibrary { + + // Map that overrides OpenSearch score translation by space type of scores returned by faiss + private final static Map> SCORE_TRANSLATIONS = ImmutableMap.of( + SpaceType.INNER_PRODUCT, + rawScore -> SpaceType.INNER_PRODUCT.scoreTranslation(-1 * rawScore) + ); + + // Define encoders supported by faiss + private final static MethodComponentContext ENCODER_DEFAULT = new MethodComponentContext( + KNNConstants.ENCODER_FLAT, + Collections.emptyMap() + ); + + // TODO: To think about in future: for PQ, if dimension is not divisible by code count, PQ will fail. Right now, + // we do not have a way to base validation off of dimension. Failure will happen during training in JNI. + private final static Map encoderComponents = ImmutableMap.of( + KNNConstants.ENCODER_FLAT, + MethodComponent.Builder.builder(KNNConstants.ENCODER_FLAT) + .setMapGenerator( + ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( + KNNConstants.FAISS_FLAT_DESCRIPTION, + methodComponent, + methodComponentContext + ).build()) + ) + .build(), + KNNConstants.ENCODER_PQ, + MethodComponent.Builder.builder(KNNConstants.ENCODER_PQ) + .addParameter( + ENCODER_PARAMETER_PQ_M, + new Parameter.IntegerParameter( + ENCODER_PARAMETER_PQ_M, + ENCODER_PARAMETER_PQ_CODE_COUNT_DEFAULT, + v -> v > 0 && v < ENCODER_PARAMETER_PQ_CODE_COUNT_LIMIT + ) + ) + .addParameter( + ENCODER_PARAMETER_PQ_CODE_SIZE, + new Parameter.IntegerParameter( + ENCODER_PARAMETER_PQ_CODE_SIZE, + ENCODER_PARAMETER_PQ_CODE_SIZE_DEFAULT, + v -> v > 0 && v < ENCODER_PARAMETER_PQ_CODE_SIZE_LIMIT + ) + ) + .setRequiresTraining(true) + .setMapGenerator( + ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( + FAISS_PQ_DESCRIPTION, + methodComponent, + methodComponentContext + ).addParameter(ENCODER_PARAMETER_PQ_M, "", "").addParameter(ENCODER_PARAMETER_PQ_CODE_SIZE, "x", "").build()) + ) + .setOverheadInKBEstimator((methodComponent, methodComponentContext, dimension) -> { + // Size estimate formula: (4 * d * 2^code_size) / 1024 + 1 + + // Get value of code size passed in by user + Object codeSizeObject = methodComponentContext.getParameters().get(ENCODER_PARAMETER_PQ_CODE_SIZE); + + // If not specified, get default value of code size + if (codeSizeObject == null) { + Parameter codeSizeParameter = methodComponent.getParameters().get(ENCODER_PARAMETER_PQ_CODE_SIZE); + if (codeSizeParameter == null) { + throw new IllegalStateException(ENCODER_PARAMETER_PQ_CODE_SIZE + " is not a valid " + " parameter. This is a bug."); + } + + codeSizeObject = codeSizeParameter.getDefaultValue(); + } + + if (!(codeSizeObject instanceof Integer)) { + throw new IllegalStateException(ENCODER_PARAMETER_PQ_CODE_SIZE + " must be " + "an integer."); + } + + int codeSize = (Integer) codeSizeObject; + return ((4L * (1L << codeSize) * dimension) / BYTES_PER_KILOBYTES) + 1; + }) + .build() + ); + + // Define methods supported by faiss + private final static Map METHODS = ImmutableMap.of( + METHOD_HNSW, + KNNMethod.Builder.builder( + MethodComponent.Builder.builder(METHOD_HNSW) + .addParameter( + METHOD_PARAMETER_M, + new Parameter.IntegerParameter(METHOD_PARAMETER_M, KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_M, v -> v > 0) + ) + .addParameter( + METHOD_PARAMETER_EF_CONSTRUCTION, + new Parameter.IntegerParameter( + METHOD_PARAMETER_EF_CONSTRUCTION, + KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION, + v -> v > 0 + ) + ) + .addParameter( + METHOD_PARAMETER_EF_SEARCH, + new Parameter.IntegerParameter( + METHOD_PARAMETER_EF_SEARCH, + KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH, + v -> v > 0 + ) + ) + .addParameter( + METHOD_ENCODER_PARAMETER, + new Parameter.MethodComponentContextParameter(METHOD_ENCODER_PARAMETER, ENCODER_DEFAULT, encoderComponents) + ) + .setMapGenerator( + ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( + FAISS_HNSW_DESCRIPTION, + methodComponent, + methodComponentContext + ).addParameter(METHOD_PARAMETER_M, "", "").addParameter(METHOD_ENCODER_PARAMETER, ",", "").build()) + ) + .build() + ).addSpaces(SpaceType.L2, SpaceType.INNER_PRODUCT).build(), + METHOD_IVF, + KNNMethod.Builder.builder( + MethodComponent.Builder.builder(METHOD_IVF) + .addParameter( + METHOD_PARAMETER_NPROBES, + new Parameter.IntegerParameter( + METHOD_PARAMETER_NPROBES, + METHOD_PARAMETER_NPROBES_DEFAULT, + v -> v > 0 && v < METHOD_PARAMETER_NPROBES_LIMIT + ) + ) + .addParameter( + METHOD_PARAMETER_NLIST, + new Parameter.IntegerParameter( + METHOD_PARAMETER_NLIST, + METHOD_PARAMETER_NLIST_DEFAULT, + v -> v > 0 && v < METHOD_PARAMETER_NLIST_LIMIT + ) + ) + .addParameter( + METHOD_ENCODER_PARAMETER, + new Parameter.MethodComponentContextParameter(METHOD_ENCODER_PARAMETER, ENCODER_DEFAULT, encoderComponents) + ) + .setRequiresTraining(true) + .setMapGenerator( + ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( + FAISS_IVF_DESCRIPTION, + methodComponent, + methodComponentContext + ).addParameter(METHOD_PARAMETER_NLIST, "", "").addParameter(METHOD_ENCODER_PARAMETER, ",", "").build()) + ) + .setOverheadInKBEstimator((methodComponent, methodComponentContext, dimension) -> { + // Size estimate formula: (4 * nlists * d) / 1024 + 1 + + // Get value of nlists passed in by user + Object nlistObject = methodComponentContext.getParameters().get(METHOD_PARAMETER_NLIST); + + // If not specified, get default value of nlist + if (nlistObject == null) { + Parameter nlistParameter = methodComponent.getParameters().get(METHOD_PARAMETER_NLIST); + if (nlistParameter == null) { + throw new IllegalStateException(METHOD_PARAMETER_NLIST + " is not a valid " + " parameter. This is a bug."); + } + + nlistObject = nlistParameter.getDefaultValue(); + } + + if (!(nlistObject instanceof Integer)) { + throw new IllegalStateException(METHOD_PARAMETER_NLIST + " must be " + "an integer."); + } + + int centroids = (Integer) nlistObject; + return ((4L * centroids * dimension) / BYTES_PER_KILOBYTES) + 1; + }) + .build() + ).addSpaces(SpaceType.L2, SpaceType.INNER_PRODUCT).build() + ); + + public final static Faiss INSTANCE = new Faiss( + METHODS, + SCORE_TRANSLATIONS, + Version.LATEST.getBuildVersion(), + Version.LATEST.indexLibraryVersion(), + KNNConstants.FAISS_EXTENSION + ); + + /** + * Constructor for Faiss + * + * @param methods map of methods the native library supports + * @param scoreTranslation Map of translation of space type to scores returned by the library + * @param latestLibraryBuildVersion String representation of latest build version of the library + * @param latestLibraryVersion String representation of latest version of the library + * @param extension String representing the extension that library files should use + */ + private Faiss( + Map methods, + Map> scoreTranslation, + String latestLibraryBuildVersion, + String latestLibraryVersion, + String extension + ) { + super(methods, scoreTranslation, latestLibraryBuildVersion, latestLibraryVersion, extension); + } + + /** + * MethodAsMap builder is used to create the map that will be passed to the jni to create the faiss index. + * Faiss's index factory takes an "index description" that it uses to build the index. In this description, + * some parameters of the index can be configured; others need to be manually set. MethodMap builder creates + * the index description from a set of parameters and removes them from the map. On build, it sets the index + * description in the map and returns the processed map + */ + static class MethodAsMapBuilder { + String indexDescription; + MethodComponent methodComponent; + Map methodAsMap; + + /** + * Constructor + * + * @param baseDescription the basic description this component should start with + * @param methodComponent the method component that maps to this builder + * @param initialMap the initial parameter map that will be modified + */ + MethodAsMapBuilder(String baseDescription, MethodComponent methodComponent, Map initialMap) { + this.indexDescription = baseDescription; + this.methodComponent = methodComponent; + this.methodAsMap = initialMap; + } + + /** + * Add a parameter that will be used in the index description for the given method component + * + * @param parameterName name of the parameter + * @param prefix to append to the index description before the parameter + * @param suffix to append to the index description after the parameter + * @return this builder + */ + @SuppressWarnings("unchecked") + MethodAsMapBuilder addParameter(String parameterName, String prefix, String suffix) { + indexDescription += prefix; + + // When we add a parameter, what we are doing is taking it from the methods parameter and building it + // into the index description string faiss uses to create the index. + Map methodParameters = (Map) methodAsMap.get(PARAMETERS); + Parameter parameter = methodComponent.getParameters().get(parameterName); + Object value = methodParameters.containsKey(parameterName) ? methodParameters.get(parameterName) : parameter.getDefaultValue(); + + // Recursion is needed if the parameter is a method component context itself. + if (parameter instanceof Parameter.MethodComponentContextParameter) { + MethodComponentContext subMethodComponentContext = (MethodComponentContext) value; + MethodComponent subMethodComponent = ((Parameter.MethodComponentContextParameter) parameter).getMethodComponent( + subMethodComponentContext.getName() + ); + + Map subMethodAsMap = subMethodComponent.getAsMap(subMethodComponentContext); + indexDescription += subMethodAsMap.get(KNNConstants.INDEX_DESCRIPTION_PARAMETER); + subMethodAsMap.remove(KNNConstants.INDEX_DESCRIPTION_PARAMETER); + + // We replace parameterName with the map that contains only parameters that are not included in + // the method description + methodParameters.put(parameterName, subMethodAsMap); + } else { + // Just add the value to the method description and remove from map + indexDescription += value; + methodParameters.remove(parameterName); + } + + indexDescription += suffix; + return this; + } + + /** + * Build + * + * @return Method as a map + */ + Map build() { + methodAsMap.put(KNNConstants.INDEX_DESCRIPTION_PARAMETER, indexDescription); + return methodAsMap; + } + + static MethodAsMapBuilder builder( + String baseDescription, + MethodComponent methodComponent, + MethodComponentContext methodComponentContext + ) { + Map initialMap = new HashMap<>(); + initialMap.put(NAME, methodComponent.getName()); + initialMap.put(PARAMETERS, MethodComponent.getParameterMapWithDefaultsAdded(methodComponentContext, methodComponent)); + return new MethodAsMapBuilder(baseDescription, methodComponent, initialMap); + } + } + + private enum Version implements LibVersion { + V165("165"); + + public static final Version LATEST = V165; + + private final String buildVersion; + + Version(String buildVersion) { + this.buildVersion = buildVersion; + } + + public String indexLibraryVersion() { + return KNNConstants.FAISS_JNI_LIBRARY_NAME; + } + + public String getBuildVersion() { + return buildVersion; + } + } +} diff --git a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java index 365226a01..839cae7ff 100644 --- a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java +++ b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java @@ -42,8 +42,8 @@ public enum KNNEngine implements KNNLibrary { this.knnLibrary = knnLibrary; } - private String name; - private KNNLibrary knnLibrary; + private final String name; + private final KNNLibrary knnLibrary; /** * Get the engine diff --git a/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java b/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java index 767a9ad61..d24a0b280 100644 --- a/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java @@ -12,46 +12,11 @@ package org.opensearch.knn.index.util; import org.opensearch.common.ValidationException; -import org.opensearch.knn.common.KNNConstants; import org.opensearch.knn.index.KNNMethod; import org.opensearch.knn.index.KNNMethodContext; -import org.opensearch.knn.index.KNNSettings; -import org.opensearch.knn.index.MethodComponent; -import org.opensearch.knn.index.MethodComponentContext; -import org.opensearch.knn.index.Parameter; import org.opensearch.knn.index.SpaceType; -import com.google.common.collect.ImmutableMap; -import java.util.Collections; -import java.util.HashMap; import java.util.Map; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Function; - -import static org.opensearch.knn.common.KNNConstants.BYTES_PER_KILOBYTES; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_M; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_COUNT_DEFAULT; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_COUNT_LIMIT; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE_DEFAULT; -import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE_LIMIT; -import static org.opensearch.knn.common.KNNConstants.FAISS_HNSW_DESCRIPTION; -import static org.opensearch.knn.common.KNNConstants.FAISS_IVF_DESCRIPTION; -import static org.opensearch.knn.common.KNNConstants.FAISS_PQ_DESCRIPTION; -import static org.opensearch.knn.common.KNNConstants.METHOD_ENCODER_PARAMETER; -import static org.opensearch.knn.common.KNNConstants.METHOD_HNSW; -import static org.opensearch.knn.common.KNNConstants.METHOD_IVF; -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_NLIST; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NLIST_DEFAULT; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NLIST_LIMIT; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NPROBES; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NPROBES_DEFAULT; -import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_NPROBES_LIMIT; -import static org.opensearch.knn.common.KNNConstants.NAME; -import static org.opensearch.knn.common.KNNConstants.PARAMETERS; /** * KNNLibrary is an interface that helps the plugin communicate with k-NN libraries @@ -100,7 +65,7 @@ public interface KNNLibrary { * will return a score where the lower the score, the better the result. This is the opposite of how Lucene scores * documents. * - * @param rawScore returned by the library + * @param rawScore returned by the library * @param spaceType spaceType used to compute the score * @return Lucene score for the rawScore */ @@ -127,7 +92,7 @@ public interface KNNLibrary { * Estimate overhead of KNNMethodContext in Kilobytes. * * @param knnMethodContext to estimate size for - * @param dimension to estimate size for + * @param dimension to estimate size for * @return size overhead estimate in KB */ int estimateOverheadInKB(KNNMethodContext knnMethodContext, int dimension); @@ -153,539 +118,4 @@ public interface KNNLibrary { * @param isInitialized whether library has been initialized */ void setInitialized(Boolean isInitialized); - - /** - * Abstract implementation of KNNLibrary. It contains several default methods and fields that - * are common across different underlying libraries. - */ - abstract class NativeLibrary implements KNNLibrary { - protected Map methods; - private Map> scoreTranslation; - private String latestLibraryBuildVersion; - private String latestLibraryVersion; - private String extension; - private AtomicBoolean initialized; - - /** - * Constructor for NativeLibrary - * - * @param methods map of methods the native library supports - * @param scoreTranslation Map of translation of space type to scores returned by the library - * @param latestLibraryBuildVersion String representation of latest build version of the library - * @param latestLibraryVersion String representation of latest version of the library - * @param extension String representing the extension that library files should use - */ - public NativeLibrary( - Map methods, - Map> scoreTranslation, - String latestLibraryBuildVersion, - String latestLibraryVersion, - String extension - ) { - this.methods = methods; - this.scoreTranslation = scoreTranslation; - this.latestLibraryBuildVersion = latestLibraryBuildVersion; - this.latestLibraryVersion = latestLibraryVersion; - this.extension = extension; - this.initialized = new AtomicBoolean(false); - } - - @Override - public String getLatestBuildVersion() { - return this.latestLibraryBuildVersion; - } - - @Override - public String getLatestLibVersion() { - return this.latestLibraryVersion; - } - - @Override - public String getExtension() { - return this.extension; - } - - @Override - public String getCompoundExtension() { - return getExtension() + KNNConstants.COMPOUND_EXTENSION; - } - - @Override - public KNNMethod getMethod(String methodName) { - KNNMethod method = methods.get(methodName); - if (method != null) { - return method; - } - throw new IllegalArgumentException("Invalid method name: " + methodName); - } - - @Override - public float score(float rawScore, SpaceType spaceType) { - if (this.scoreTranslation.containsKey(spaceType)) { - return this.scoreTranslation.get(spaceType).apply(rawScore); - } - - return spaceType.scoreTranslation(rawScore); - } - - @Override - public ValidationException validateMethod(KNNMethodContext knnMethodContext) { - String methodName = knnMethodContext.getMethodComponent().getName(); - return getMethod(methodName).validate(knnMethodContext); - } - - @Override - public boolean isTrainingRequired(KNNMethodContext knnMethodContext) { - String methodName = knnMethodContext.getMethodComponent().getName(); - return getMethod(methodName).isTrainingRequired(knnMethodContext); - } - - @Override - public int estimateOverheadInKB(KNNMethodContext knnMethodContext, int dimension) { - String methodName = knnMethodContext.getMethodComponent().getName(); - return getMethod(methodName).estimateOverheadInKB(knnMethodContext, dimension); - } - - @Override - public Map getMethodAsMap(KNNMethodContext knnMethodContext) { - KNNMethod knnMethod = methods.get(knnMethodContext.getMethodComponent().getName()); - - if (knnMethod == null) { - throw new IllegalArgumentException("Invalid method name: " + knnMethodContext.getMethodComponent().getName()); - } - - return knnMethod.getAsMap(knnMethodContext); - } - - @Override - public Boolean isInitialized() { - return initialized.get(); - } - - @Override - public void setInitialized(Boolean isInitialized) { - initialized.set(isInitialized); - } - } - - /** - * Implements NativeLibrary for the nmslib native library - */ - class Nmslib extends NativeLibrary { - // ====================================== - // Constants pertaining to nmslib library - // ====================================== - public final static String HNSW_LIB_NAME = "hnsw"; - public final static String EXTENSION = ".hnsw"; - - public final static Map METHODS = ImmutableMap.of( - METHOD_HNSW, - KNNMethod.Builder.builder( - MethodComponent.Builder.builder(HNSW_LIB_NAME) - .addParameter( - METHOD_PARAMETER_M, - new Parameter.IntegerParameter(METHOD_PARAMETER_M, KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_M, v -> v > 0) - ) - .addParameter( - METHOD_PARAMETER_EF_CONSTRUCTION, - new Parameter.IntegerParameter( - METHOD_PARAMETER_EF_CONSTRUCTION, - KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION, - v -> v > 0 - ) - ) - .build() - ).addSpaces(SpaceType.L2, SpaceType.L1, SpaceType.LINF, SpaceType.COSINESIMIL, SpaceType.INNER_PRODUCT).build() - ); - - public final static Nmslib INSTANCE = new Nmslib( - METHODS, - Collections.emptyMap(), - Version.LATEST.getBuildVersion(), - Version.LATEST.indexLibraryVersion(), - EXTENSION - ); - - /** - * Constructor for Nmslib - * - * @param methods Set of methods the native library supports - * @param scoreTranslation Map of translation of space type to scores returned by the library - * @param latestLibraryBuildVersion String representation of latest build version of the library - * @param latestLibraryVersion String representation of latest version of the library - * @param extension String representing the extension that library files should use - */ - private Nmslib( - Map methods, - Map> scoreTranslation, - String latestLibraryBuildVersion, - String latestLibraryVersion, - String extension - ) { - super(methods, scoreTranslation, latestLibraryBuildVersion, latestLibraryVersion, extension); - } - - public enum Version { - /** - * Latest available nmslib version - */ - V2011("2011") { - @Override - public String indexLibraryVersion() { - return KNNConstants.NMSLIB_JNI_LIBRARY_NAME; - } - }; - - public static final Version LATEST = V2011; - - private String buildVersion; - - Version(String buildVersion) { - this.buildVersion = buildVersion; - } - - /** - * NMS library version used by the KNN codec - * @return nmslib name - */ - public abstract String indexLibraryVersion(); - - public String getBuildVersion() { - return buildVersion; - } - } - } - - /** - * Implements NativeLibrary for the faiss native library - */ - class Faiss extends NativeLibrary { - - // Map that overrides OpenSearch score translation by space type of scores returned by faiss - public final static Map> SCORE_TRANSLATIONS = ImmutableMap.of( - SpaceType.INNER_PRODUCT, - rawScore -> SpaceType.INNER_PRODUCT.scoreTranslation(-1 * rawScore) - ); - - // Define encoders supported by faiss - public final static MethodComponentContext ENCODER_DEFAULT = new MethodComponentContext( - KNNConstants.ENCODER_FLAT, - Collections.emptyMap() - ); - - // TODO: To think about in future: for PQ, if dimension is not divisible by code count, PQ will fail. Right now, - // we do not have a way to base validation off of dimension. Failure will happen during training in JNI. - public final static Map encoderComponents = ImmutableMap.of( - KNNConstants.ENCODER_FLAT, - MethodComponent.Builder.builder(KNNConstants.ENCODER_FLAT) - .setMapGenerator( - ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( - KNNConstants.FAISS_FLAT_DESCRIPTION, - methodComponent, - methodComponentContext - ).build()) - ) - .build(), - KNNConstants.ENCODER_PQ, - MethodComponent.Builder.builder(KNNConstants.ENCODER_PQ) - .addParameter( - ENCODER_PARAMETER_PQ_M, - new Parameter.IntegerParameter( - ENCODER_PARAMETER_PQ_M, - ENCODER_PARAMETER_PQ_CODE_COUNT_DEFAULT, - v -> v > 0 && v < ENCODER_PARAMETER_PQ_CODE_COUNT_LIMIT - ) - ) - .addParameter( - ENCODER_PARAMETER_PQ_CODE_SIZE, - new Parameter.IntegerParameter( - ENCODER_PARAMETER_PQ_CODE_SIZE, - ENCODER_PARAMETER_PQ_CODE_SIZE_DEFAULT, - v -> v > 0 && v < ENCODER_PARAMETER_PQ_CODE_SIZE_LIMIT - ) - ) - .setRequiresTraining(true) - .setMapGenerator( - ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( - FAISS_PQ_DESCRIPTION, - methodComponent, - methodComponentContext - ).addParameter(ENCODER_PARAMETER_PQ_M, "", "").addParameter(ENCODER_PARAMETER_PQ_CODE_SIZE, "x", "").build()) - ) - .setOverheadInKBEstimator((methodComponent, methodComponentContext, dimension) -> { - // Size estimate formula: (4 * d * 2^code_size) / 1024 + 1 - - // Get value of code size passed in by user - Object codeSizeObject = methodComponentContext.getParameters().get(ENCODER_PARAMETER_PQ_CODE_SIZE); - - // If not specified, get default value of code size - if (codeSizeObject == null) { - Object codeSizeParameter = methodComponent.getParameters().get(ENCODER_PARAMETER_PQ_CODE_SIZE); - if (codeSizeParameter == null) { - throw new IllegalStateException( - ENCODER_PARAMETER_PQ_CODE_SIZE + " is not a valid " + " parameter. This is a bug." - ); - } - - codeSizeObject = ((Parameter) codeSizeParameter).getDefaultValue(); - } - - if (!(codeSizeObject instanceof Integer)) { - throw new IllegalStateException(ENCODER_PARAMETER_PQ_CODE_SIZE + " must be " + "an integer."); - } - - int codeSize = (Integer) codeSizeObject; - return ((4L * (1 << codeSize) * dimension) / BYTES_PER_KILOBYTES) + 1; - }) - .build() - ); - - // Define methods supported by faiss - public final static Map METHODS = ImmutableMap.of( - METHOD_HNSW, - KNNMethod.Builder.builder( - MethodComponent.Builder.builder(METHOD_HNSW) - .addParameter( - METHOD_PARAMETER_M, - new Parameter.IntegerParameter(METHOD_PARAMETER_M, KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_M, v -> v > 0) - ) - .addParameter( - METHOD_PARAMETER_EF_CONSTRUCTION, - new Parameter.IntegerParameter( - METHOD_PARAMETER_EF_CONSTRUCTION, - KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION, - v -> v > 0 - ) - ) - .addParameter( - METHOD_PARAMETER_EF_SEARCH, - new Parameter.IntegerParameter( - METHOD_PARAMETER_EF_SEARCH, - KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_SEARCH, - v -> v > 0 - ) - ) - .addParameter( - METHOD_ENCODER_PARAMETER, - new Parameter.MethodComponentContextParameter(METHOD_ENCODER_PARAMETER, ENCODER_DEFAULT, encoderComponents) - ) - .setMapGenerator( - ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( - FAISS_HNSW_DESCRIPTION, - methodComponent, - methodComponentContext - ).addParameter(METHOD_PARAMETER_M, "", "").addParameter(METHOD_ENCODER_PARAMETER, ",", "").build()) - ) - .build() - ).addSpaces(SpaceType.L2, SpaceType.INNER_PRODUCT).build(), - METHOD_IVF, - KNNMethod.Builder.builder( - MethodComponent.Builder.builder(METHOD_IVF) - .addParameter( - METHOD_PARAMETER_NPROBES, - new Parameter.IntegerParameter( - METHOD_PARAMETER_NPROBES, - METHOD_PARAMETER_NPROBES_DEFAULT, - v -> v > 0 && v < METHOD_PARAMETER_NPROBES_LIMIT - ) - ) - .addParameter( - METHOD_PARAMETER_NLIST, - new Parameter.IntegerParameter( - METHOD_PARAMETER_NLIST, - METHOD_PARAMETER_NLIST_DEFAULT, - v -> v > 0 && v < METHOD_PARAMETER_NLIST_LIMIT - ) - ) - .addParameter( - METHOD_ENCODER_PARAMETER, - new Parameter.MethodComponentContextParameter(METHOD_ENCODER_PARAMETER, ENCODER_DEFAULT, encoderComponents) - ) - .setRequiresTraining(true) - .setMapGenerator( - ((methodComponent, methodComponentContext) -> MethodAsMapBuilder.builder( - FAISS_IVF_DESCRIPTION, - methodComponent, - methodComponentContext - ).addParameter(METHOD_PARAMETER_NLIST, "", "").addParameter(METHOD_ENCODER_PARAMETER, ",", "").build()) - ) - .setOverheadInKBEstimator((methodComponent, methodComponentContext, dimension) -> { - // Size estimate formula: (4 * nlists * d) / 1024 + 1 - - // Get value of nlists passed in by user - Object nlistObject = methodComponentContext.getParameters().get(METHOD_PARAMETER_NLIST); - - // If not specified, get default value of nlist - if (nlistObject == null) { - Object nlistParameter = methodComponent.getParameters().get(METHOD_PARAMETER_NLIST); - if (nlistParameter == null) { - throw new IllegalStateException(METHOD_PARAMETER_NLIST + " is not a valid " + " parameter. This is a bug."); - } - - nlistObject = ((Parameter) nlistParameter).getDefaultValue(); - } - - if (!(nlistObject instanceof Integer)) { - throw new IllegalStateException(METHOD_PARAMETER_NLIST + " must be " + "an integer."); - } - - int centroids = (Integer) nlistObject; - return ((4L * centroids * dimension) / BYTES_PER_KILOBYTES) + 1; - }) - .build() - ).addSpaces(SpaceType.L2, SpaceType.INNER_PRODUCT).build() - ); - - public final static Faiss INSTANCE = new Faiss( - METHODS, - SCORE_TRANSLATIONS, - Version.LATEST.getBuildVersion(), - Version.LATEST.indexLibraryVersion(), - KNNConstants.FAISS_EXTENSION - ); - - /** - * Constructor for Faiss - * - * @param methods map of methods the native library supports - * @param scoreTranslation Map of translation of space type to scores returned by the library - * @param latestLibraryBuildVersion String representation of latest build version of the library - * @param latestLibraryVersion String representation of latest version of the library - * @param extension String representing the extension that library files should use - */ - private Faiss( - Map methods, - Map> scoreTranslation, - String latestLibraryBuildVersion, - String latestLibraryVersion, - String extension - ) { - super(methods, scoreTranslation, latestLibraryBuildVersion, latestLibraryVersion, extension); - } - - /** - * MethodAsMap builder is used to create the map that will be passed to the jni to create the faiss index. - * Faiss's index factory takes an "index description" that it uses to build the index. In this description, - * some parameters of the index can be configured; others need to be manually set. MethodMap builder creates - * the index description from a set of parameters and removes them from the map. On build, it sets the index - * description in the map and returns the processed map - */ - protected static class MethodAsMapBuilder { - String indexDescription; - MethodComponent methodComponent; - Map methodAsMap; - - /** - * Constructor - * - * @param baseDescription the basic description this component should start with - * @param methodComponent the method component that maps to this builder - * @param initialMap the initial parameter map that will be modified - */ - MethodAsMapBuilder(String baseDescription, MethodComponent methodComponent, Map initialMap) { - this.indexDescription = baseDescription; - this.methodComponent = methodComponent; - this.methodAsMap = initialMap; - } - - /** - * Add a parameter that will be used in the index description for the given method component - * - * @param parameterName name of the parameter - * @param prefix to append to the index description before the parameter - * @param suffix to append to the index description after the parameter - * @return this builder - */ - @SuppressWarnings("unchecked") - MethodAsMapBuilder addParameter(String parameterName, String prefix, String suffix) { - indexDescription += prefix; - - // When we add a parameter, what we are doing is taking it from the methods parameter and building it - // into the index description string faiss uses to create the index. - Map methodParameters = (Map) methodAsMap.get(PARAMETERS); - Parameter parameter = methodComponent.getParameters().get(parameterName); - Object value = methodParameters.containsKey(parameterName) - ? methodParameters.get(parameterName) - : parameter.getDefaultValue(); - - // Recursion is needed if the parameter is a method component context itself. - if (parameter instanceof Parameter.MethodComponentContextParameter) { - MethodComponentContext subMethodComponentContext = (MethodComponentContext) value; - MethodComponent subMethodComponent = ((Parameter.MethodComponentContextParameter) parameter).getMethodComponent( - subMethodComponentContext.getName() - ); - - Map subMethodAsMap = subMethodComponent.getAsMap(subMethodComponentContext); - indexDescription += subMethodAsMap.get(KNNConstants.INDEX_DESCRIPTION_PARAMETER); - subMethodAsMap.remove(KNNConstants.INDEX_DESCRIPTION_PARAMETER); - - // We replace parameterName with the map that contains only parameters that are not included in - // the method description - methodParameters.put(parameterName, subMethodAsMap); - } else { - // Just add the value to the method description and remove from map - indexDescription += value; - methodParameters.remove(parameterName); - } - - indexDescription += suffix; - return this; - } - - /** - * Build - * - * @return Method as a map - */ - Map build() { - methodAsMap.put(KNNConstants.INDEX_DESCRIPTION_PARAMETER, indexDescription); - return methodAsMap; - } - - static MethodAsMapBuilder builder( - String baseDescription, - MethodComponent methodComponent, - MethodComponentContext methodComponentContext - ) { - Map initialMap = new HashMap<>(); - initialMap.put(NAME, methodComponent.getName()); - initialMap.put(PARAMETERS, MethodComponent.getParameterMapWithDefaultsAdded(methodComponentContext, methodComponent)); - return new MethodAsMapBuilder(baseDescription, methodComponent, initialMap); - } - } - - /** - * Enum containing information about faiss versioning - */ - private enum Version { - - /** - * Latest available nmslib version - */ - V165("165") { - @Override - public String indexLibraryVersion() { - return KNNConstants.FAISS_JNI_LIBRARY_NAME; - } - }; - - static final Version LATEST = V165; - - String buildVersion; - - Version(String buildVersion) { - this.buildVersion = buildVersion; - } - - /** - * Faiss version used by the KNN codec - * @return library name - */ - abstract String indexLibraryVersion(); - - String getBuildVersion() { - return buildVersion; - } - } - } } diff --git a/src/main/java/org/opensearch/knn/index/util/LibVersion.java b/src/main/java/org/opensearch/knn/index/util/LibVersion.java new file mode 100644 index 000000000..9f45fd137 --- /dev/null +++ b/src/main/java/org/opensearch/knn/index/util/LibVersion.java @@ -0,0 +1,19 @@ +/* + * 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; + +interface LibVersion { + + String indexLibraryVersion(); + + String getBuildVersion(); +} diff --git a/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java b/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java new file mode 100644 index 000000000..6d7642595 --- /dev/null +++ b/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java @@ -0,0 +1,136 @@ +/* + * 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 org.opensearch.common.ValidationException; +import org.opensearch.knn.common.KNNConstants; +import org.opensearch.knn.index.KNNMethod; +import org.opensearch.knn.index.KNNMethodContext; +import org.opensearch.knn.index.SpaceType; + +import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Function; + +/** + * Abstract implementation of KNNLibrary. It contains several default methods and fields that + * are common across different underlying libraries. + */ +abstract class NativeLibrary implements KNNLibrary { + protected Map methods; + private final Map> scoreTranslation; + private final String latestLibraryBuildVersion; + private final String latestLibraryVersion; + private final String extension; + private final AtomicBoolean initialized; + + /** + * Constructor for NativeLibrary + * + * @param methods map of methods the native library supports + * @param scoreTranslation Map of translation of space type to scores returned by the library + * @param latestLibraryBuildVersion String representation of latest build version of the library + * @param latestLibraryVersion String representation of latest version of the library + * @param extension String representing the extension that library files should use + */ + public NativeLibrary( + Map methods, + Map> scoreTranslation, + String latestLibraryBuildVersion, + String latestLibraryVersion, + String extension + ) { + this.methods = methods; + this.scoreTranslation = scoreTranslation; + this.latestLibraryBuildVersion = latestLibraryBuildVersion; + this.latestLibraryVersion = latestLibraryVersion; + this.extension = extension; + this.initialized = new AtomicBoolean(false); + } + + @Override + public String getLatestBuildVersion() { + return this.latestLibraryBuildVersion; + } + + @Override + public String getLatestLibVersion() { + return this.latestLibraryVersion; + } + + @Override + public String getExtension() { + return this.extension; + } + + @Override + public String getCompoundExtension() { + return getExtension() + KNNConstants.COMPOUND_EXTENSION; + } + + @Override + public KNNMethod getMethod(String methodName) { + KNNMethod method = methods.get(methodName); + if (method != null) { + return method; + } + throw new IllegalArgumentException("Invalid method name: " + methodName); + } + + @Override + public float score(float rawScore, SpaceType spaceType) { + if (this.scoreTranslation.containsKey(spaceType)) { + return this.scoreTranslation.get(spaceType).apply(rawScore); + } + + return spaceType.scoreTranslation(rawScore); + } + + @Override + public ValidationException validateMethod(KNNMethodContext knnMethodContext) { + String methodName = knnMethodContext.getMethodComponent().getName(); + return getMethod(methodName).validate(knnMethodContext); + } + + @Override + public boolean isTrainingRequired(KNNMethodContext knnMethodContext) { + String methodName = knnMethodContext.getMethodComponent().getName(); + return getMethod(methodName).isTrainingRequired(knnMethodContext); + } + + @Override + public int estimateOverheadInKB(KNNMethodContext knnMethodContext, int dimension) { + String methodName = knnMethodContext.getMethodComponent().getName(); + return getMethod(methodName).estimateOverheadInKB(knnMethodContext, dimension); + } + + @Override + public Map getMethodAsMap(KNNMethodContext knnMethodContext) { + KNNMethod knnMethod = methods.get(knnMethodContext.getMethodComponent().getName()); + + if (knnMethod == null) { + throw new IllegalArgumentException("Invalid method name: " + knnMethodContext.getMethodComponent().getName()); + } + + return knnMethod.getAsMap(knnMethodContext); + } + + @Override + public Boolean isInitialized() { + return initialized.get(); + } + + @Override + public void setInitialized(Boolean isInitialized) { + initialized.set(isInitialized); + } +} diff --git a/src/main/java/org/opensearch/knn/index/util/Nmslib.java b/src/main/java/org/opensearch/knn/index/util/Nmslib.java new file mode 100644 index 000000000..ca3ee642f --- /dev/null +++ b/src/main/java/org/opensearch/knn/index/util/Nmslib.java @@ -0,0 +1,101 @@ +/* + * 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 com.google.common.collect.ImmutableMap; +import org.opensearch.knn.common.KNNConstants; +import org.opensearch.knn.index.*; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Function; + +import static org.opensearch.knn.common.KNNConstants.*; +import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION; + +/** + * Implements NativeLibrary for the nmslib native library + */ +class Nmslib extends NativeLibrary { + // ====================================== + // Constants pertaining to nmslib library + // ====================================== + public final static String HNSW_LIB_NAME = "hnsw"; + public final static String EXTENSION = ".hnsw"; + + public final static Map METHODS = ImmutableMap.of( + METHOD_HNSW, + KNNMethod.Builder.builder( + MethodComponent.Builder.builder(HNSW_LIB_NAME) + .addParameter( + METHOD_PARAMETER_M, + new Parameter.IntegerParameter(METHOD_PARAMETER_M, KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_M, v -> v > 0) + ) + .addParameter( + METHOD_PARAMETER_EF_CONSTRUCTION, + new Parameter.IntegerParameter( + METHOD_PARAMETER_EF_CONSTRUCTION, + KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_EF_CONSTRUCTION, + v -> v > 0 + ) + ) + .build() + ).addSpaces(SpaceType.L2, SpaceType.L1, SpaceType.LINF, SpaceType.COSINESIMIL, SpaceType.INNER_PRODUCT).build() + ); + + public final static Nmslib INSTANCE = new Nmslib( + METHODS, + Collections.emptyMap(), + Version.LATEST.getBuildVersion(), + Version.LATEST.indexLibraryVersion(), + EXTENSION + ); + + /** + * Constructor for Nmslib + * + * @param methods Set of methods the native library supports + * @param scoreTranslation Map of translation of space type to scores returned by the library + * @param latestLibraryBuildVersion String representation of latest build version of the library + * @param latestLibraryVersion String representation of latest version of the library + * @param extension String representing the extension that library files should use + */ + private Nmslib( + Map methods, + Map> scoreTranslation, + String latestLibraryBuildVersion, + String latestLibraryVersion, + String extension + ) { + super(methods, scoreTranslation, latestLibraryBuildVersion, latestLibraryVersion, extension); + } + + public enum Version implements LibVersion { + V2011("2011"); + + public static final Version LATEST = V2011; + + private final String buildVersion; + + Version(String buildVersion) { + this.buildVersion = buildVersion; + } + + public String indexLibraryVersion() { + return KNNConstants.NMSLIB_JNI_LIBRARY_NAME; + } + + public String getBuildVersion() { + return buildVersion; + } + } +} diff --git a/src/test/java/org/opensearch/knn/index/util/FaissTests.java b/src/test/java/org/opensearch/knn/index/util/FaissTests.java new file mode 100644 index 000000000..831b4a84c --- /dev/null +++ b/src/test/java/org/opensearch/knn/index/util/FaissTests.java @@ -0,0 +1,74 @@ +/* + * 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 org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.knn.KNNTestCase; +import org.opensearch.knn.index.MethodComponent; +import org.opensearch.knn.index.MethodComponentContext; +import org.opensearch.knn.index.Parameter; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +import static org.opensearch.knn.common.KNNConstants.INDEX_DESCRIPTION_PARAMETER; +import static org.opensearch.knn.common.KNNConstants.NAME; +import static org.opensearch.knn.common.KNNConstants.PARAMETERS; + +public class FaissTests extends KNNTestCase { + + public void testMethodAsMapBuilder() throws IOException { + String methodName = "test-method"; + String methodDescription = "test-description"; + String parameter1 = "test-parameter-1"; + Integer value1 = 10; + Integer defaultValue1 = 1; + String parameter2 = "test-parameter-2"; + Integer value2 = 15; + Integer defaultValue2 = 2; + String parameter3 = "test-parameter-3"; + Integer defaultValue3 = 3; + MethodComponent methodComponent = MethodComponent.Builder.builder(methodName) + .addParameter(parameter1, new Parameter.IntegerParameter(parameter1, defaultValue1, value -> value > 0)) + .addParameter(parameter2, new Parameter.IntegerParameter(parameter2, defaultValue2, value -> value > 0)) + .addParameter(parameter3, new Parameter.IntegerParameter(parameter3, defaultValue3, value -> value > 0)) + .build(); + + XContentBuilder xContentBuilder = XContentFactory.jsonBuilder() + .startObject() + .field(NAME, methodName) + .startObject(PARAMETERS) + .field(parameter1, value1) + .field(parameter2, value2) + .endObject() + .endObject(); + Map in = xContentBuilderToMap(xContentBuilder); + MethodComponentContext methodComponentContext = MethodComponentContext.parse(in); + + Map expectedParametersMap = new HashMap<>(methodComponentContext.getParameters()); + expectedParametersMap.put(parameter3, defaultValue3); + expectedParametersMap.remove(parameter1); + Map expectedMap = new HashMap<>(); + expectedMap.put(PARAMETERS, expectedParametersMap); + expectedMap.put(NAME, methodName); + expectedMap.put(INDEX_DESCRIPTION_PARAMETER, methodDescription + value1); + + Map methodAsMap = Faiss.MethodAsMapBuilder.builder(methodDescription, methodComponent, methodComponentContext) + .addParameter(parameter1, "", "") + .build(); + + assertEquals(expectedMap, methodAsMap); + } + +} diff --git a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java index a62a556e7..5870d93c4 100644 --- a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java +++ b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java @@ -19,7 +19,7 @@ public class KNNEngineTests extends KNNTestCase { * Get latest build version from library */ public void testDelegateLibraryFunctions() { - assertEquals(KNNLibrary.Nmslib.INSTANCE.getLatestLibVersion(), KNNEngine.NMSLIB.getLatestLibVersion()); + assertEquals(Nmslib.INSTANCE.getLatestLibVersion(), KNNEngine.NMSLIB.getLatestLibVersion()); } /** @@ -38,9 +38,9 @@ public void testGetEngine() { } public void testGetEngineFromPath() { - String hnswPath1 = "test" + KNNLibrary.Nmslib.EXTENSION; + String hnswPath1 = "test" + Nmslib.EXTENSION; assertEquals(KNNEngine.NMSLIB, KNNEngine.getEngineNameFromPath(hnswPath1)); - String hnswPath2 = "test" + KNNLibrary.Nmslib.EXTENSION + KNNConstants.COMPOUND_EXTENSION; + String hnswPath2 = "test" + Nmslib.EXTENSION + KNNConstants.COMPOUND_EXTENSION; assertEquals(KNNEngine.NMSLIB, KNNEngine.getEngineNameFromPath(hnswPath2)); String faissPath1 = "test" + KNNConstants.FAISS_EXTENSION; diff --git a/src/test/java/org/opensearch/knn/index/util/KNNLibraryTests.java b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java similarity index 73% rename from src/test/java/org/opensearch/knn/index/util/KNNLibraryTests.java rename to src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java index 4f99c6833..c1f592fa2 100644 --- a/src/test/java/org/opensearch/knn/index/util/KNNLibraryTests.java +++ b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java @@ -11,19 +11,17 @@ package org.opensearch.knn.index.util; +import com.google.common.collect.ImmutableMap; +import org.opensearch.common.ValidationException; +import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.knn.KNNTestCase; import org.opensearch.knn.common.KNNConstants; import org.opensearch.knn.index.KNNMethod; import org.opensearch.knn.index.KNNMethodContext; import org.opensearch.knn.index.MethodComponent; import org.opensearch.knn.index.MethodComponentContext; -import org.opensearch.knn.index.Parameter; import org.opensearch.knn.index.SpaceType; -import com.google.common.collect.ImmutableMap; -import org.opensearch.common.ValidationException; -import org.opensearch.common.xcontent.XContentBuilder; -import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.knn.index.util.KNNLibrary.Faiss.MethodAsMapBuilder; import java.io.IOException; import java.util.Collections; @@ -31,15 +29,13 @@ import java.util.Map; import java.util.function.Function; -import static org.opensearch.knn.common.KNNConstants.INDEX_DESCRIPTION_PARAMETER; import static org.opensearch.knn.common.KNNConstants.NAME; -import static org.opensearch.knn.common.KNNConstants.PARAMETERS; -public class KNNLibraryTests extends KNNTestCase { +public class NativeLibraryTests extends KNNTestCase { /** * Test native library build version getter */ - public void testNativeLibrary_getLatestBuildVersion() { + public void testGetLatestBuildVersion() { String latestBuildVersion = "test-build-version"; TestNativeLibrary testNativeLibrary = new TestNativeLibrary( Collections.emptyMap(), @@ -54,7 +50,7 @@ public void testNativeLibrary_getLatestBuildVersion() { /** * Test native library version getter */ - public void testNativeLibrary_getLatestLibVersion() { + public void testGetLatestLibVersion() { String latestVersion = "test-lib-version"; TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", latestVersion, ""); assertEquals(latestVersion, testNativeLibrary.getLatestLibVersion()); @@ -63,7 +59,7 @@ public void testNativeLibrary_getLatestLibVersion() { /** * Test native library extension getter */ - public void testNativeLibrary_getExtension() { + public void testGetExtension() { String extension = ".extension"; TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", "", extension); assertEquals(extension, testNativeLibrary.getExtension()); @@ -72,7 +68,7 @@ public void testNativeLibrary_getExtension() { /** * Test native library compound extension getter */ - public void testNativeLibrary_getCompoundExtension() { + public void testGetCompoundExtension() { String extension = ".extension"; TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", "", extension); assertEquals(extension + "c", testNativeLibrary.getCompoundExtension()); @@ -81,7 +77,7 @@ public void testNativeLibrary_getCompoundExtension() { /** * Test native library compound extension getter */ - public void testNativeLibrary_getMethod() { + public void testGetMethod() { String methodName1 = "test-method-1"; KNNMethod knnMethod1 = KNNMethod.Builder.builder(MethodComponent.Builder.builder(methodName1).build()).build(); @@ -99,7 +95,7 @@ public void testNativeLibrary_getMethod() { /** * Test native library scoring override */ - public void testNativeLibrary_score() { + public void testScore() { Map> translationMap = ImmutableMap.of(SpaceType.L2, s -> s * 2); TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), translationMap, "", "", ""); // Test override @@ -112,7 +108,7 @@ public void testNativeLibrary_score() { /** * Test native library method validation */ - public void testNativeLibrary_validateMethod() throws IOException { + public void testValidateMethod() throws IOException { // Invalid - method not supported String methodName1 = "test-method-1"; KNNMethod knnMethod1 = KNNMethod.Builder.builder(MethodComponent.Builder.builder(methodName1).build()).build(); @@ -142,7 +138,7 @@ public ValidationException validate(KNNMethodContext knnMethodContext) { assertNotNull(testNativeLibrary2.validateMethod(knnMethodContext2)); } - public void testNativeLibrary_getMethodAsMap() { + public void testGetMethodAsMap() { String methodName = "test-method-1"; SpaceType spaceType = SpaceType.DEFAULT; Map generatedMap = ImmutableMap.of("test-key", "test-param"); @@ -178,50 +174,7 @@ public void testNativeLibrary_getMethodAsMap() { expectThrows(IllegalArgumentException.class, () -> testNativeLibrary.getMethodAsMap(invalidKnnMethodContext)); } - public void testFaiss_methodAsMapBuilder() throws IOException { - String methodName = "test-method"; - String methodDescription = "test-description"; - String parameter1 = "test-parameter-1"; - Integer value1 = 10; - Integer defaultValue1 = 1; - String parameter2 = "test-parameter-2"; - Integer value2 = 15; - Integer defaultValue2 = 2; - String parameter3 = "test-parameter-3"; - Integer defaultValue3 = 3; - MethodComponent methodComponent = MethodComponent.Builder.builder(methodName) - .addParameter(parameter1, new Parameter.IntegerParameter(parameter1, defaultValue1, value -> value > 0)) - .addParameter(parameter2, new Parameter.IntegerParameter(parameter2, defaultValue2, value -> value > 0)) - .addParameter(parameter3, new Parameter.IntegerParameter(parameter3, defaultValue3, value -> value > 0)) - .build(); - - XContentBuilder xContentBuilder = XContentFactory.jsonBuilder() - .startObject() - .field(NAME, methodName) - .startObject(PARAMETERS) - .field(parameter1, value1) - .field(parameter2, value2) - .endObject() - .endObject(); - Map in = xContentBuilderToMap(xContentBuilder); - MethodComponentContext methodComponentContext = MethodComponentContext.parse(in); - - Map expectedParametersMap = new HashMap<>(methodComponentContext.getParameters()); - expectedParametersMap.put(parameter3, defaultValue3); - expectedParametersMap.remove(parameter1); - Map expectedMap = new HashMap<>(); - expectedMap.put(PARAMETERS, expectedParametersMap); - expectedMap.put(NAME, methodName); - expectedMap.put(INDEX_DESCRIPTION_PARAMETER, methodDescription + value1); - - Map methodAsMap = MethodAsMapBuilder.builder(methodDescription, methodComponent, methodComponentContext) - .addParameter(parameter1, "", "") - .build(); - - assertEquals(expectedMap, methodAsMap); - } - - static class TestNativeLibrary extends KNNLibrary.NativeLibrary { + static class TestNativeLibrary extends NativeLibrary { /** * Constructor for TestNativeLibrary * From 4cfbea7d642a5479b808cfad06ddf5ee926e37dc Mon Sep 17 00:00:00 2001 From: John Mazanec Date: Mon, 11 Jul 2022 11:51:20 -0700 Subject: [PATCH 2/5] Simplify versioning related to engines Removes complex versioning related to the engines. Simplifies it to a single string. Signed-off-by: John Mazanec --- .../KNN80Codec/KNN80DocValuesConsumer.java | 4 +- .../org/opensearch/knn/index/util/Faiss.java | 35 +++---------- .../opensearch/knn/index/util/KNNEngine.java | 9 +--- .../opensearch/knn/index/util/KNNLibrary.java | 11 +--- .../opensearch/knn/index/util/LibVersion.java | 19 ------- .../knn/index/util/NativeLibrary.java | 21 +++----- .../org/opensearch/knn/index/util/Nmslib.java | 52 ++++++------------- .../KNN80DocValuesConsumerTests.java | 8 +-- .../knn/index/util/KNNEngineTests.java | 2 +- .../knn/index/util/NativeLibraryTests.java | 35 ++++--------- .../LibraryInitializedSupplierTests.java | 9 +--- 11 files changed, 53 insertions(+), 152 deletions(-) delete mode 100644 src/main/java/org/opensearch/knn/index/util/LibVersion.java diff --git a/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java b/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java index 36ffe16de..a3727e128 100644 --- a/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java +++ b/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java @@ -101,7 +101,7 @@ public void addKNNBinaryField(FieldInfo field, DocValuesProducer valuesProducer) engineFileName = buildEngineFileName( state.segmentInfo.name, - knnEngine.getLatestBuildVersion(), + knnEngine.getCurrentVersion(), field.name, knnEngine.getExtension() ); @@ -121,7 +121,7 @@ public void addKNNBinaryField(FieldInfo field, DocValuesProducer valuesProducer) engineFileName = buildEngineFileName( state.segmentInfo.name, - knnEngine.getLatestBuildVersion(), + knnEngine.getCurrentVersion(), field.name, knnEngine.getExtension() ); diff --git a/src/main/java/org/opensearch/knn/index/util/Faiss.java b/src/main/java/org/opensearch/knn/index/util/Faiss.java index f7ac1ffaf..926fc2e72 100644 --- a/src/main/java/org/opensearch/knn/index/util/Faiss.java +++ b/src/main/java/org/opensearch/knn/index/util/Faiss.java @@ -55,6 +55,8 @@ */ class Faiss extends NativeLibrary { + private final static String CURRENT_VERSION = "165"; + // Map that overrides OpenSearch score translation by space type of scores returned by faiss private final static Map> SCORE_TRANSLATIONS = ImmutableMap.of( SpaceType.INNER_PRODUCT, @@ -228,11 +230,10 @@ class Faiss extends NativeLibrary { ).addSpaces(SpaceType.L2, SpaceType.INNER_PRODUCT).build() ); - public final static Faiss INSTANCE = new Faiss( + final static Faiss INSTANCE = new Faiss( METHODS, SCORE_TRANSLATIONS, - Version.LATEST.getBuildVersion(), - Version.LATEST.indexLibraryVersion(), + CURRENT_VERSION, KNNConstants.FAISS_EXTENSION ); @@ -241,18 +242,16 @@ class Faiss extends NativeLibrary { * * @param methods map of methods the native library supports * @param scoreTranslation Map of translation of space type to scores returned by the library - * @param latestLibraryBuildVersion String representation of latest build version of the library - * @param latestLibraryVersion String representation of latest version of the library + * @param currentVersion String representation of current version of the library * @param extension String representing the extension that library files should use */ private Faiss( Map methods, Map> scoreTranslation, - String latestLibraryBuildVersion, - String latestLibraryVersion, + String currentVersion, String extension ) { - super(methods, scoreTranslation, latestLibraryBuildVersion, latestLibraryVersion, extension); + super(methods, scoreTranslation, currentVersion, extension); } /** @@ -343,24 +342,4 @@ static MethodAsMapBuilder builder( return new MethodAsMapBuilder(baseDescription, methodComponent, initialMap); } } - - private enum Version implements LibVersion { - V165("165"); - - public static final Version LATEST = V165; - - private final String buildVersion; - - Version(String buildVersion) { - this.buildVersion = buildVersion; - } - - public String indexLibraryVersion() { - return KNNConstants.FAISS_JNI_LIBRARY_NAME; - } - - public String getBuildVersion() { - return buildVersion; - } - } } diff --git a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java index 839cae7ff..c9390621f 100644 --- a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java +++ b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java @@ -91,13 +91,8 @@ public String getName() { } @Override - public String getLatestBuildVersion() { - return knnLibrary.getLatestBuildVersion(); - } - - @Override - public String getLatestLibVersion() { - return knnLibrary.getLatestLibVersion(); + public String getCurrentVersion() { + return knnLibrary.getCurrentVersion(); } @Override diff --git a/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java b/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java index d24a0b280..49bd427a8 100644 --- a/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java @@ -24,18 +24,11 @@ public interface KNNLibrary { /** - * Gets the library's latest build version + * Gets the library's current version * * @return the string representing the library's latest build version */ - String getLatestBuildVersion(); - - /** - * Gets the library's latest version - * - * @return the string representing the library's latest version - */ - String getLatestLibVersion(); + String getCurrentVersion(); /** * Gets the extension that files written with this library should have diff --git a/src/main/java/org/opensearch/knn/index/util/LibVersion.java b/src/main/java/org/opensearch/knn/index/util/LibVersion.java deleted file mode 100644 index 9f45fd137..000000000 --- a/src/main/java/org/opensearch/knn/index/util/LibVersion.java +++ /dev/null @@ -1,19 +0,0 @@ -/* - * 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; - -interface LibVersion { - - String indexLibraryVersion(); - - String getBuildVersion(); -} 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 6d7642595..e27ce78aa 100644 --- a/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java @@ -28,8 +28,7 @@ abstract class NativeLibrary implements KNNLibrary { protected Map methods; private final Map> scoreTranslation; - private final String latestLibraryBuildVersion; - private final String latestLibraryVersion; + private final String currentVersion; private final String extension; private final AtomicBoolean initialized; @@ -38,33 +37,25 @@ abstract class NativeLibrary implements KNNLibrary { * * @param methods map of methods the native library supports * @param scoreTranslation Map of translation of space type to scores returned by the library - * @param latestLibraryBuildVersion String representation of latest build version of the library - * @param latestLibraryVersion String representation of latest version of the library + * @param currentVersion String representation of current version of the library * @param extension String representing the extension that library files should use */ public NativeLibrary( Map methods, Map> scoreTranslation, - String latestLibraryBuildVersion, - String latestLibraryVersion, + String currentVersion, String extension ) { this.methods = methods; this.scoreTranslation = scoreTranslation; - this.latestLibraryBuildVersion = latestLibraryBuildVersion; - this.latestLibraryVersion = latestLibraryVersion; + this.currentVersion = currentVersion; this.extension = extension; this.initialized = new AtomicBoolean(false); } @Override - public String getLatestBuildVersion() { - return this.latestLibraryBuildVersion; - } - - @Override - public String getLatestLibVersion() { - return this.latestLibraryVersion; + public String getCurrentVersion() { + return this.currentVersion; } @Override diff --git a/src/main/java/org/opensearch/knn/index/util/Nmslib.java b/src/main/java/org/opensearch/knn/index/util/Nmslib.java index ca3ee642f..a96f83b10 100644 --- a/src/main/java/org/opensearch/knn/index/util/Nmslib.java +++ b/src/main/java/org/opensearch/knn/index/util/Nmslib.java @@ -12,8 +12,11 @@ package org.opensearch.knn.index.util; import com.google.common.collect.ImmutableMap; -import org.opensearch.knn.common.KNNConstants; -import org.opensearch.knn.index.*; +import org.opensearch.knn.index.KNNMethod; +import org.opensearch.knn.index.KNNSettings; +import org.opensearch.knn.index.MethodComponent; +import org.opensearch.knn.index.Parameter; +import org.opensearch.knn.index.SpaceType; import java.util.Collections; import java.util.Map; @@ -26,13 +29,13 @@ * Implements NativeLibrary for the nmslib native library */ class Nmslib extends NativeLibrary { - // ====================================== - // Constants pertaining to nmslib library - // ====================================== - public final static String HNSW_LIB_NAME = "hnsw"; - public final static String EXTENSION = ".hnsw"; - public final static Map METHODS = ImmutableMap.of( + final static String HNSW_LIB_NAME = "hnsw"; + final static String EXTENSION = ".hnsw"; + + final static String CURRENT_VERSION = "2011"; + + final static Map METHODS = ImmutableMap.of( METHOD_HNSW, KNNMethod.Builder.builder( MethodComponent.Builder.builder(HNSW_LIB_NAME) @@ -52,11 +55,10 @@ class Nmslib extends NativeLibrary { ).addSpaces(SpaceType.L2, SpaceType.L1, SpaceType.LINF, SpaceType.COSINESIMIL, SpaceType.INNER_PRODUCT).build() ); - public final static Nmslib INSTANCE = new Nmslib( + final static Nmslib INSTANCE = new Nmslib( METHODS, Collections.emptyMap(), - Version.LATEST.getBuildVersion(), - Version.LATEST.indexLibraryVersion(), + CURRENT_VERSION, EXTENSION ); @@ -65,37 +67,15 @@ class Nmslib extends NativeLibrary { * * @param methods Set of methods the native library supports * @param scoreTranslation Map of translation of space type to scores returned by the library - * @param latestLibraryBuildVersion String representation of latest build version of the library - * @param latestLibraryVersion String representation of latest version of the library + * @param currentVersion String representation of current version of the library * @param extension String representing the extension that library files should use */ private Nmslib( Map methods, Map> scoreTranslation, - String latestLibraryBuildVersion, - String latestLibraryVersion, + String currentVersion, String extension ) { - super(methods, scoreTranslation, latestLibraryBuildVersion, latestLibraryVersion, extension); - } - - public enum Version implements LibVersion { - V2011("2011"); - - public static final Version LATEST = V2011; - - private final String buildVersion; - - Version(String buildVersion) { - this.buildVersion = buildVersion; - } - - public String indexLibraryVersion() { - return KNNConstants.NMSLIB_JNI_LIBRARY_NAME; - } - - public String getBuildVersion() { - return buildVersion; - } + super(methods, scoreTranslation, currentVersion, extension); } } 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 adbaf74d7..35feed76e 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 @@ -180,7 +180,7 @@ public void testAddKNNBinaryField_fromScratch_nmslibCurrent() throws IOException // The document should be created in the correct location String expectedFile = KNNCodecUtil.buildEngineFileName( segmentName, - knnEngine.getLatestBuildVersion(), + knnEngine.getCurrentVersion(), fieldName, knnEngine.getExtension() ); @@ -229,7 +229,7 @@ public void testAddKNNBinaryField_fromScratch_nmslibLegacy() throws IOException // The document should be created in the correct location String expectedFile = KNNCodecUtil.buildEngineFileName( segmentName, - knnEngine.getLatestBuildVersion(), + knnEngine.getCurrentVersion(), fieldName, knnEngine.getExtension() ); @@ -285,7 +285,7 @@ public void testAddKNNBinaryField_fromScratch_faissCurrent() throws IOException // The document should be created in the correct location String expectedFile = KNNCodecUtil.buildEngineFileName( segmentName, - knnEngine.getLatestBuildVersion(), + knnEngine.getCurrentVersion(), fieldName, knnEngine.getExtension() ); @@ -366,7 +366,7 @@ public void testAddKNNBinaryField_fromModel_faiss() throws IOException, Executio // The document should be created in the correct location String expectedFile = KNNCodecUtil.buildEngineFileName( segmentName, - knnEngine.getLatestBuildVersion(), + knnEngine.getCurrentVersion(), fieldName, knnEngine.getExtension() ); diff --git a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java index 5870d93c4..a0750d9e7 100644 --- a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java +++ b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java @@ -19,7 +19,7 @@ public class KNNEngineTests extends KNNTestCase { * Get latest build version from library */ public void testDelegateLibraryFunctions() { - assertEquals(Nmslib.INSTANCE.getLatestLibVersion(), KNNEngine.NMSLIB.getLatestLibVersion()); + assertEquals(Nmslib.INSTANCE.getCurrentVersion(), KNNEngine.NMSLIB.getCurrentVersion()); } /** diff --git a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java index c1f592fa2..ad56588f0 100644 --- a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java +++ b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java @@ -35,25 +35,15 @@ public class NativeLibraryTests extends KNNTestCase { /** * Test native library build version getter */ - public void testGetLatestBuildVersion() { + public void testGetCurrentVersion() { String latestBuildVersion = "test-build-version"; TestNativeLibrary testNativeLibrary = new TestNativeLibrary( Collections.emptyMap(), Collections.emptyMap(), latestBuildVersion, - "", "" ); - assertEquals(latestBuildVersion, testNativeLibrary.getLatestBuildVersion()); - } - - /** - * Test native library version getter - */ - public void testGetLatestLibVersion() { - String latestVersion = "test-lib-version"; - TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", latestVersion, ""); - assertEquals(latestVersion, testNativeLibrary.getLatestLibVersion()); + assertEquals(latestBuildVersion, testNativeLibrary.getCurrentVersion()); } /** @@ -61,7 +51,7 @@ public void testGetLatestLibVersion() { */ public void testGetExtension() { String extension = ".extension"; - TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", "", extension); + TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", extension); assertEquals(extension, testNativeLibrary.getExtension()); } @@ -70,7 +60,7 @@ public void testGetExtension() { */ public void testGetCompoundExtension() { String extension = ".extension"; - TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", "", extension); + TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), "", extension); assertEquals(extension + "c", testNativeLibrary.getCompoundExtension()); } @@ -86,7 +76,7 @@ public void testGetMethod() { Map knnMethodMap = ImmutableMap.of(methodName1, knnMethod1, methodName2, knnMethod2); - TestNativeLibrary testNativeLibrary = new TestNativeLibrary(knnMethodMap, Collections.emptyMap(), "", "", ""); + TestNativeLibrary testNativeLibrary = new TestNativeLibrary(knnMethodMap, Collections.emptyMap(), "", ""); assertEquals(knnMethod1, testNativeLibrary.getMethod(methodName1)); assertEquals(knnMethod2, testNativeLibrary.getMethod(methodName2)); expectThrows(IllegalArgumentException.class, () -> testNativeLibrary.getMethod("invalid")); @@ -97,7 +87,7 @@ public void testGetMethod() { */ public void testScore() { Map> translationMap = ImmutableMap.of(SpaceType.L2, s -> s * 2); - TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), translationMap, "", "", ""); + TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), translationMap, "", ""); // Test override assertEquals(2f, testNativeLibrary.score(1f, SpaceType.L2), 0.0001); @@ -114,7 +104,7 @@ public void testValidateMethod() throws IOException { KNNMethod knnMethod1 = KNNMethod.Builder.builder(MethodComponent.Builder.builder(methodName1).build()).build(); Map methodMap = ImmutableMap.of(methodName1, knnMethod1); - TestNativeLibrary testNativeLibrary1 = new TestNativeLibrary(methodMap, Collections.emptyMap(), "", "", ""); + TestNativeLibrary testNativeLibrary1 = new TestNativeLibrary(methodMap, Collections.emptyMap(), "", ""); XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().startObject().field(NAME, "invalid").endObject(); Map in = xContentBuilderToMap(xContentBuilder); @@ -131,7 +121,7 @@ public ValidationException validate(KNNMethodContext knnMethodContext) { }; methodMap = ImmutableMap.of(methodName2, knnMethod2); - TestNativeLibrary testNativeLibrary2 = new TestNativeLibrary(methodMap, Collections.emptyMap(), "", "", ""); + TestNativeLibrary testNativeLibrary2 = new TestNativeLibrary(methodMap, Collections.emptyMap(), "", ""); xContentBuilder = XContentFactory.jsonBuilder().startObject().field(NAME, methodName2).endObject(); in = xContentBuilderToMap(xContentBuilder); KNNMethodContext knnMethodContext2 = KNNMethodContext.parse(in); @@ -151,7 +141,6 @@ public void testGetMethodAsMap() { ImmutableMap.of(methodName, knnMethod), Collections.emptyMap(), "", - "", "" ); @@ -180,18 +169,16 @@ static class TestNativeLibrary extends NativeLibrary { * * @param methods map of methods the native library supports * @param scoreTranslation Map of translation of space type to scores returned by the library - * @param latestLibraryBuildVersion String representation of latest build version of the library - * @param latestLibraryVersion String representation of latest version of the library + * @param currentVersion String representation of current version of the library * @param extension String representing the extension that library files should use */ public TestNativeLibrary( Map methods, Map> scoreTranslation, - String latestLibraryBuildVersion, - String latestLibraryVersion, + String currentVersion, String extension ) { - super(methods, scoreTranslation, latestLibraryBuildVersion, latestLibraryVersion, extension); + super(methods, scoreTranslation, currentVersion, extension); } } } diff --git a/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java b/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java index 759f4dd5b..6154fcb5d 100644 --- a/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java +++ b/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java @@ -31,7 +31,7 @@ public void testEngineInitialized() { assertTrue(libraryInitializedSupplier.get()); } - private class TestLibrary implements KNNLibrary { + private static class TestLibrary implements KNNLibrary { private Boolean initialized; TestLibrary() { @@ -39,12 +39,7 @@ private class TestLibrary implements KNNLibrary { } @Override - public String getLatestBuildVersion() { - return null; - } - - @Override - public String getLatestLibVersion() { + public String getCurrentVersion() { return null; } From d34829fd4d07e4071b3ab96d2e29a185fe4ed915 Mon Sep 17 00:00:00 2001 From: John Mazanec Date: Mon, 11 Jul 2022 12:30:03 -0700 Subject: [PATCH 3/5] Fix spotless Signed-off-by: John Mazanec --- .../java/org/opensearch/knn/index/util/Faiss.java | 7 +------ .../java/org/opensearch/knn/index/util/Nmslib.java | 7 +------ .../knn/index/util/NativeLibraryTests.java | 14 ++------------ 3 files changed, 4 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/opensearch/knn/index/util/Faiss.java b/src/main/java/org/opensearch/knn/index/util/Faiss.java index 926fc2e72..9b4b96f0d 100644 --- a/src/main/java/org/opensearch/knn/index/util/Faiss.java +++ b/src/main/java/org/opensearch/knn/index/util/Faiss.java @@ -230,12 +230,7 @@ class Faiss extends NativeLibrary { ).addSpaces(SpaceType.L2, SpaceType.INNER_PRODUCT).build() ); - final static Faiss INSTANCE = new Faiss( - METHODS, - SCORE_TRANSLATIONS, - CURRENT_VERSION, - KNNConstants.FAISS_EXTENSION - ); + final static Faiss INSTANCE = new Faiss(METHODS, SCORE_TRANSLATIONS, CURRENT_VERSION, KNNConstants.FAISS_EXTENSION); /** * Constructor for Faiss diff --git a/src/main/java/org/opensearch/knn/index/util/Nmslib.java b/src/main/java/org/opensearch/knn/index/util/Nmslib.java index a96f83b10..e8a17c63f 100644 --- a/src/main/java/org/opensearch/knn/index/util/Nmslib.java +++ b/src/main/java/org/opensearch/knn/index/util/Nmslib.java @@ -55,12 +55,7 @@ class Nmslib extends NativeLibrary { ).addSpaces(SpaceType.L2, SpaceType.L1, SpaceType.LINF, SpaceType.COSINESIMIL, SpaceType.INNER_PRODUCT).build() ); - final static Nmslib INSTANCE = new Nmslib( - METHODS, - Collections.emptyMap(), - CURRENT_VERSION, - EXTENSION - ); + final static Nmslib INSTANCE = new Nmslib(METHODS, Collections.emptyMap(), CURRENT_VERSION, EXTENSION); /** * Constructor for Nmslib diff --git a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java index ad56588f0..1ad7ebab5 100644 --- a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java +++ b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java @@ -37,12 +37,7 @@ public class NativeLibraryTests extends KNNTestCase { */ public void testGetCurrentVersion() { String latestBuildVersion = "test-build-version"; - TestNativeLibrary testNativeLibrary = new TestNativeLibrary( - Collections.emptyMap(), - Collections.emptyMap(), - latestBuildVersion, - "" - ); + TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), latestBuildVersion, ""); assertEquals(latestBuildVersion, testNativeLibrary.getCurrentVersion()); } @@ -137,12 +132,7 @@ public void testGetMethodAsMap() { .build(); KNNMethod knnMethod = KNNMethod.Builder.builder(methodComponent).build(); - TestNativeLibrary testNativeLibrary = new TestNativeLibrary( - ImmutableMap.of(methodName, knnMethod), - Collections.emptyMap(), - "", - "" - ); + TestNativeLibrary testNativeLibrary = new TestNativeLibrary(ImmutableMap.of(methodName, knnMethod), Collections.emptyMap(), "", ""); // Check that map is expected Map expectedMap = new HashMap<>(generatedMap); From 382061047dbfc605b1d8580ba8bfd176670ddd37 Mon Sep 17 00:00:00 2001 From: John Mazanec Date: Tue, 12 Jul 2022 14:50:28 -0700 Subject: [PATCH 4/5] Address comments from review Signed-off-by: John Mazanec --- .../org/opensearch/knn/index/util/Faiss.java | 35 ++++++------------- .../opensearch/knn/index/util/KNNEngine.java | 10 ++---- .../knn/index/util/NativeLibrary.java | 16 ++++----- .../org/opensearch/knn/index/util/Nmslib.java | 15 +++----- .../opensearch/knn/index/util/FaissTests.java | 8 +---- .../knn/index/util/KNNEngineTests.java | 8 +---- .../knn/index/util/NativeLibraryTests.java | 8 +---- 7 files changed, 27 insertions(+), 73 deletions(-) diff --git a/src/main/java/org/opensearch/knn/index/util/Faiss.java b/src/main/java/org/opensearch/knn/index/util/Faiss.java index 9b4b96f0d..6c091da03 100644 --- a/src/main/java/org/opensearch/knn/index/util/Faiss.java +++ b/src/main/java/org/opensearch/knn/index/util/Faiss.java @@ -1,17 +1,12 @@ /* + * Copyright OpenSearch Contributors * 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 com.google.common.collect.ImmutableMap; +import lombok.AllArgsConstructor; import org.opensearch.knn.common.KNNConstants; import org.opensearch.knn.index.KNNMethod; import org.opensearch.knn.index.KNNSettings; @@ -118,14 +113,16 @@ class Faiss extends NativeLibrary { if (codeSizeObject == null) { Parameter codeSizeParameter = methodComponent.getParameters().get(ENCODER_PARAMETER_PQ_CODE_SIZE); if (codeSizeParameter == null) { - throw new IllegalStateException(ENCODER_PARAMETER_PQ_CODE_SIZE + " is not a valid " + " parameter. This is a bug."); + throw new IllegalStateException( + String.format("%s is not a valid parameter. This is a bug.", ENCODER_PARAMETER_PQ_CODE_SIZE) + ); } codeSizeObject = codeSizeParameter.getDefaultValue(); } if (!(codeSizeObject instanceof Integer)) { - throw new IllegalStateException(ENCODER_PARAMETER_PQ_CODE_SIZE + " must be " + "an integer."); + throw new IllegalStateException(String.format("%s must be an integer.", ENCODER_PARAMETER_PQ_CODE_SIZE)); } int codeSize = (Integer) codeSizeObject; @@ -213,14 +210,16 @@ class Faiss extends NativeLibrary { if (nlistObject == null) { Parameter nlistParameter = methodComponent.getParameters().get(METHOD_PARAMETER_NLIST); if (nlistParameter == null) { - throw new IllegalStateException(METHOD_PARAMETER_NLIST + " is not a valid " + " parameter. This is a bug."); + throw new IllegalStateException( + String.format("%s is not a valid parameter. This is a bug.", METHOD_PARAMETER_NLIST) + ); } nlistObject = nlistParameter.getDefaultValue(); } if (!(nlistObject instanceof Integer)) { - throw new IllegalStateException(METHOD_PARAMETER_NLIST + " must be " + "an integer."); + throw new IllegalStateException(String.format("%s must be an integer.", METHOD_PARAMETER_NLIST)); } int centroids = (Integer) nlistObject; @@ -256,24 +255,12 @@ private Faiss( * the index description from a set of parameters and removes them from the map. On build, it sets the index * description in the map and returns the processed map */ + @AllArgsConstructor static class MethodAsMapBuilder { String indexDescription; MethodComponent methodComponent; Map methodAsMap; - /** - * Constructor - * - * @param baseDescription the basic description this component should start with - * @param methodComponent the method component that maps to this builder - * @param initialMap the initial parameter map that will be modified - */ - MethodAsMapBuilder(String baseDescription, MethodComponent methodComponent, Map initialMap) { - this.indexDescription = baseDescription; - this.methodComponent = methodComponent; - this.methodAsMap = initialMap; - } - /** * Add a parameter that will be used in the index description for the given method component * diff --git a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java index c9390621f..12d1b9b00 100644 --- a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java +++ b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java @@ -1,12 +1,6 @@ /* + * Copyright OpenSearch Contributors * 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; @@ -60,7 +54,7 @@ public static KNNEngine getEngine(String name) { return FAISS; } - throw new IllegalArgumentException("Invalid engine type: " + name); + throw new IllegalArgumentException(String.format("Invalid engine type: %s", name)); } /** 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 e27ce78aa..ff1838a4b 100644 --- a/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java @@ -1,12 +1,6 @@ /* + * Copyright OpenSearch Contributors * 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; @@ -18,6 +12,7 @@ import org.opensearch.knn.index.SpaceType; import java.util.Map; +import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; @@ -40,7 +35,7 @@ abstract class NativeLibrary implements KNNLibrary { * @param currentVersion String representation of current version of the library * @param extension String representing the extension that library files should use */ - public NativeLibrary( + NativeLibrary( Map methods, Map> scoreTranslation, String currentVersion, @@ -74,7 +69,7 @@ public KNNMethod getMethod(String methodName) { if (method != null) { return method; } - throw new IllegalArgumentException("Invalid method name: " + methodName); + throw new IllegalArgumentException(String.format("Invalid method name: %s", methodName)); } @Override @@ -109,7 +104,7 @@ public Map getMethodAsMap(KNNMethodContext knnMethodContext) { KNNMethod knnMethod = methods.get(knnMethodContext.getMethodComponent().getName()); if (knnMethod == null) { - throw new IllegalArgumentException("Invalid method name: " + knnMethodContext.getMethodComponent().getName()); + throw new IllegalArgumentException(String.format("Invalid method name: %s", knnMethodContext.getMethodComponent().getName())); } return knnMethod.getAsMap(knnMethodContext); @@ -122,6 +117,7 @@ public Boolean isInitialized() { @Override public void setInitialized(Boolean isInitialized) { + Objects.requireNonNull(isInitialized, "isInitialized must not be null"); initialized.set(isInitialized); } } diff --git a/src/main/java/org/opensearch/knn/index/util/Nmslib.java b/src/main/java/org/opensearch/knn/index/util/Nmslib.java index e8a17c63f..617b311f4 100644 --- a/src/main/java/org/opensearch/knn/index/util/Nmslib.java +++ b/src/main/java/org/opensearch/knn/index/util/Nmslib.java @@ -1,12 +1,6 @@ /* + * Copyright OpenSearch Contributors * 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; @@ -22,15 +16,16 @@ import java.util.Map; import java.util.function.Function; -import static org.opensearch.knn.common.KNNConstants.*; +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_M; /** * Implements NativeLibrary for the nmslib native library */ class Nmslib extends NativeLibrary { - final static String HNSW_LIB_NAME = "hnsw"; + // Extension to be used for Nmslib files. It is ".hnsw" and not ".nmslib" for legacy purposes. final static String EXTENSION = ".hnsw"; final static String CURRENT_VERSION = "2011"; @@ -38,7 +33,7 @@ class Nmslib extends NativeLibrary { final static Map METHODS = ImmutableMap.of( METHOD_HNSW, KNNMethod.Builder.builder( - MethodComponent.Builder.builder(HNSW_LIB_NAME) + MethodComponent.Builder.builder(METHOD_HNSW) .addParameter( METHOD_PARAMETER_M, new Parameter.IntegerParameter(METHOD_PARAMETER_M, KNNSettings.INDEX_KNN_DEFAULT_ALGO_PARAM_M, v -> v > 0) 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 831b4a84c..abcb5e7f0 100644 --- a/src/test/java/org/opensearch/knn/index/util/FaissTests.java +++ b/src/test/java/org/opensearch/knn/index/util/FaissTests.java @@ -1,12 +1,6 @@ /* + * Copyright OpenSearch Contributors * 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; diff --git a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java index a0750d9e7..4cb0e5756 100644 --- a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java +++ b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java @@ -1,12 +1,6 @@ /* + * Copyright OpenSearch Contributors * 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; diff --git a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java index 1ad7ebab5..7cb6bc0a3 100644 --- a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java +++ b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java @@ -1,12 +1,6 @@ /* + * Copyright OpenSearch Contributors * 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; From 6c05fd6824bdf633dda987fdc054b645a7410f98 Mon Sep 17 00:00:00 2001 From: John Mazanec Date: Tue, 12 Jul 2022 17:02:27 -0700 Subject: [PATCH 5/5] Change getCurrentVersion for library to getVersion Signed-off-by: John Mazanec --- .../KNN80Codec/KNN80DocValuesConsumer.java | 14 ++-------- .../opensearch/knn/index/util/KNNEngine.java | 4 +-- .../opensearch/knn/index/util/KNNLibrary.java | 8 ++++-- .../knn/index/util/NativeLibrary.java | 2 +- .../KNN80DocValuesConsumerTests.java | 28 +++---------------- .../knn/index/util/KNNEngineTests.java | 4 +-- .../knn/index/util/NativeLibraryTests.java | 6 ++-- .../LibraryInitializedSupplierTests.java | 2 +- 8 files changed, 20 insertions(+), 48 deletions(-) diff --git a/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java b/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java index a3727e128..05a0873f7 100644 --- a/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java +++ b/src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java @@ -99,12 +99,7 @@ public void addKNNBinaryField(FieldInfo field, DocValuesProducer valuesProducer) KNNEngine knnEngine = model.getModelMetadata().getKnnEngine(); - engineFileName = buildEngineFileName( - state.segmentInfo.name, - knnEngine.getCurrentVersion(), - field.name, - knnEngine.getExtension() - ); + engineFileName = buildEngineFileName(state.segmentInfo.name, knnEngine.getVersion(), field.name, knnEngine.getExtension()); indexPath = Paths.get(((FSDirectory) (FilterDirectory.unwrap(state.directory))).getDirectory().toString(), engineFileName) .toString(); @@ -119,12 +114,7 @@ public void addKNNBinaryField(FieldInfo field, DocValuesProducer valuesProducer) String engineName = field.attributes().getOrDefault(KNNConstants.KNN_ENGINE, KNNEngine.DEFAULT.getName()); KNNEngine knnEngine = KNNEngine.getEngine(engineName); - engineFileName = buildEngineFileName( - state.segmentInfo.name, - knnEngine.getCurrentVersion(), - field.name, - knnEngine.getExtension() - ); + engineFileName = buildEngineFileName(state.segmentInfo.name, knnEngine.getVersion(), field.name, knnEngine.getExtension()); indexPath = Paths.get(((FSDirectory) (FilterDirectory.unwrap(state.directory))).getDirectory().toString(), engineFileName) .toString(); diff --git a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java index 12d1b9b00..d67c15f44 100644 --- a/src/main/java/org/opensearch/knn/index/util/KNNEngine.java +++ b/src/main/java/org/opensearch/knn/index/util/KNNEngine.java @@ -85,8 +85,8 @@ public String getName() { } @Override - public String getCurrentVersion() { - return knnLibrary.getCurrentVersion(); + public String getVersion() { + return knnLibrary.getVersion(); } @Override diff --git a/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java b/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java index 49bd427a8..7cb14f7f0 100644 --- a/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/KNNLibrary.java @@ -24,11 +24,13 @@ public interface KNNLibrary { /** - * Gets the library's current version + * Gets the version of the library that is being used. In general, this can be used for ensuring compatibility of + * serialized artifacts. For instance, this can be used to check if a given file that was created on a different + * cluster is compatible with this instance of the library. * - * @return the string representing the library's latest build version + * @return the string representing the library's version */ - String getCurrentVersion(); + String getVersion(); /** * Gets the extension that files written with this library should have 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 ff1838a4b..2cbfcb12d 100644 --- a/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java +++ b/src/main/java/org/opensearch/knn/index/util/NativeLibrary.java @@ -49,7 +49,7 @@ abstract class NativeLibrary implements KNNLibrary { } @Override - public String getCurrentVersion() { + public String getVersion() { return this.currentVersion; } 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 35feed76e..b86b4051a 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 @@ -178,12 +178,7 @@ public void testAddKNNBinaryField_fromScratch_nmslibCurrent() throws IOException knn80DocValuesConsumer.addKNNBinaryField(fieldInfoArray[0], randomVectorDocValuesProducer); // The document should be created in the correct location - String expectedFile = KNNCodecUtil.buildEngineFileName( - segmentName, - knnEngine.getCurrentVersion(), - fieldName, - knnEngine.getExtension() - ); + String expectedFile = KNNCodecUtil.buildEngineFileName(segmentName, knnEngine.getVersion(), fieldName, knnEngine.getExtension()); assertFileInCorrectLocation(state, expectedFile); // The footer should be valid @@ -227,12 +222,7 @@ public void testAddKNNBinaryField_fromScratch_nmslibLegacy() throws IOException knn80DocValuesConsumer.addKNNBinaryField(fieldInfoArray[0], randomVectorDocValuesProducer); // The document should be created in the correct location - String expectedFile = KNNCodecUtil.buildEngineFileName( - segmentName, - knnEngine.getCurrentVersion(), - fieldName, - knnEngine.getExtension() - ); + String expectedFile = KNNCodecUtil.buildEngineFileName(segmentName, knnEngine.getVersion(), fieldName, knnEngine.getExtension()); assertFileInCorrectLocation(state, expectedFile); // The footer should be valid @@ -283,12 +273,7 @@ public void testAddKNNBinaryField_fromScratch_faissCurrent() throws IOException knn80DocValuesConsumer.addKNNBinaryField(fieldInfoArray[0], randomVectorDocValuesProducer); // The document should be created in the correct location - String expectedFile = KNNCodecUtil.buildEngineFileName( - segmentName, - knnEngine.getCurrentVersion(), - fieldName, - knnEngine.getExtension() - ); + String expectedFile = KNNCodecUtil.buildEngineFileName(segmentName, knnEngine.getVersion(), fieldName, knnEngine.getExtension()); assertFileInCorrectLocation(state, expectedFile); // The footer should be valid @@ -364,12 +349,7 @@ public void testAddKNNBinaryField_fromModel_faiss() throws IOException, Executio knn80DocValuesConsumer.addKNNBinaryField(fieldInfoArray[0], randomVectorDocValuesProducer); // The document should be created in the correct location - String expectedFile = KNNCodecUtil.buildEngineFileName( - segmentName, - knnEngine.getCurrentVersion(), - fieldName, - knnEngine.getExtension() - ); + String expectedFile = KNNCodecUtil.buildEngineFileName(segmentName, knnEngine.getVersion(), fieldName, knnEngine.getExtension()); assertFileInCorrectLocation(state, expectedFile); // The footer should be valid diff --git a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java index 4cb0e5756..59467c36c 100644 --- a/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java +++ b/src/test/java/org/opensearch/knn/index/util/KNNEngineTests.java @@ -10,10 +10,10 @@ public class KNNEngineTests extends KNNTestCase { /** - * Get latest build version from library + * Check that version from engine and library match */ public void testDelegateLibraryFunctions() { - assertEquals(Nmslib.INSTANCE.getCurrentVersion(), KNNEngine.NMSLIB.getCurrentVersion()); + assertEquals(Nmslib.INSTANCE.getVersion(), KNNEngine.NMSLIB.getVersion()); } /** diff --git a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java index 7cb6bc0a3..bac3534d8 100644 --- a/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java +++ b/src/test/java/org/opensearch/knn/index/util/NativeLibraryTests.java @@ -27,12 +27,12 @@ public class NativeLibraryTests extends KNNTestCase { /** - * Test native library build version getter + * Test native library version getter */ - public void testGetCurrentVersion() { + public void testGetVersion() { String latestBuildVersion = "test-build-version"; TestNativeLibrary testNativeLibrary = new TestNativeLibrary(Collections.emptyMap(), Collections.emptyMap(), latestBuildVersion, ""); - assertEquals(latestBuildVersion, testNativeLibrary.getCurrentVersion()); + assertEquals(latestBuildVersion, testNativeLibrary.getVersion()); } /** diff --git a/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java b/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java index 6154fcb5d..64cf86381 100644 --- a/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java +++ b/src/test/java/org/opensearch/knn/plugin/stats/suppliers/LibraryInitializedSupplierTests.java @@ -39,7 +39,7 @@ private static class TestLibrary implements KNNLibrary { } @Override - public String getCurrentVersion() { + public String getVersion() { return null; }