Skip to content

Commit d70b27b

Browse files
mengweiericmengweieric
committed
[BugFix] Tighten vectorSearch() option and vector parsing error messages
Polishes validation behavior for vectorSearch() so malformed options and vector literals surface a clear, actionable error instead of a generic one: - Reject empty option values and mid-segment empty values with a "expected key=value" message that names the offending segment. - Trim whitespace around option keys and values so natural spacing (e.g., "k = 5") works. - Reject unknown option keys with a message listing the supported set. - Detect the common non-comma separators (';', ':', '|') in vector literals and surface a dedicated message pointing to the correct comma-separated form, rather than a generic "Invalid vector component". - Reject negative max_distance and min_score values with a clear non-negative requirement, while continuing to accept zero. Adds unit coverage in VectorSearchTableFunctionImplementationTest and integration coverage in VectorSearchIT for the new error paths. Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 01a5d87 commit d70b27b

3 files changed

Lines changed: 179 additions & 0 deletions

File tree

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -580,6 +580,48 @@ public void testVectorSearchAgainstIndexWithScoreFieldRejects() throws IOExcepti
580580
assertThat(ex.getMessage(), containsString("collides"));
581581
}
582582

583+
@Test
584+
public void testSemicolonSeparatorInVectorRejected() throws IOException {
585+
ResponseException ex =
586+
expectThrows(
587+
ResponseException.class,
588+
() ->
589+
executeQuery(
590+
"SELECT v._id FROM vectorSearch(table='t', field='f', "
591+
+ "vector='[1.0;2.0]', option='k=5') AS v"));
592+
593+
assertThat(ex.getMessage(), containsString("vector="));
594+
assertThat(ex.getMessage(), containsString("comma-separated"));
595+
}
596+
597+
@Test
598+
public void testNegativeMinScoreRejected() throws IOException {
599+
ResponseException ex =
600+
expectThrows(
601+
ResponseException.class,
602+
() ->
603+
executeQuery(
604+
"SELECT v._id FROM vectorSearch(table='t', field='f', "
605+
+ "vector='[1.0]', option='min_score=-0.5') AS v"));
606+
607+
assertThat(ex.getMessage(), containsString("min_score"));
608+
assertThat(ex.getMessage(), containsString("non-negative"));
609+
}
610+
611+
@Test
612+
public void testNegativeMaxDistanceRejected() throws IOException {
613+
ResponseException ex =
614+
expectThrows(
615+
ResponseException.class,
616+
() ->
617+
executeQuery(
618+
"SELECT v._id FROM vectorSearch(table='t', field='f', "
619+
+ "vector='[1.0]', option='max_distance=-1.0') AS v"));
620+
621+
assertThat(ex.getMessage(), containsString("max_distance"));
622+
assertThat(ex.getMessage(), containsString("non-negative"));
623+
}
624+
583625
private void deleteIndexIfExists(String indexName) {
584626
try {
585627
client().performRequest(new Request("DELETE", "/" + indexName));

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,15 @@ private float[] parseVector(String vectorLiteral) {
126126
if (cleaned.isEmpty()) {
127127
throw new ExpressionEvaluationException("Vector literal must not be empty");
128128
}
129+
// Reject common non-comma separators before Float.parseFloat fails with a generic
130+
// "Invalid vector component" that doesn't hint the user at the separator.
131+
if (cleaned.indexOf(';') >= 0 || cleaned.indexOf(':') >= 0 || cleaned.indexOf('|') >= 0) {
132+
throw new ExpressionEvaluationException(
133+
String.format(
134+
"Invalid vector literal '%s': vector= requires comma-separated components,"
135+
+ " e.g., vector='[1.0,2.0,3.0]'",
136+
vectorLiteral));
137+
}
129138
String[] parts = cleaned.split(",");
130139
float[] vector = new float[parts.length];
131140
for (int i = 0; i < parts.length; i++) {
@@ -274,9 +283,20 @@ private void validateOptions(Map<String, String> options) {
274283
}
275284
if (hasMaxDistance) {
276285
parseDoubleOption(options, "max_distance");
286+
double maxDistance = Double.parseDouble(options.get("max_distance"));
287+
if (maxDistance < 0) {
288+
throw new ExpressionEvaluationException(
289+
String.format(
290+
"max_distance must be non-negative, got %s", options.get("max_distance")));
291+
}
277292
}
278293
if (hasMinScore) {
279294
parseDoubleOption(options, "min_score");
295+
double minScore = Double.parseDouble(options.get("min_score"));
296+
if (minScore < 0) {
297+
throw new ExpressionEvaluationException(
298+
String.format("min_score must be non-negative, got %s", options.get("min_score")));
299+
}
280300
}
281301
}
282302

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

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,123 @@ void validateNamedArgs_rejectsDuplicateNames() {
517517
assertTrue(ex.getMessage().contains("table"));
518518
}
519519

520+
// ── Option parsing: empty value, whitespace, unknown keys ────────────
521+
522+
@Test
523+
void parseOptions_rejectsEmptyValue() {
524+
ExpressionEvaluationException ex =
525+
assertThrows(
526+
ExpressionEvaluationException.class,
527+
() -> VectorSearchTableFunctionImplementation.parseOptions("k="));
528+
assertTrue(ex.getMessage().contains("Malformed option segment"));
529+
}
530+
531+
@Test
532+
void parseOptions_rejectsEmptyValueInMidSegment() {
533+
ExpressionEvaluationException ex =
534+
assertThrows(
535+
ExpressionEvaluationException.class,
536+
() -> VectorSearchTableFunctionImplementation.parseOptions("k=,filter_type=post"));
537+
assertTrue(ex.getMessage().contains("Malformed option segment"));
538+
}
539+
540+
@Test
541+
void parseOptions_trimsWhitespaceAroundKeyAndValue() {
542+
Map<String, String> options =
543+
VectorSearchTableFunctionImplementation.parseOptions(" k = 5 , filter_type = post ");
544+
assertEquals("5", options.get("k"));
545+
assertEquals("post", options.get("filter_type"));
546+
}
547+
548+
@Test
549+
void applyArguments_rejectsUnknownOptionKey() {
550+
VectorSearchTableFunctionImplementation impl =
551+
createImplWithArgs(
552+
"my-index", "embedding", "[1.0, 2.0]", "k=5,method_parameters.ef_search=100");
553+
ExpressionEvaluationException ex =
554+
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
555+
assertTrue(ex.getMessage().contains("Unknown option key"));
556+
assertTrue(ex.getMessage().contains("method_parameters.ef_search"));
557+
}
558+
559+
// ── Vector parsing: non-comma separator ─────────────────────────────
560+
561+
@Test
562+
void applyArguments_rejectsSemicolonSeparatorInVector() {
563+
VectorSearchTableFunctionImplementation impl =
564+
createImplWithArgs("my-index", "embedding", "[1.0;2.0]", "k=5");
565+
ExpressionEvaluationException ex =
566+
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
567+
assertTrue(ex.getMessage().contains("vector="));
568+
assertTrue(ex.getMessage().contains("comma-separated"));
569+
}
570+
571+
@Test
572+
void applyArguments_rejectsColonSeparatorInVector() {
573+
VectorSearchTableFunctionImplementation impl =
574+
createImplWithArgs("my-index", "embedding", "[1.0:2.0]", "k=5");
575+
ExpressionEvaluationException ex =
576+
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
577+
assertTrue(ex.getMessage().contains("vector="));
578+
}
579+
580+
@Test
581+
void applyArguments_rejectsPipeSeparatorInVector() {
582+
VectorSearchTableFunctionImplementation impl =
583+
createImplWithArgs("my-index", "embedding", "[1.0|2.0]", "k=5");
584+
ExpressionEvaluationException ex =
585+
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
586+
assertTrue(ex.getMessage().contains("vector="));
587+
}
588+
589+
// ── Option bounds: negative k, min_score, max_distance ──────────────
590+
591+
@Test
592+
void applyArguments_negativeKMessageCitesRange() {
593+
VectorSearchTableFunctionImplementation impl =
594+
createImplWithArgs("my-index", "embedding", "[1.0, 2.0]", "k=-3");
595+
ExpressionEvaluationException ex =
596+
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
597+
assertTrue(ex.getMessage().contains("1"));
598+
assertTrue(ex.getMessage().contains("10000"));
599+
}
600+
601+
@Test
602+
void applyArguments_rejectsNegativeMinScore() {
603+
VectorSearchTableFunctionImplementation impl =
604+
createImplWithArgs("my-index", "embedding", "[1.0, 2.0]", "min_score=-0.5");
605+
ExpressionEvaluationException ex =
606+
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
607+
assertTrue(ex.getMessage().contains("min_score"));
608+
assertTrue(ex.getMessage().contains("non-negative"));
609+
}
610+
611+
@Test
612+
void applyArguments_rejectsNegativeMaxDistance() {
613+
VectorSearchTableFunctionImplementation impl =
614+
createImplWithArgs("my-index", "embedding", "[1.0, 2.0]", "max_distance=-1.0");
615+
ExpressionEvaluationException ex =
616+
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
617+
assertTrue(ex.getMessage().contains("max_distance"));
618+
assertTrue(ex.getMessage().contains("non-negative"));
619+
}
620+
621+
@Test
622+
void applyArguments_acceptsZeroMinScore() {
623+
VectorSearchTableFunctionImplementation impl =
624+
createImplWithArgs("my-index", "embedding", "[1.0, 2.0]", "min_score=0");
625+
Table table = impl.applyArguments();
626+
assertTrue(table instanceof VectorSearchIndex);
627+
}
628+
629+
@Test
630+
void applyArguments_acceptsZeroMaxDistance() {
631+
VectorSearchTableFunctionImplementation impl =
632+
createImplWithArgs("my-index", "embedding", "[1.0, 2.0]", "max_distance=0");
633+
Table table = impl.applyArguments();
634+
assertTrue(table instanceof VectorSearchIndex);
635+
}
636+
520637
private VectorSearchTableFunctionImplementation createImpl() {
521638
return createImplWithArgs("my-index", "embedding", "[1.0, 2.0, 3.0]", "k=5");
522639
}

0 commit comments

Comments
 (0)