Skip to content

Commit c0faf7c

Browse files
committed
Apply second-pass polish: return parsed numeric options, tighten key-order test
- parseIntOption / parseDoubleOption now return the parsed value so validateOptions does not re-parse the canonicalized string solely to check numeric bounds. - The unknown-option-key stable-order test now matches the rendered list literal "[k, max_distance, min_score, filter_type]" rather than using indexOf("k"), which would match the "k" in "Unknown option key" and reduce the assertion to a tautology. - Adds a direct unit test pinning parseOptions("") -> empty map, so the wholly-empty option string contract (which flows to the "Missing required option" gate) is exercised explicitly. No functional change for user-visible paths. Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 12c5553 commit c0faf7c

2 files changed

Lines changed: 21 additions & 14 deletions

File tree

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

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -296,50 +296,49 @@ private void validateOptions(Map<String, String> options) {
296296
}
297297
// Parse and canonicalize numeric values — closes JSON injection via option values
298298
if (hasK) {
299-
parseIntOption(options, "k");
300-
int k = Integer.parseInt(options.get("k"));
299+
int k = parseIntOption(options, "k");
301300
if (k < 1 || k > 10000) {
302301
throw new ExpressionEvaluationException(
303302
String.format("k must be between 1 and 10000, got %d", k));
304303
}
305304
}
306305
if (hasMaxDistance) {
307-
parseDoubleOption(options, "max_distance");
308-
double maxDistance = Double.parseDouble(options.get("max_distance"));
306+
double maxDistance = parseDoubleOption(options, "max_distance");
309307
if (maxDistance < 0) {
310308
throw new ExpressionEvaluationException(
311309
String.format(
312310
"max_distance must be non-negative, got %s", options.get("max_distance")));
313311
}
314312
}
315313
if (hasMinScore) {
316-
parseDoubleOption(options, "min_score");
317-
double minScore = Double.parseDouble(options.get("min_score"));
314+
double minScore = parseDoubleOption(options, "min_score");
318315
if (minScore < 0) {
319316
throw new ExpressionEvaluationException(
320317
String.format("min_score must be non-negative, got %s", options.get("min_score")));
321318
}
322319
}
323320
}
324321

325-
private void parseIntOption(Map<String, String> options, String key) {
322+
private int parseIntOption(Map<String, String> options, String key) {
326323
try {
327324
int value = Integer.parseInt(options.get(key));
328325
options.put(key, Integer.toString(value));
326+
return value;
329327
} catch (NumberFormatException e) {
330328
throw new ExpressionEvaluationException(
331329
String.format("Option '%s' must be an integer, got '%s'", key, options.get(key)));
332330
}
333331
}
334332

335-
private void parseDoubleOption(Map<String, String> options, String key) {
333+
private double parseDoubleOption(Map<String, String> options, String key) {
336334
try {
337335
double value = Double.parseDouble(options.get(key));
338336
if (!Double.isFinite(value)) {
339337
throw new ExpressionEvaluationException(
340338
String.format("Option '%s' must be a finite number, got '%s'", key, options.get(key)));
341339
}
342340
options.put(key, Double.toString(value));
341+
return value;
343342
} catch (NumberFormatException e) {
344343
throw new ExpressionEvaluationException(
345344
String.format("Option '%s' must be a number, got '%s'", key, options.get(key)));

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

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -703,12 +703,20 @@ void applyArguments_unknownOptionKeyErrorListsSupportedKeysInStableOrder() {
703703
createImplWithArgs("my-index", "embedding", "[1.0, 2.0]", "k=5,bogus=1");
704704
ExpressionEvaluationException ex =
705705
assertThrows(ExpressionEvaluationException.class, () -> impl.applyArguments());
706-
String msg = ex.getMessage();
707-
int kIdx = msg.indexOf("k");
708-
int maxIdx = msg.indexOf("max_distance");
709-
int minIdx = msg.indexOf("min_score");
710-
int filterIdx = msg.indexOf("filter_type");
711-
assertTrue(kIdx >= 0 && maxIdx > kIdx && minIdx > maxIdx && filterIdx > minIdx);
706+
// Match the rendered list literal (e.g. "[k, max_distance, min_score, filter_type]") rather
707+
// than searching for the substring "k", which would match the first "k" in "Unknown option
708+
// key" and reduce the assertion to a tautology.
709+
assertTrue(
710+
ex.getMessage().contains("[k, max_distance, min_score, filter_type]"),
711+
"expected stable key order in error; got: " + ex.getMessage());
712+
}
713+
714+
@Test
715+
void parseOptions_emptyStringReturnsEmptyMap() {
716+
// The wholly empty option string is explicitly allowed through parseOptions so it flows to
717+
// the "Missing required option" gate in validateOptions. Pins that contract.
718+
Map<String, String> opts = VectorSearchTableFunctionImplementation.parseOptions("");
719+
assertTrue(opts.isEmpty());
712720
}
713721

714722
private VectorSearchTableFunctionImplementation createImpl() {

0 commit comments

Comments
 (0)