[ES|QL] Support Named and Positional Parameters in EsqlQueryRequest by fang-xing-esql · Pull Request #108421 · elastic/elasticsearch

fang-xing-esql · 2024-05-08T15:57:44Z

Unnamed parameters are already supported in EsqlQueryRequest. e.g.

{
  "version": "2024.04.01",
  "query": "ROW x = ?",
  "params": [{"value": 1, "type" : "integer"}]
}

       x       
---------------
1

This PR is to support named and positional parameters in EsqlQueryRequest, e.g.

Named parameter:

{
  "version": "2024.04.01",
  "query": "ROW x = ?name1, y = ?name1",
  "params": [{"name1": 1}]
}

       x       |       y       
---------------+---------------
1              |1

Positional parameter

{
  "version": "2024.04.01",
  "query": "ROW x = ?1, y = ?1",
  "params": [{"name1": 1}]
}
'

       x       |       y       
---------------+---------------
1              |1

And also drop the support to explicit type:

{
  "version": "2024.04.01",
  "query": "ROW x = ?",
  "params": [{"value": 1}]
}

       x       
---------------
1

elasticsearchmachine · 2024-05-08T15:58:08Z

Hi @fang-xing-esql, I've created a changelog YAML for you.

costin

Had a call with Fang about this.
FTR:

mixing anonymous, named and ordinal params should not be allowed
drop param type, we never use it/advertised it
change List to a Params like object that returns the value of the given input param and does validation (for params to not be mixed, invalid ordinal, names, etc...)

libs/x-content/src/main/java/org/elasticsearch/xcontent/ObjectParser.java

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

elasticsearchmachine · 2024-05-14T21:38:43Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

bpintea

Had a first look, left some comments. I guess deciding to remove the type would make things clearer about how to progress.

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/Params.java

bpintea · 2024-05-15T14:42:58Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java

 final class RequestXContent {

-    private static final ConstructingObjectParser<TypedParamValue, Void> PARAM_PARSER = new ConstructingObjectParser<>(
+    private static class TempObjects {


This feels like a strange class to have. I guess if we get rid of the Params it might no longer be needed.

costin

This needs more work. In its current form it's breaking bwc since it changes the declaration for anonymous / un-named params from values: [1,2,3, "string"] to values: {[value:1], ...

This is of course a problem

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/Params.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlParser.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

costin · 2024-05-17T21:11:46Z

Unnamed parameters are already supported in EsqlQueryRequest. e.g.

{
  "version": "2024.04.01",
  "query": "ROW x = ?",
  "params": [{"value": 1, "type" : "integer"}]
}

       x       
---------------
1

^
This is inaccurate - this style was supported by not advertised and now it is being removed.
The PR should preserve the current style which is:

{
"version": "2024.04.01",
"query": "ROW x = ?",
"params": [2, 3, "string"]
}

fang-xing-esql · 2024-05-22T13:54:48Z

This needs more work. In its current form it's breaking bwc since it changes the declaration for anonymous / un-named params from values: [1,2,3, "string"] to values: {[value:1], ...

This is of course a problem

We will keep params as an array, since it is already an array, not an object.

astefan

I haven't looked at the tests yet, but there are aspects that can be improved regarding the error messaging and, probably, the way these error messages are tested.

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/StringUtils.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/QueryParam.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

elasticsearchmachine · 2024-06-04T14:23:50Z

Pinging @elastic/kibana-esql (ES|QL-ui)

fang-xing-esql · 2024-06-04T15:56:57Z

I haven't looked at the tests yet, but there are aspects that can be improved regarding the error messaging and, probably, the way these error messages are tested.

Thanks for reviewing @astefan ! The tests and messages are updated in the last commit, just need to clean up CI.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

costin

LGTM

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/10_basic.yml

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlParser.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/QueryParams.java

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

fang-xing-esql · 2024-06-07T17:20:30Z

I haven't looked at the tests yet, but there are aspects that can be improved regarding the error messaging and, probably, the way these error messages are tested.

Thank you so much for thorough review @astefan !! I believe that the existing comments are all addressed. Please just let me know if I missed anything.

astefan

For the ParsingExceptions that I was suggesting to have the location included in the message in EsqlParser, please also adjust any tests that assert those messages.

Once the latest comments are addressed, this LGTM. Thank you for your patience with all the comments I had.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/QueryParams.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlParser.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java

…eld (#184361) ## Summary Based on elastic/elasticsearch#108421 Closes #180805 It allows the users to add an `?earliest` and `?latest` variable in the ES|QL editor. When we are detecting this variable, we are also sending the values to ES. - Earliest is the from value of the date picker - Latest is the to value of the date picker ![meow](https://github.com/elastic/kibana/assets/17003240/177ceafe-558d-43ea-809c-f0ec2833ec17) Usage in bucket ![meow](https://github.com/elastic/kibana/assets/17003240/8dea8188-b4e0-43e6-894e-236811374030) This enables 2 very important features: - I can use the values of the time picker in the bucket function - I can use the time picker for indices without `@timestamp` field ### For reviewers - Although it seems as a big PR, the majority of the changes happen due to the signature change of the `getESQLAdHocDataview` - The ML changes are mostly because the ML code has a lot of repetition. I think the code needs to be refactored to have a central point (preferably the `getESQLResults` from the esql-utils. I will create an issue for the ML team. - I am not proposing this in bucket autocomplete because it doesnt work great in general for the date histogram case. We are working on autocomplete improvements so I am expecting this to be part of a follow up PR. - I want to talk to the docs team to add it in the docs. ### Follow ups - Change the histogram to use the bucket instead of the date_trunc (needs investigation first) - Speak with the docs team about adding an example on our official docs ### Flaky test runner https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6521 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

fang-xing-esql added 3 commits May 8, 2024 10:24

support named parameter in EsqlQueryRequest

aae5b4c

Merge branch 'main' into namedParameters

28e7dd0

fix tests

7697155

fang-xing-esql added >enhancement :Analytics/ES|QL AKA ESQL v8.15.0 labels May 8, 2024

Update docs/changelog/108421.yaml

bc6c7f8

fang-xing-esql requested review from astefan, bpintea and costin May 8, 2024 15:58

costin requested changes May 8, 2024

View reviewed changes

fang-xing-esql added 4 commits May 10, 2024 19:35

better antlr tokens and rules

9a9085a

Merge branch 'main' into namedParameters

1d0ea40

do not allow mixed parameters

c2147f6

Merge branch 'main' into namedParameters

f025da5

fang-xing-esql added the ES|QL-ui Impacts ES|QL UI label May 13, 2024

fang-xing-esql added 3 commits May 13, 2024 12:48

Merge branch 'main' into namedParameters

edff49c

fix ci failures

95110bc

dissect and grok are not supported yet

74eb1c3

fang-xing-esql marked this pull request as ready for review May 14, 2024 21:38

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 14, 2024

bpintea reviewed May 15, 2024

View reviewed changes

costin requested changes May 17, 2024

View reviewed changes

fang-xing-esql added 2 commits May 21, 2024 12:59

Merge branch 'main' into namedParameters

1cb1a3c

Merge branch 'main' into namedParameters

bcf5a04

update according to review comments

2e5324e

astefan reviewed Jun 3, 2024

View reviewed changes

fang-xing-esql added 4 commits June 3, 2024 21:01

Merge branch 'main' into namedParameters

1ee8092

update tests and messages

8430434

Merge branch 'main' into namedParameters

4389e4a

update tests and messages

d82f72d

fang-xing-esql added 2 commits June 4, 2024 15:40

Merge branch 'main' into namedParameters

4196362

Merge branch 'main' into namedParameters

e8b954d

astefan reviewed Jun 5, 2024

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java Outdated Show resolved Hide resolved

Merge branch 'main' into namedParameters

0cade07

costin approved these changes Jun 6, 2024

View reviewed changes

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/10_basic.yml Show resolved Hide resolved

update accoding to review comments

9cb7cb7

astefan reviewed Jun 6, 2024

View reviewed changes

update according to review comments

de32c69

astefan reviewed Jun 7, 2024

View reviewed changes

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java Outdated Show resolved Hide resolved

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java Outdated Show resolved Hide resolved

update according to review comments

05e433b

astefan approved these changes Jun 10, 2024

View reviewed changes

fang-xing-esql added 3 commits June 10, 2024 10:47

Merge branch 'main' into namedParameters

8e492f7

update according to review comments and add EsqlCapabilities

fab51eb

Merge branch 'main' into namedParameters

9bdceca

fang-xing-esql merged commit ccef9e7 into elastic:main Jun 11, 2024

stratoula mentioned this pull request Jun 17, 2024

[ES|QL] Identify named parameters constants and get the fields elastic/kibana#186305

Closed

ChrisHegarty mentioned this pull request Jun 21, 2024

[ES|QL][Named parameters] Not sending the params in a query with named parameters results in blocking all future _query requests #110028

Closed

fang-xing-esql mentioned this pull request Jul 23, 2024

[DOCS] ES|QL Add examples in docs for named and positional parameters #111178

Merged

fang-xing-esql deleted the namedParameters branch August 5, 2024 13:25

fang-xing-esql mentioned this pull request Aug 8, 2024

[ES|QL] Support date_period and time_duration in named parameters #111717

Closed

ivancea mentioned this pull request Jan 21, 2026

ESQL: Timezone API docs elastic/elasticsearch-specification#5956

Merged

Conversation

fang-xing-esql commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented May 8, 2024

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented May 14, 2024

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bpintea May 15, 2024

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

costin commented May 17, 2024

Uh oh!

fang-xing-esql commented May 22, 2024

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 4, 2024

Uh oh!

fang-xing-esql commented Jun 4, 2024

Uh oh!

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fang-xing-esql commented May 8, 2024 •

edited

Loading