[ES|QL] Client side validation enhancements

[stratoula]

Kibana version:

Here we are gathering enhancements of the client side validation which means things that ES is marking as errors but we allow them. The impact is low because the users will get the errors when run the query on the server side.

• [ ] from kibana_sample_data_logs | where 23 — where shouldn't accept non-boolean expressions

• [ ] We could validate if the user is trying to use a field which is not available in a pipe such as

... | eval b = ... | stats a = avg(field) | keep b

Here b is not valid as stats is not using it

• [ ] https://github.com/elastic/kibana/issues/186409

• [ ] ... | STATS AVG(false OR false) should be an error. AVG does not support boolean. The validator works correctly with a literal: | STATS AVG(false), just not a function (OR) return type.

• [ ] We don't support dotted field names with separate parts escaped

FROM kibana_sample_data_logs | KEEP `geo`.`dest`

• [ ] We could consider validating named variables like ?t_start and ?t_end

[elasticmachine]

Pinging @elastic/kibana-visualizations (Team:Visualizations)

[elasticmachine]

Pinging @elastic/kibana-esql (Team:ESQL)

[drewdaemon]

Added problem case: from kibana_sample_data_logs | where 23 — where shouldn't accept non-boolean expressions

[stratoula]

Nice catch!

[dej611]

Added problem case: from kibana_sample_data_logs | where 23 — where shouldn't accept non-boolean expressions

This is a legacy problem. ES has been accepting them casting any non-boolean values as false in the past so the validation code had to be relaxed. It can be made stricter again but mind that can change again.

[stratoula]

I think the latter addition from index | stats max(<date_field>) is much more important case as is a very common thing to do and we should prioritize it. The from kibana_sample_data_logs | where 23 is not bothering me so much as the editor doesn't complain, it should but is ok if we fix it with lower priority. But the aggregations with date fields is an important mis-validation from our side.

[dej611]

I went checking the ES annotations for Max but it's not mention any date: https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Max.java

Also the documentation seems to not mention dates.

Given the work on auto sync the signatures from ES, it is probably worth notifying this thing to them as well, to avoid regressions.

[drewdaemon]

@dej611 thanks for looking into that! It's a good idea to let them know, but FWIW we've changed strategy and won't be using those signature annotations in https://github.com/elastic/kibana/issues/179634. Instead, the function signatures will be generated from the unit tests in Elasticsearch (ref).

[dej611]

There are no date tests for the max aggs function in Elasticsearch: https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/aggregation/, hence the lack of annotation.

[sphilipse]

Index patterns with a negative produce a syntax error, despite working perfectly:

(this pattern should return all indices except those that start with a .)

[drewdaemon]

@sphilipse thanks for the report

[drewdaemon]

Pretty much all functions and operators can accept null in place of any parameter. We should update the validation to reflect this.

[stratoula]

We are tracking valid time units as wrong

[drewdaemon]

We aren't catching nested aggregation functions (e.g. from kibana_sample_data_logs | stats avg(to_long(avg(1))))

[stratoula]

I looked through all the open issues here and except maybe from this

 Wrapped boolean expression are not always handled with their final type:

all the others are mostly enhancements. So I am lowering the impact from high to medium.

[stratoula]

Using field names that are also field types cause the validation to complain wrongly

[dej611]

Using field names that are also field types cause the validation to complain wrongly

The ip field is of type IP. I guess trim implicitly cast it to string on ES side.

[stratoula]

Sorry MArco what do you mean? If I use meow instead of ip the client side validator won't complain. Dissect must return strings if I am not mistaken

[dej611]

Yeah, you are right. I guess that is a type conflict that the validation logic won't fix by default. I guess that is another issue of the Make variable/fields check aware of query location problem.

[drewdaemon]

... | STATS AVG(false OR false) should be an error. AVG does not support boolean. The validator works correctly with a literal: ... | STATS AVG(false), just not a function return type.

cc @stratoula

[stratoula]

I cleaned it up and kept here only the enhancements (also updated the description)

We are going to gather the bugs here https://github.com/elastic/kibana/issues/192255

Validation bugs are things we are marking as errors while they are not in ES side