Address review feedback: reject empty vector / option segments, stabilize key order

- parseVector now uses split(",", -1) and rejects empty components so
  malformed literals like "[1.0,]" or "[1.0,,2.0]" surface an explicit
  error instead of silently shrinking the vector to a lower dimension.
- parseOptions now rejects empty option segments with an explicit
  "Malformed option segment" error; a wholly empty option string
  continues to flow to the "Missing required option" gate for the
  existing, clearer message.
- ALLOWED_OPTION_KEYS is now a List so the unknown-key error renders
  the supported keys in a stable, user-friendly order
  (k, max_distance, min_score, filter_type) rather than Set iteration
  order.

Adds unit coverage for trailing, leading, and consecutive empty
segments in both vector and option parsing, and for stable key order
in the unknown-option-key error message. Adds an IT for the trailing
comma vector case.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>

Author: mengweieric

integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java

@@ -622,6 +622,20 @@ public void testNegativeMaxDistanceRejected() throws IOException {
     assertThat(ex.getMessage(), containsString("non-negative"));
   }
 
+  @Test
+  public void testTrailingCommaInVectorRejected() throws IOException {
+    ResponseException ex =
+        expectThrows(
+            ResponseException.class,
+            () ->
+                executeQuery(
+                    "SELECT v._id FROM vectorSearch(table='t', field='f', "
+                        + "vector='[1.0,2.0,]', option='k=5') AS v"));
+
+    assertThat(ex.getMessage(), containsString("Invalid vector component"));
+    assertThat(ex.getMessage(), containsString("trailing or consecutive commas"));
+  }
+
   private void deleteIndexIfExists(String indexName) {
     try {
       client().performRequest(new Request("DELETE", "/" + indexName));

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionImplementation.java

@@ -14,7 +14,6 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import org.opensearch.sql.common.setting.Settings;
@@ -35,9 +34,13 @@
 public class VectorSearchTableFunctionImplementation extends FunctionExpression
     implements TableFunctionImplementation {
 
-  /** P0 allowed option keys. Rejects unknown/future keys to prevent unvalidated DSL injection. */
-  static final Set<String> ALLOWED_OPTION_KEYS =
-      Set.of("k", "max_distance", "min_score", "filter_type");
+  /**
+   * P0 allowed option keys. Rejects unknown/future keys to prevent unvalidated DSL injection. A
+   * {@link List} (rather than a {@link Set}) so the unknown-key error message renders the supported
+   * keys in a stable, user-friendly order.
+   */
+  static final List<String> ALLOWED_OPTION_KEYS =
+      List.of("k", "max_distance", "min_score", "filter_type");
 
   /**
    * Field names must be safe for JSON interpolation: alphanumeric, dots (nested), underscores,
@@ -135,30 +138,49 @@ private float[] parseVector(String vectorLiteral) {
                   + " e.g., vector='[1.0,2.0,3.0]'",
               vectorLiteral));
     }
-    String[] parts = cleaned.split(",");
+    // Preserve trailing empties (split(",", -1)) so malformed literals like "[1.0,]" or
+    // "[1.0,,2.0]" surface an explicit error instead of silently shrinking the vector.
+    String[] parts = cleaned.split(",", -1);
     float[] vector = new float[parts.length];
     for (int i = 0; i < parts.length; i++) {
+      String component = parts[i].trim();
+      if (component.isEmpty()) {
+        throw new ExpressionEvaluationException(
+            String.format(
+                "Invalid vector component at position %d: must be a number (check for"
+                    + " trailing or consecutive commas in '%s')",
+                i, vectorLiteral));
+      }
       try {
-        vector[i] = Float.parseFloat(parts[i].trim());
+        vector[i] = Float.parseFloat(component);
       } catch (NumberFormatException e) {
         throw new ExpressionEvaluationException(
-            String.format("Invalid vector component '%s': must be a number", parts[i].trim()));
+            String.format("Invalid vector component '%s': must be a number", component));
       }
       if (!Float.isFinite(vector[i])) {
         throw new ExpressionEvaluationException(
-            String.format(
-                "Invalid vector component '%s': must be a finite number", parts[i].trim()));
+            String.format("Invalid vector component '%s': must be a finite number", component));
       }
     }
     return vector;
   }
 
   static Map<String, String> parseOptions(String optionStr) {
     Map<String, String> options = new LinkedHashMap<>();
-    for (String pair : optionStr.split(",")) {
+    // A wholly empty option string is handled downstream with a clearer "missing required option"
+    // message than a generic malformed-segment error.
+    if (optionStr.trim().isEmpty()) {
+      return options;
+    }
+    // split(",", -1) preserves trailing empties so malformed inputs like "k=5," or "k=5,,k2=v"
+    // surface an explicit error instead of being silently dropped.
+    String[] pairs = optionStr.split(",", -1);
+    for (String pair : pairs) {
       String trimmed = pair.trim();
       if (trimmed.isEmpty()) {
-        continue;
+        throw new ExpressionEvaluationException(
+            "Malformed option segment '': expected key=value (check for trailing or"
+                + " consecutive commas)");
       }
       String[] kv = trimmed.split("=", 2);
       if (kv.length != 2 || kv[0].trim().isEmpty() || kv[1].trim().isEmpty()) {

opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionImplementationTest.java

@@ -634,6 +634,83 @@ void applyArguments_acceptsZeroMaxDistance() {
     assertTrue(table instanceof VectorSearchIndex);
   }
 
+  // ── Vector parsing: trailing / empty components (PR #5381 review) ─────
+
+  @Test
+  void applyArguments_rejectsTrailingCommaInVector() {
+    VectorSearchTableFunctionImplementation impl =
+        createImplWithArgs("my-index", "embedding", "[1.0,2.0,]", "k=5");
+    ExpressionEvaluationException ex =
+        assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
+    assertTrue(ex.getMessage().contains("Invalid vector component"));
+    assertTrue(ex.getMessage().contains("trailing or consecutive commas"));
+  }
+
+  @Test
+  void applyArguments_rejectsConsecutiveCommasInVector() {
+    VectorSearchTableFunctionImplementation impl =
+        createImplWithArgs("my-index", "embedding", "[1.0,,2.0]", "k=5");
+    ExpressionEvaluationException ex =
+        assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
+    assertTrue(ex.getMessage().contains("Invalid vector component"));
+    assertTrue(ex.getMessage().contains("trailing or consecutive commas"));
+  }
+
+  @Test
+  void applyArguments_rejectsLeadingCommaInVector() {
+    VectorSearchTableFunctionImplementation impl =
+        createImplWithArgs("my-index", "embedding", "[,1.0,2.0]", "k=5");
+    ExpressionEvaluationException ex =
+        assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
+    assertTrue(ex.getMessage().contains("Invalid vector component"));
+  }
+
+  // ── Option parsing: empty segments (PR #5381 review) ─────────────────
+
+  @Test
+  void parseOptions_rejectsTrailingEmptySegment() {
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> VectorSearchTableFunctionImplementation.parseOptions("k=5,"));
+    assertTrue(ex.getMessage().contains("Malformed option segment"));
+    assertTrue(ex.getMessage().contains("trailing or consecutive commas"));
+  }
+
+  @Test
+  void parseOptions_rejectsLeadingEmptySegment() {
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> VectorSearchTableFunctionImplementation.parseOptions(",k=5"));
+    assertTrue(ex.getMessage().contains("Malformed option segment"));
+  }
+
+  @Test
+  void parseOptions_rejectsConsecutiveCommas() {
+    ExpressionEvaluationException ex =
+        assertThrows(
+            ExpressionEvaluationException.class,
+            () -> VectorSearchTableFunctionImplementation.parseOptions("k=5,,filter_type=post"));
+    assertTrue(ex.getMessage().contains("Malformed option segment"));
+  }
+
+  // ── Unknown-key error lists supported keys in stable order (PR #5381 review) ──
+
+  @Test
+  void applyArguments_unknownOptionKeyErrorListsSupportedKeysInStableOrder() {
+    VectorSearchTableFunctionImplementation impl =
+        createImplWithArgs("my-index", "embedding", "[1.0, 2.0]", "k=5,bogus=1");
+    ExpressionEvaluationException ex =
+        assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
+    String msg = ex.getMessage();
+    int kIdx = msg.indexOf("k");
+    int maxIdx = msg.indexOf("max_distance");
+    int minIdx = msg.indexOf("min_score");
+    int filterIdx = msg.indexOf("filter_type");
+    assertTrue(kIdx >= 0 && maxIdx > kIdx && minIdx > maxIdx && filterIdx > minIdx);
+  }
+
   private VectorSearchTableFunctionImplementation createImpl() {
     return createImplWithArgs("my-index", "embedding", "[1.0, 2.0, 3.0]", "k=5");
   }