-
Notifications
You must be signed in to change notification settings - Fork 25.7k
ES|QL - Allow multivalued query parameters #134317
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
ES|QL - Allow multivalued query parameters #134317
Conversation
|
Hi @carlosdelest, I've created a changelog YAML for you. |
179d9b2 to
b13d467
Compare
|
Hi @carlosdelest, I've updated the changelog YAML for you. |
| case ZonedDateTime zonedDateTime -> { | ||
| return DATETIME; | ||
| } | ||
| case List<?> list -> { |
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.
Add data type resolution for lists
| } | ||
| } else { | ||
| paramValue = null; | ||
| if (token == XContentParser.Token.VALUE_STRING) { |
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 code moved to parseSingleParamValue - it can be confusing, switching to split view can be a better experience for reviewing this class
…i-valued-params # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java
| /** | ||
| * Multivalued query parameters | ||
| */ | ||
| QUERY_PARAMS_MULTI_VALUES(Build.current().isSnapshot()); |
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.
I'm thinking on not making this available just for snapshots, but would like to hear concerns about it
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.
I think the best practice is to remove snapshot in a followup PR right?
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.
We might want to give a heads up to kibana, and add test-release label if this is behind snapshot.
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.
I think the best practice is to remove snapshot in a followup PR right?
We tend to do that when there's incomplete work or we have to make a decision about it being useful or not as it is. I think this doesn't need to be hidden behind snapshot, as it's useful and even if we want to improve it in the future we can do out of snapshot.
Happy to hear other thoughts!
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.
I've removed the snapshot capability in b9a03ff - this change will go on all builds
|
Hi @carlosdelest, I've updated the changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
kderusso
left a comment
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.
Changes LGTM
| /** | ||
| * Multivalued query parameters | ||
| */ | ||
| QUERY_PARAMS_MULTI_VALUES(Build.current().isSnapshot()); |
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.
I think the best practice is to remove snapshot in a followup PR right?
fang-xing-esql
left a comment
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.
Thank you for picking this up @carlosdelest. LGTM, I added some comments around the error message.
| ParamValueAndType valueAndDataType = parseSingleParamValue(p, errors); | ||
| DataType currentType = valueAndDataType.type; | ||
| if (currentType == DataType.NULL) { | ||
| errors.add(new XContentParseException(loc, "Unnamed parameter contains a null in a multivalued value")); |
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.
It would be better if error messages were consistent between named and unnamed parameters.
Does this sound good?
[paramValues.toString()] contains a null entry. Null values are not allowed in multivalued params.
If we want a complete list of paramValues in the error message, a flag can be set if there is null value in the list, and this error can be added after this while loop.
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.
That sounds good. I did that on a846f9a
| errors.add( | ||
| new XContentParseException( | ||
| loc, | ||
| "Unnamed parameter has values from different types, found " + arrayType + " and " + currentType |
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.
Same as above, it would be better if error messages were consistent between named and unnamed parameters.
How about
[paramValues.toString()] contains mixed data types. Mixed data types are not allowed in multivalued params.
Do we want to allow mixed numeric values like [1, 2.0]? If it is not relevant to the current context, it can be added later.
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.
Changed on a846f9a .
Do we want to allow mixed numeric values like [1, 2.0]? If it is not relevant to the current context, it can be added later.
I'd say we can do that later if needed.
| errors.add( | ||
| new XContentParseException( | ||
| loc, | ||
| "Parameter [" + entry.getKey() + "] has values from different types, found " + arrayType + " and " + currentType |
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.
A consistent error message as the unnamed param might be better.
[valueList.toString()] contains mixed data types. Mixed data types are not allowed in multivalued params., Perhaps refactor this error message to a method that can be access by both named and unnamed params.
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.
Let me know whether a846f9a fixes this.
| errors.add( | ||
| new XContentParseException( | ||
| loc, | ||
| "Parameter [" + entry.getKey() + "] contains a null value. Null values are not allowed for multivalues" |
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.
[valueList.toString()] contains a null entry. Null values are not allowed in multivalued params.
| Arrays.fill(queryVector, 0); | ||
| EsqlQueryRequest queryRequest = new EsqlQueryRequest(); | ||
| QueryParams queryParams = new QueryParams( | ||
| List.of(new QueryParam("queryVector", Arrays.asList(queryVector), DataType.INTEGER, ParserUtils.ParamClassification.PATTERN)) |
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.
Is queryVector a constant value? Why this param set as a PATTERN?
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.
My bad - fixed in f1909da
| /** | ||
| * Multivalued query parameters | ||
| */ | ||
| QUERY_PARAMS_MULTI_VALUES(Build.current().isSnapshot()); |
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.
We might want to give a heads up to kibana, and add test-release label if this is behind snapshot.
…ued-params' into enhancement/esql-multi-valued-params # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
…i-valued-params # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java
|
Pinging @elastic/kibana-esql (ES|QL-ui) |
Adds support for multivalues as named and unnamed ES\QL query parameters.
The following is now possible:
Parameters cannot contain a null value. Multivalues are not expected to contain them as they are not kept at the storage layer.
This will allow for dense_vector values to be provided to dense_vector related functions.
Related: