[ES|QL] Client side validation enhancements #177699

stratoula · 2024-02-23T12:04:20Z

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

[ES|QL] better inline casting validation with ES metadata #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 could consider validating named variables like ?t_start and ?t_end

The text was updated successfully, but these errors were encountered:

elasticmachine · 2024-02-23T12:04:39Z

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

## Summary Related issue #177699 Fix variables logic for expressions at `stats by ...` level. Fix validation logic for agg functions within `eval` or `where` scope. Fix validation and autocomplete logic for nested quoted expressions * i.e. ``` from index | eval round(numberField) + 1 | eval `round(numberField) + 1` + 1 | eval ```round(numberField) + 1`` + 1` + 1 | eval ```````round(numberField) + 1```` + 1`` + 1` + 1 | eval ```````````````round(numberField) + 1```````` + 1```` + 1`` + 1` + 1 | keep ```````````````````````````````round(numberField) + 1```````````````` + 1```````` + 1```` + 1`` + 1` ``` * updated `count_distinct` agg definition to have the `precision` second optional param. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

## Summary Related issue elastic#177699 Fix variables logic for expressions at `stats by ...` level. Fix validation logic for agg functions within `eval` or `where` scope. Fix validation and autocomplete logic for nested quoted expressions * i.e. ``` from index | eval round(numberField) + 1 | eval `round(numberField) + 1` + 1 | eval ```round(numberField) + 1`` + 1` + 1 | eval ```````round(numberField) + 1```` + 1`` + 1` + 1 | eval ```````````````round(numberField) + 1```````` + 1```` + 1`` + 1` + 1 | keep ```````````````````````````````round(numberField) + 1```````````````` + 1```````` + 1```` + 1`` + 1` ``` * updated `count_distinct` agg definition to have the `precision` second optional param. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> (cherry picked from commit cad276f) # Conflicts: # packages/kbn-monaco/src/esql/lib/ast/validation/esql_validation_meta_tests.json

## Summary Part of #177699 We had `case` marked as if it required three parameters when in reality it only requires two. <img width="600" alt="Screenshot 2024-03-19 at 4 23 29 PM" src="https://github.com/elastic/kibana/assets/315764/45f7578a-e6ad-4ba9-b71a-05bb1978a384"> Note: we could consider testing these n-1 cases to prevent this kind of bug in the future. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>

## Summary Part of elastic#177699 We had `case` marked as if it required three parameters when in reality it only requires two. <img width="600" alt="Screenshot 2024-03-19 at 4 23 29 PM" src="https://github.com/elastic/kibana/assets/315764/45f7578a-e6ad-4ba9-b71a-05bb1978a384"> Note: we could consider testing these n-1 cases to prevent this kind of bug in the future. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com> (cherry picked from commit 28277c2) # Conflicts: # packages/kbn-monaco/src/esql/lib/ast/validation/esql_validation_meta_tests.json

# Backport This will backport the following commits from `main` to `8.13`: - [[ES|QL] case only requires two parameters (#179011)](#179011)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

# Backport This will backport the following commits from `main` to `8.13`: - [[ES|QL] Fix some validation misconfiguration (#177783)](#177783)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Marco Liberati <dej611@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

elasticmachine · 2024-03-27T07:46:11Z

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

…79707) ## Summary Part of #177699 `AND` and `OR` were not accepting `NULL` as an argument when they should have. The most basic test case is `ROW var = TRUE or NULL`. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

drewdaemon · 2024-04-10T20:53:00Z

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

stratoula · 2024-04-11T07:41:07Z

Nice catch!

dej611 · 2024-04-16T07:42:14Z

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 · 2024-04-16T07:54:15Z

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 · 2024-04-16T08:05:55Z

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 · 2024-04-16T13:37:59Z

@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 #179634. Instead, the function signatures will be generated from the unit tests in Elasticsearch (ref).

dej611 · 2024-04-16T13:59:40Z

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.

## Summary This is a follow-on from elastic/elasticsearch#107859. Two main changes to our client-side validation code. 1. comparison operators like `==` and `>=` now support implicit casting from strings for `version`, `ip`, and `boolean` types 2. `in` and `not in` operators support implicit casting from strings for `version`, `ip`, `boolean`, and `date` constants. To address this quickly, I have disabled type-checking for array values (e.g. `in ( version_field, "1.2.3", "2.3.1" )`) because our parameter typing system does not currently support arrays of mixed types which it will need to in order to re-enable checking while allowing string literals. I have added this to elastic#177699 ### A note on testing These implicit casting changes may not be on the latest Elasticsearch snapshot. Instead, check out the `8.14` branch in a local Elasticsearch repo and run `yarn es source --source-path='path/to/elasticsearch'` See [these tests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763) for a set of good use cases to try. `to_<type>` functions can be used if the sample data doesn't contain a field of the type you want to test. ### A note on string to date casting In elastic#182856 we started accepting string literals for any function arguments that are dates. By the same logic, `"2022" - 1 day` would work, so our validator doesn't complain about it. However, it currently fails at the Elasticsearch level. In a discussion with Fang, we determined that this is a valid use case, so I have created elastic/elasticsearch#108432 and determined not to tighten the client-side validation since this seems more like a casting "hole" that will soon be filled in on the ES side (though not in 8.14). ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or (cherry picked from commit c3e1027)

# Backport This will backport the following commits from `main` to `8.14`: - [[ES|QL] implicit casting changes (#182989)](#182989)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Drew Tate <drew.tate@elastic.co>

sphilipse · 2024-05-15T13:16:22Z

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 · 2024-05-15T13:21:52Z

@sphilipse thanks for the report

drewdaemon · 2024-05-22T16:39:55Z

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

stratoula · 2024-06-07T11:45:47Z

We are tracking valid time units as wrong

drewdaemon · 2024-06-10T20:37:05Z

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

stratoula · 2024-06-20T11:24:38Z

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 · 2024-07-05T12:03:22Z

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

dej611 · 2024-07-05T12:18:12Z

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 · 2024-07-05T12:28:27Z

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 · 2024-07-05T12:43:39Z

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 · 2024-07-15T20:02:59Z

... | 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

## Summary Part of #189662 ## Suggests comma and pipe https://github.com/user-attachments/assets/c09bc6fd-20a6-4f42-a871-c70e68e6d81a ## Doesn't suggest comma when there are no more fields https://github.com/user-attachments/assets/29fce13e-e58b-4d93-bce5-6b1f913b4d92 ## Doesn't work for escaped columns :( https://github.com/user-attachments/assets/3d65f3b9-923d-4c0e-9c50-51dd83115c8b As part of this effort I discovered #191100 and #191105, as well as a problem with column name validation (see #177699) I think we can revisit column escaping and probably resolve all of these issues (issue [here](#191111)). ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

stratoula · 2024-09-06T11:39:43Z

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

We are going to gather the bugs here #192255

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

…191312) ## Summary Part of #189662 Also, a follow-on to #187184 with certain source names (e.g. `foo$bar`, `my-policy`) and fields in the `ENRICH` command. During this effort I discovered #191321 and a bug with the validation of field names in the "ENRICH ... WITH" list (documented [here](#177699)), both of which will be addressed separately. ## Sources ### General flow https://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf ### Works with wild-card matches https://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c ### `METADATA` field list https://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a ## ENRICH Autosuggest now helps you along https://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f Also, fixed this bug (follow on to #187184 ) https://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85 ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

…lastic#191312) ## Summary Part of elastic#189662 Also, a follow-on to elastic#187184 with certain source names (e.g. `foo$bar`, `my-policy`) and fields in the `ENRICH` command. During this effort I discovered elastic#191321 and a bug with the validation of field names in the "ENRICH ... WITH" list (documented [here](elastic#177699)), both of which will be addressed separately. ## Sources ### General flow https://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf ### Works with wild-card matches https://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c ### `METADATA` field list https://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a ## ENRICH Autosuggest now helps you along https://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f Also, fixed this bug (follow on to elastic#187184 ) https://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85 ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> (cherry picked from commit e404a39)

…60;ENRICH` (#191312) (#193013) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] open suggestions automatically in sources lists and `ENRICH` (#191312)](#191312)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Drew Tate <drew.tate@elastic.co>

…lastic#191312) ## Summary Part of elastic#189662 Also, a follow-on to elastic#187184 with certain source names (e.g. `foo$bar`, `my-policy`) and fields in the `ENRICH` command. During this effort I discovered elastic#191321 and a bug with the validation of field names in the "ENRICH ... WITH" list (documented [here](elastic#177699)), both of which will be addressed separately. ## Sources ### General flow https://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf ### Works with wild-card matches https://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c ### `METADATA` field list https://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a ## ENRICH Autosuggest now helps you along https://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f Also, fixed this bug (follow on to elastic#187184 ) https://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85 ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

stratoula added the bug Fixes for quality problems that affect the customer experience label Feb 23, 2024

botelastic bot added the needs-team Issues missing a team label label Feb 23, 2024

stratoula added Team:Visualizations Visualization editors, elastic-charts and infrastructure impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Feb 23, 2024

botelastic bot removed the needs-team Issues missing a team label label Feb 23, 2024

stratoula added the Feature:ES|QL ES|QL related features in Kibana label Feb 23, 2024

stratoula changed the title ~~[ES|QL] Client side validation miscofniguration~~ [ES|QL] Client side validation misconfiguration Feb 23, 2024

stratoula mentioned this issue Feb 24, 2024

[ES|QL] Group by a function and reusing the column in another pipe is rendered as error while it is valid #177748

Closed

dej611 mentioned this issue Feb 26, 2024

[ES|QL] Fix some validation misconfiguration #177783

Merged

1 task

drewdaemon mentioned this issue Mar 19, 2024

[ES|QL] case only requires two parameters #179011

Merged

stratoula added Team:ESQL ES|QL related features in Kibana and removed Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Mar 27, 2024

drewdaemon mentioned this issue Mar 29, 2024

[ES|QL] client-side validation: make and and or accept null #179707

Merged

1 task

drewdaemon mentioned this issue May 8, 2024

[ES|QL] implicit casting changes #182989

Merged

1 task

drewdaemon mentioned this issue Jun 10, 2024

[ES|QL] METRICS command definition and validation #184905

Merged

2 tasks

stratoula added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Jun 20, 2024

This was referenced Aug 22, 2024

[ES|QL] Auto open suggestions for field lists #190466

Merged

[ES|QL] revisit field and variable representations #191111

Closed

drewdaemon mentioned this issue Aug 28, 2024

[ES|QL] open suggestions automatically in sources lists and ENRICH #191312

Merged

1 task

stratoula changed the title ~~[ES|QL] Client side validation misconfiguration~~ [ES|QL] Client side validation enhancements Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Client side validation enhancements #177699

[ES|QL] Client side validation enhancements #177699

stratoula commented Feb 23, 2024 •

edited by drewdaemon

Loading

elasticmachine commented Feb 23, 2024

elasticmachine commented Mar 27, 2024

drewdaemon commented Apr 10, 2024

stratoula commented Apr 11, 2024

dej611 commented Apr 16, 2024 •

edited

Loading

stratoula commented Apr 16, 2024

dej611 commented Apr 16, 2024

drewdaemon commented Apr 16, 2024

dej611 commented Apr 16, 2024

sphilipse commented May 15, 2024

drewdaemon commented May 15, 2024

drewdaemon commented May 22, 2024

stratoula commented Jun 7, 2024

drewdaemon commented Jun 10, 2024

stratoula commented Jun 20, 2024

stratoula commented Jul 5, 2024

dej611 commented Jul 5, 2024

stratoula commented Jul 5, 2024 •

edited

Loading

dej611 commented Jul 5, 2024

drewdaemon commented Jul 15, 2024 •

edited

Loading

stratoula commented Sep 6, 2024 •

edited

Loading

[ES|QL] Client side validation enhancements #177699

[ES|QL] Client side validation enhancements #177699

Comments

stratoula commented Feb 23, 2024 • edited by drewdaemon Loading

elasticmachine commented Feb 23, 2024

elasticmachine commented Mar 27, 2024

drewdaemon commented Apr 10, 2024

stratoula commented Apr 11, 2024

dej611 commented Apr 16, 2024 • edited Loading

stratoula commented Apr 16, 2024

dej611 commented Apr 16, 2024

drewdaemon commented Apr 16, 2024

dej611 commented Apr 16, 2024

sphilipse commented May 15, 2024

drewdaemon commented May 15, 2024

drewdaemon commented May 22, 2024

stratoula commented Jun 7, 2024

drewdaemon commented Jun 10, 2024

stratoula commented Jun 20, 2024

stratoula commented Jul 5, 2024

dej611 commented Jul 5, 2024

stratoula commented Jul 5, 2024 • edited Loading

dej611 commented Jul 5, 2024

drewdaemon commented Jul 15, 2024 • edited Loading

stratoula commented Sep 6, 2024 • edited Loading

stratoula commented Feb 23, 2024 •

edited by drewdaemon

Loading

dej611 commented Apr 16, 2024 •

edited

Loading

stratoula commented Jul 5, 2024 •

edited

Loading

drewdaemon commented Jul 15, 2024 •

edited

Loading

stratoula commented Sep 6, 2024 •

edited

Loading