apply review comments

Signed-off-by: panguixin <[email protected]>

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

@@ -0,0 +1,113 @@
+/*
+ * 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.common;
+
+import java.util.Locale;
+import lombok.AccessLevel;
+import lombok.NoArgsConstructor;
+import org.opensearch.knn.index.SpaceType;
+import org.opensearch.knn.index.VectorDataType;
+
+import static org.opensearch.knn.common.KNNConstants.VECTOR_DATA_TYPE_FIELD;
+import static org.opensearch.knn.common.KNNVectorUtil.isZeroVector;
+
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+public class KNNValidationUtil {
+    /**
+     * Validate the float vector value and throw exception if it is not a number or not in the finite range.
+     *
+     * @param value  float vector value
+     */
+    public static void validateFloatVectorValue(float value) {
+        if (Float.isNaN(value)) {
+            throw new IllegalArgumentException("KNN vector values cannot be NaN");
+        }
+
+        if (Float.isInfinite(value)) {
+            throw new IllegalArgumentException("KNN vector values cannot be infinity");
+        }
+    }
+
+    /**
+     * Validate the float vector value in the byte range if it is a finite number,
+     * with no decimal values and in the byte range of [-128 to 127]. If not throw IllegalArgumentException.
+     *
+     * @param value  float value in byte range
+     */
+    public static void validateByteVectorValue(float value) {
+        validateFloatVectorValue(value);
+        if (value % 1 != 0) {
+            throw new IllegalArgumentException(
+                String.format(
+                    Locale.ROOT,
+                    "[%s] field was set as [%s] in index mapping. But, KNN vector values are floats instead of byte integers",
+                    VECTOR_DATA_TYPE_FIELD,
+                    VectorDataType.BYTE.getValue()
+                )
+
+            );
+        }
+        if ((int) value < Byte.MIN_VALUE || (int) value > Byte.MAX_VALUE) {
+            throw new IllegalArgumentException(
+                String.format(
+                    Locale.ROOT,
+                    "[%s] field was set as [%s] in index mapping. But, KNN vector values are not within in the byte range [%d, %d]",
+                    VECTOR_DATA_TYPE_FIELD,
+                    VectorDataType.BYTE.getValue(),
+                    Byte.MIN_VALUE,
+                    Byte.MAX_VALUE
+                )
+            );
+        }
+    }
+
+    /**
+     * Validate if the given byte vector is supported by the given space type
+     *
+     * @param vector     the given vector
+     * @param spaceType  the given space type
+     */
+    public static void validateByteVector(byte[] vector, SpaceType spaceType) {
+        if (spaceType == SpaceType.COSINESIMIL && isZeroVector(vector)) {
+            throw new IllegalArgumentException(
+                String.format(Locale.ROOT, "zero vector is not supported when space type is [%s]", spaceType.getValue())
+            );
+        }
+    }
+
+    /**
+     * Validate if the given float vector is supported by the given space type
+     *
+     * @param vector     the given vector
+     * @param spaceType  the given space type
+     */
+    public static void validateFloatVector(float[] vector, SpaceType spaceType) {
+        if (spaceType == SpaceType.COSINESIMIL && isZeroVector(vector)) {
+            throw new IllegalArgumentException(
+                String.format(Locale.ROOT, "zero vector is not supported when space type is [%s]", spaceType.getValue())
+            );
+        }
+    }
+
+    /**
+     * Validate if the given vector size matches with the dimension provided in mapping.
+     *
+     * @param dimension dimension of vector
+     * @param vectorSize size of the vector
+     */
+    public static void validateVectorDimension(int dimension, int vectorSize) {
+        if (dimension != vectorSize) {
+            String errorMessage = String.format(Locale.ROOT, "Vector dimension mismatch. Expected: %d, Given: %d", dimension, vectorSize);
+            throw new IllegalArgumentException(errorMessage);
+        }
+    }
+}

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

@@ -5,16 +5,20 @@
 
 package org.opensearch.knn.common;
 
-public class KNNVectorUtil {
-    private KNNVectorUtil() {}
+import java.util.Objects;
+import lombok.AccessLevel;
+import lombok.NoArgsConstructor;
 
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+public class KNNVectorUtil {
     /**
      * Check if all the elements of a given vector are zero
      *
      * @param vector the vector
      * @return true if yes; otherwise false
      */
     public static boolean isZeroVector(byte[] vector) {
+        Objects.requireNonNull(vector, "vector must not be null");
         for (byte e : vector) {
             if (e != 0) {
                 return false;
@@ -30,6 +34,7 @@ public static boolean isZeroVector(byte[] vector) {
      * @return true if yes; otherwise false
      */
     public static boolean isZeroVector(float[] vector) {
+        Objects.requireNonNull(vector, "vector must not be null");
         for (float e : vector) {
             if (e != 0f) {
                 return false;

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java

@@ -57,14 +57,14 @@
 import static org.opensearch.knn.common.KNNConstants.KNN_METHOD;
 import static org.opensearch.knn.common.KNNConstants.VECTOR_DATA_TYPE_FIELD;
 import static org.opensearch.knn.index.KNNSettings.KNN_INDEX;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateByteVector;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateFloatVector;
 import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateVectorDataTypeWithEngine;
 import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateVectorDataTypeWithKnnIndexSetting;
 import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.addStoredFieldForVectorField;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateByteVectorValue;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateFloatVectorValue;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateVectorDimension;
+import static org.opensearch.knn.common.KNNValidationUtil.validateByteVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateByteVectorValue;
+import static org.opensearch.knn.common.KNNValidationUtil.validateFloatVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateFloatVectorValue;
+import static org.opensearch.knn.common.KNNValidationUtil.validateVectorDimension;
 
 /**
  * Field Mapper for KNN vector type.

src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java

@@ -11,12 +11,13 @@
 
 package org.opensearch.knn.index.mapper;
 
+import lombok.AccessLevel;
+import lombok.NoArgsConstructor;
 import org.apache.lucene.document.FieldType;
 import org.apache.lucene.document.StoredField;
 import org.apache.lucene.index.DocValuesType;
 import org.opensearch.index.mapper.ParametrizedFieldMapper;
 import org.opensearch.index.mapper.ParseContext;
-import org.opensearch.knn.index.SpaceType;
 import org.opensearch.knn.index.VectorDataType;
 import org.opensearch.knn.index.util.KNNEngine;
 
@@ -25,98 +26,9 @@
 import static org.opensearch.knn.common.KNNConstants.KNN_ENGINE;
 import static org.opensearch.knn.common.KNNConstants.LUCENE_NAME;
 import static org.opensearch.knn.common.KNNConstants.VECTOR_DATA_TYPE_FIELD;
-import static org.opensearch.knn.common.KNNVectorUtil.isZeroVector;
 
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
 public class KNNVectorFieldMapperUtil {
-    /**
-     * Validate the float vector value and throw exception if it is not a number or not in the finite range.
-     *
-     * @param value  float vector value
-     */
-    public static void validateFloatVectorValue(float value) {
-        if (Float.isNaN(value)) {
-            throw new IllegalArgumentException("KNN vector values cannot be NaN");
-        }
-
-        if (Float.isInfinite(value)) {
-            throw new IllegalArgumentException("KNN vector values cannot be infinity");
-        }
-    }
-
-    /**
-     * Validate the float vector value in the byte range if it is a finite number,
-     * with no decimal values and in the byte range of [-128 to 127]. If not throw IllegalArgumentException.
-     *
-     * @param value  float value in byte range
-     */
-    public static void validateByteVectorValue(float value) {
-        validateFloatVectorValue(value);
-        if (value % 1 != 0) {
-            throw new IllegalArgumentException(
-                String.format(
-                    Locale.ROOT,
-                    "[%s] field was set as [%s] in index mapping. But, KNN vector values are floats instead of byte integers",
-                    VECTOR_DATA_TYPE_FIELD,
-                    VectorDataType.BYTE.getValue()
-                )
-
-            );
-        }
-        if ((int) value < Byte.MIN_VALUE || (int) value > Byte.MAX_VALUE) {
-            throw new IllegalArgumentException(
-                String.format(
-                    Locale.ROOT,
-                    "[%s] field was set as [%s] in index mapping. But, KNN vector values are not within in the byte range [%d, %d]",
-                    VECTOR_DATA_TYPE_FIELD,
-                    VectorDataType.BYTE.getValue(),
-                    Byte.MIN_VALUE,
-                    Byte.MAX_VALUE
-                )
-            );
-        }
-    }
-
-    /**
-     * Validate if the given byte vector is supported by the given space type
-     *
-     * @param vector     the given vector
-     * @param spaceType  the given space type
-     */
-    public static void validateByteVector(byte[] vector, SpaceType spaceType) {
-        if (spaceType == SpaceType.COSINESIMIL && isZeroVector(vector)) {
-            throw new IllegalArgumentException(
-                String.format(Locale.ROOT, "zero vector is not supported when space type is [%s]", spaceType.getValue())
-            );
-        }
-    }
-
-    /**
-     * Validate if the given float vector is supported by the given space type
-     *
-     * @param vector     the given vector
-     * @param spaceType  the given space type
-     */
-    public static void validateFloatVector(float[] vector, SpaceType spaceType) {
-        if (spaceType == SpaceType.COSINESIMIL && isZeroVector(vector)) {
-            throw new IllegalArgumentException(
-                String.format(Locale.ROOT, "zero vector is not supported when space type is [%s]", spaceType.getValue())
-            );
-        }
-    }
-
-    /**
-     * Validate if the given vector size matches with the dimension provided in mapping.
-     *
-     * @param dimension dimension of vector
-     * @param vectorSize size of the vector
-     */
-    public static void validateVectorDimension(int dimension, int vectorSize) {
-        if (dimension != vectorSize) {
-            String errorMessage = String.format(Locale.ROOT, "Vector dimension mismatch. Expected: %d, Given: %d", dimension, vectorSize);
-            throw new IllegalArgumentException(errorMessage);
-        }
-    }
-
     /**
      * Validates and throws exception if data_type field is set in the index mapping
      * using any VectorDataType (other than float, which is default) because other

src/main/java/org/opensearch/knn/index/mapper/LuceneFieldMapper.java

@@ -27,8 +27,8 @@
 import static org.opensearch.knn.common.KNNConstants.VECTOR_DATA_TYPE_FIELD;
 import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.addStoredFieldForVectorField;
 import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.buildDocValuesFieldType;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateByteVector;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateFloatVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateByteVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateFloatVector;
 
 /**
  * Field mapper for case when Lucene has been set as an engine.

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

@@ -35,9 +35,9 @@
 import java.util.Objects;
 
 import static org.opensearch.knn.index.IndexUtil.isClusterOnOrAfterMinRequiredVersion;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateByteVector;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateByteVectorValue;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateFloatVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateByteVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateByteVectorValue;
+import static org.opensearch.knn.common.KNNValidationUtil.validateFloatVector;
 
 /**
  * Helper class to build the KNN query
@@ -290,8 +290,10 @@ protected Query doToQuery(QueryShardContext context) {
         SpaceType spaceType = knnVectorFieldType.getSpaceType();
 
         if (fieldDimension == -1) {
+            if (spaceType != null) {
+                throw new IllegalStateException("Space type should be null when the field uses a model");
+            }
             // If dimension is not set, the field uses a model and the information needs to be retrieved from there
-            assert spaceType == null;
             ModelMetadata modelMetadata = getModelMetadataForField(knnVectorFieldType);
             fieldDimension = modelMetadata.getDimension();
             knnEngine = modelMetadata.getKnnEngine();

src/main/java/org/opensearch/knn/plugin/script/KNNScoringSpace.java

@@ -19,7 +19,7 @@
 import java.util.Map;
 import java.util.function.BiFunction;
 
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateFloatVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateFloatVector;
 import static org.opensearch.knn.plugin.script.KNNScoringSpaceUtil.getVectorMagnitudeSquared;
 import static org.opensearch.knn.plugin.script.KNNScoringSpaceUtil.isBinaryFieldType;
 import static org.opensearch.knn.plugin.script.KNNScoringSpaceUtil.isKNNVectorFieldType;

src/main/java/org/opensearch/knn/plugin/script/KNNScoringSpaceUtil.java

@@ -17,7 +17,7 @@
 import java.util.Base64;
 
 import static org.opensearch.index.mapper.NumberFieldMapper.NumberType.LONG;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateByteVectorValue;
+import static org.opensearch.knn.common.KNNValidationUtil.validateByteVectorValue;
 
 public class KNNScoringSpaceUtil {
 

src/main/java/org/opensearch/knn/plugin/script/KNNScoringUtil.java

@@ -15,8 +15,8 @@
 import java.util.List;
 import java.util.Objects;
 
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateByteVectorValue;
-import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateFloatVector;
+import static org.opensearch.knn.common.KNNValidationUtil.validateByteVectorValue;
+import static org.opensearch.knn.common.KNNValidationUtil.validateFloatVector;
 
 public class KNNScoringUtil {
     private static Logger logger = LogManager.getLogger(KNNScoringUtil.class);