-
Notifications
You must be signed in to change notification settings - Fork 211
[BugFix] Return 400 instead of 500 on vectorSearch() argument-count mismatch #5589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,7 +162,7 @@ private Optional<FunctionBuilder> resolve( | |
| || sourceTypes.equals(targetTypes)) { | ||
| return Optional.of(funcBuilder); | ||
| } | ||
| return Optional.of(castArguments(sourceTypes, targetTypes, funcBuilder)); | ||
| return Optional.of(castArguments(functionName, sourceTypes, targetTypes, funcBuilder)); | ||
| } else { | ||
| return Optional.empty(); | ||
| } | ||
|
|
@@ -175,8 +175,24 @@ private Optional<FunctionBuilder> resolve( | |
| * this case, wrap F and return equal(BOOL, cast_to_bool(STRING)). | ||
| */ | ||
| private FunctionBuilder castArguments( | ||
| List<ExprType> sourceTypes, List<ExprType> targetTypes, FunctionBuilder funcBuilder) { | ||
| FunctionName funcName, | ||
| List<ExprType> sourceTypes, | ||
| List<ExprType> targetTypes, | ||
| FunctionBuilder funcBuilder) { | ||
| return (fp, arguments) -> { | ||
| // A resolver may return a fixed-arity resolved signature regardless of how many arguments it | ||
| // was called with (e.g. a table function called with a duplicate named argument). When more | ||
| // arguments are supplied than the resolved signature declares, the cast loop would index past | ||
| // the end of the resolved type list and throw an unchecked IndexOutOfBoundsException that | ||
| // surfaces as HTTP 500. Reject that overflow with a clean ExpressionEvaluationException. | ||
| // Under-arity is left to the resolver's own validation, which produces a more specific | ||
| // message; it cannot overflow the resolved type list. | ||
| if (arguments.size() > targetTypes.size()) { | ||
| throw new ExpressionEvaluationException( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be IllegalArgumentException or Syntax exception?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| String.format( | ||
| "Expected %d arguments for function %s but received %d", | ||
| targetTypes.size(), funcName.getFunctionName(), arguments.size())); | ||
| } | ||
| List<Expression> argsCasted = new ArrayList<>(); | ||
| for (int i = 0; i < arguments.size(); i++) { | ||
| Expression arg = arguments.get(i); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>and not!=?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is method is very generic. So the changes here will fix all such mismatch argument case for similar functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's intended, though the real scope is narrower than it looks. Normal functions never reach this path, since
DefaultFunctionResolverrejects an arity mismatch atmatch()first. It only matters for custom resolvers that hand rollresolve()to return a fixed arity signature without size matching the input, like the table function resolvers (vectorSearch, and the Prometheus ones follow the same shape). So it covers all fixed arity custom resolvers, which is why I put the guard here rather than only in vectorSearch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah,
>is on purpose. The crash only happens on overflow, when there are more args than the resolved signature, so the cast loop runs off the end oftargetTypes. Fewer args can't overflow, and under arity is already caught upstream anyway: normal functions reject it at the resolver viamatch()returning NOT_MATCH, and vectorSearch catches it invalidateArgumentswith the specific "requires 4 arguments (table, field, vector, option)" message. So!=would just shadow those two better messages with this generic one. One side effect worth noting: over vs under arity now produce slightly different wording, both 400s, so no functional difference.