Resolve PR feedback

Signed-off-by: Junqiu Lei <[email protected]>
junqiu-lei · Jan 9, 2024 · cf37f59 · cf37f59
1 parent 046d929
commit cf37f59
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 50 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,7 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 * Increase Lucene max dimension limit to 16,000 [#1346](https://github.com/opensearch-project/k-NN/pull/1346)
 * Tuned default values for ef_search and ef_construction for better indexing and search performance for vector search [#1353](https://github.com/opensearch-project/k-NN/pull/1353)
 * Enabled Filtering on Nested Vector fields with top level filters [#1372](https://github.com/opensearch-project/k-NN/pull/1372)
-* * Throw proper exception to invalid k-NN query [#1380](https://github.com/opensearch-project/k-NN/pull/1380)
+* Throw proper exception to invalid k-NN query [#1380](https://github.com/opensearch-project/k-NN/pull/1380)
 ### Bug Fixes
 * Fix use-after-free case on nmslib search path [#1305](https://github.com/opensearch-project/k-NN/pull/1305)
 * Allow nested knn field mapping when train model [#1318](https://github.com/opensearch-project/k-NN/pull/1318)

diff --git a/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java b/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java
@@ -101,12 +101,12 @@ public static void initialize(ModelDao modelDao) {
 
     private static float[] ObjectsToFloats(List<Object> objs) {
         if (Objects.isNull(objs)) {
-            throw new IllegalArgumentException("[" + NAME + "] requires 'vector' to be non-null");
+            throw new IllegalArgumentException(String.format("[%s] requires 'vector' to be non-null", NAME));
         }
         float[] vec = new float[objs.size()];
         for (int i = 0; i < objs.size(); i++) {
             if (!(objs.get(i) instanceof Number)) {
-                throw new IllegalArgumentException("[" + NAME + "] requires 'vector' to be an array of numbers");
+                throw new IllegalArgumentException(String.format("[%s] requires 'vector' to be an array of numbers", NAME));
             }
             vec[i] = ((Number) objs.get(i)).floatValue();
         }

diff --git a/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java b/src/test/java/org/opensearch/knn/index/VectorDataTypeIT.java
@@ -425,51 +425,6 @@ public void testKNNScriptScoreWithInvalidByteQueryVector() throws Exception {
         );
     }
 
-    @SneakyThrows
-    public void testSearchWithInvalidSearchVectorType() {
-        createKnnIndexMappingWithLuceneEngine(2, SpaceType.L2, VectorDataType.FLOAT.getValue());
-        ingestL2FloatTestData();
-        Request request = new Request("POST", String.format("/%s/_search", INDEX_NAME));
-        request.setJsonEntity(
-            "{\n"
-                + "  \"query\": {\n"
-                + "    \"knn\": {\n"
-                + "      \"test-field-vec-dt\": {\n"
-                + "        \"vector\": [\"a\", \"b\", \"c\", \"d\"],\n"
-                + "        \"k\": 4\n"
-                + "      }\n"
-                + "    }\n"
-                + "  }\n"
-                + "}"
-        );
-
-        ResponseException ex = expectThrows(ResponseException.class, () -> client().performRequest(request));
-        assertTrue(ex.getResponse().getStatusLine().getReasonPhrase().contains("Bad Request"));
-        assertTrue(ex.getMessage().contains(String.format(Locale.ROOT, "[knn] requires 'vector' to be an array of numbers")));
-    }
-
-    @SneakyThrows
-    public void testSearchWithMissingQueryVector() {
-        createKnnIndexMappingWithLuceneEngine(2, SpaceType.L2, VectorDataType.FLOAT.getValue());
-        ingestL2FloatTestData();
-        Request request = new Request("POST", String.format("/%s/_search", INDEX_NAME));
-        request.setJsonEntity(
-            "{\n"
-                + "  \"query\": {\n"
-                + "    \"knn\": {\n"
-                + "      \"test-field-vec-dt\": {\n"
-                + "        \"k\": 4\n"
-                + "      }\n"
-                + "    }\n"
-                + "  }\n"
-                + "}"
-        );
-
-        ResponseException ex = expectThrows(ResponseException.class, () -> client().performRequest(request));
-        assertTrue(ex.getResponse().getStatusLine().getReasonPhrase().contains("Bad Request"));
-        assertTrue(ex.getMessage().contains(String.format(Locale.ROOT, "[knn] requires 'vector' to be non-null")));
-    }
-
     @SneakyThrows
     private void ingestL2ByteTestData() {
         Byte[] b1 = { 6, 6 };

diff --git a/src/test/java/org/opensearch/knn/index/query/KNNQueryBuilderTests.java b/src/test/java/org/opensearch/knn/index/query/KNNQueryBuilderTests.java
@@ -144,7 +144,11 @@ public void testFromXContent_invalidQueryVectorType() throws Exception {
         builder.endObject();
         XContentParser contentParser = createParser(builder);
         contentParser.nextToken();
-        expectThrows(IllegalArgumentException.class, () -> KNNQueryBuilder.fromXContent(contentParser));
+        IllegalArgumentException exception = expectThrows(
+            IllegalArgumentException.class,
+            () -> KNNQueryBuilder.fromXContent(contentParser)
+        );
+        assertTrue(exception.getMessage().contains("[knn] requires 'vector' to be an array of numbers"));
     }
 
     public void testFromXContent_missingQueryVector() throws Exception {
@@ -161,7 +165,11 @@ public void testFromXContent_missingQueryVector() throws Exception {
         builder.endObject();
         XContentParser contentParser = createParser(builder);
         contentParser.nextToken();
-        expectThrows(IllegalArgumentException.class, () -> KNNQueryBuilder.fromXContent(contentParser));
+        IllegalArgumentException exception = expectThrows(
+            IllegalArgumentException.class,
+            () -> KNNQueryBuilder.fromXContent(contentParser)
+        );
+        assertTrue(exception.getMessage().contains("[knn] requires 'vector' to be non-null"));
     }
 
     @Override