[ES|QL] Support Named and Positional Parameters in EsqlQueryRequest

[fang-xing-esql]

Resolve #107029

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]

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

[elasticsearchmachine]

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

[costin]

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]

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.

[fang-xing-esql]

Thanks for the first round of reviewing @costin @bpintea !! I think I have addressed most of the comments if not all yet. Could you please take another look at the PR?

1. EsqlQueryRequest params is an array, not an object.
2. Explicit type is not supported in the parameters, if users specify a date/time interval, like 3 days, 12 hours, they will be taken as String/KEYWORD, not DATE_PERIOD or TIME_DURATION, another issue/PR will be open to create the casting functions to convert String to DATE_PERIOD/TIME_DURATION, so that users can specify them explicitly in the query.
3. Mixed named, positional or anonymous parameters are not allowed in the query or in the params.
4. I kept the Param, so that we can put the inferred data type in it, and also as a wrapper for null values.
5. Removed parameter in limit clause, it can be added back when needed, parameter is not supported in grok or dissect in this PR.

[stratoula]

@fang-xing-esql I am playing with your PR, really looking forward to it!

I see that when the params is not provided then the query will fail

I am just thinking that possibly someone copies this query and pastes it somewhere else where these params are not provided. Does it make sense to default to something if params is not provided?

[fang-xing-esql]

@fang-xing-esql I am playing with your PR, really looking forward to it!

I see that when the params is not provided then the query will fail

I am just thinking that possibly someone copies this query and pastes it somewhere else where these params are not provided. Does it make sense to default to something if params is not provided?

@stratoula Thanks for trying the PR! In this particular case, I think returning an error to the users is not a bad choice. It is not difficult to provide a default value(we don't have statistics yet, so this is just a guess, like 0 for numeric) in case we don't find the named parameters in the request, however if I am the user, I might want to get an error in stead, so that I can modify my query, as in most cases, my intention is to provide different literal values for each run of the query.

[costin]

@fang-xing-esql Let's improve the error messages and make them more user friendly: instead of no parameter defined for name ?avg, how about unknown query parameter [avg]. The message should take into account if any parameters are defined unknown query parameter [avg] (none were defined) and if so, do a fuzzy search like we do for the fields: unknown query parameter [agv], did you mean [avg]?. We should definitely do the latter - not sure about the former. Separately I think we should try and see if we can validate all params in one go so the user doesn't have to run the query multiple times.

[fang-xing-esql]

@fang-xing-esql Let's improve the error messages and make them more user friendly: instead of no parameter defined for name ?avg, how about unknown query parameter [avg]. The message should take into account if any parameters are defined unknown query parameter [avg] (none were defined) and if so, do a fuzzy search like we do for the fields: unknown query parameter [agv], did you mean [avg]?. We should definitely do the latter - not sure about the former. Separately I think we should try and see if we can validate all params in one go so the user doesn't have to run the query multiple times.

@costin Thanks for the suggestions! It is a good idea to validate all params in one go and provide better error messages, it is more user friendly and a lot of test cases can be combined together. The validations of params in both EsqlQueryRequest and parser are in one go in the last commit.

[elasticsearchmachine]

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

[fang-xing-esql]

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.

[fang-xing-esql]

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.