Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ES|QL] Client side validation misconfiguration #177699

Open
17 of 24 tasks
stratoula opened this issue Feb 23, 2024 · 14 comments
Open
17 of 24 tasks

[ES|QL] Client side validation misconfiguration #177699

stratoula opened this issue Feb 23, 2024 · 14 comments
Labels
bug Fixes for quality problems that affect the customer experience Feature:ES|QL impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana

Comments

@stratoula
Copy link
Contributor

stratoula commented Feb 23, 2024 •
edited by drewdaemon

Kibana version:
8.13

In 8.13 we are introducing the client side validation which unfortunately is prone to human error. The entire functionality relies on our interpretation of the ES syntax so there are cases where we might highlight something as an error while it is not. I am going to gather all the problems I find here.

  • Grouping by an expression and referencing the generated column errors out while it should not
image image image
  • Wrong boolean validation
image Screenshot 2024-05-22 at 9 35 22 AM 
from kibana_sample_data_ecommerce | eval substring(category, 0, products.price::long)
  • 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

  • Wrapped boolean expression are not always handled with their final type:
Screenshot 2024-06-04 at 10 53 28 Screenshot 2024-06-04 at 11 32 42
@stratoula stratoula added the bug Fixes for quality problems that affect the customer experience label Feb 23, 2024
@botelastic botelastic bot added the needs-team Issues missing a team label label Feb 23, 2024
@stratoula 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
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot removed the needs-team Issues missing a team label label Feb 23, 2024
@stratoula stratoula changed the title [ES|QL] Client side validation miscofniguration [ES|QL] Client side validation misconfiguration Feb 23, 2024
@stratoula stratoula mentioned this issue Feb 24, 2024
Closed
@dej611 dej611 mentioned this issue Feb 26, 2024
Merged
1 task
dej611 added a commit that referenced this issue Mar 7, 2024
@dej611 @stratoula
[ES|QL] Fix some validation misconfiguration (#177783)
cad276f 
## 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>
drewdaemon pushed a commit to drewdaemon/kibana that referenced this issue Mar 7, 2024
@dej611
[ES|QL] Fix some validation misconfiguration (elastic#177783)
18fe06f 
## 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
@drewdaemon drewdaemon mentioned this issue Mar 19, 2024
Merged
drewdaemon added a commit that referenced this issue Mar 21, 2024
@drewdaemon @kibanamachine @mbondyra
[ES|QL] case only requires two parameters (#179011)
28277c2 
## 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>
drewdaemon added a commit to drewdaemon/kibana that referenced this issue Mar 21, 2024
@drewdaemon
[ES|QL] case only requires two parameters (elastic#179011)
4a05a70 
## 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
drewdaemon added a commit that referenced this issue Mar 21, 2024
@drewdaemon
[8.13] [ES|QL] case only requires two parameters (#179011) (#179165)
2549f8d 
# Backport

This will backport the following commits from `main` to `8.13`:
- [[ES|QL] case only requires two parameters
(#179011)](#179011)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2024-03-21T14:34:38Z","message":"[ES|QL]
case only requires two parameters (#179011)\n\n## Summary\r\n\r\nPart of
#177699 had `case`
marked as if it required three parameters when in reality\r\nit only
requires two.\r\n\r\n<img width=\"600\" alt=\"Screenshot 2024-03-19 at 4
23
29 PM\"\r\nsrc=\"https://github.com/elastic/kibana/assets/315764/45f7578a-e6ad-4ba9-b71a-05bb1978a384\">\r\n\r\nNote:
we could consider testing these n-1 cases to prevent this kind of\r\nbug
in the future.\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Marta Bondyra
<4283304+mbondyra@users.noreply.github.com>","sha":"28277c25df26796a8aa51cb4b8e82b0483c9cf83","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","backport:prev-minor","v8.14.0"],"number":179011,"url":"#179011
case only requires two parameters (#179011)\n\n## Summary\r\n\r\nPart of
#177699 had `case`
marked as if it required three parameters when in reality\r\nit only
requires two.\r\n\r\n<img width=\"600\" alt=\"Screenshot 2024-03-19 at 4
23
29 PM\"\r\nsrc=\"https://github.com/elastic/kibana/assets/315764/45f7578a-e6ad-4ba9-b71a-05bb1978a384\">\r\n\r\nNote:
we could consider testing these n-1 cases to prevent this kind of\r\nbug
in the future.\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Marta Bondyra
<4283304+mbondyra@users.noreply.github.com>","sha":"28277c25df26796a8aa51cb4b8e82b0483c9cf83"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.14.0","labelRegex":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"#179011
case only requires two parameters (#179011)\n\n## Summary\r\n\r\nPart of
#177699 had `case`
marked as if it required three parameters when in reality\r\nit only
requires two.\r\n\r\n<img width=\"600\" alt=\"Screenshot 2024-03-19 at 4
23
29 PM\"\r\nsrc=\"https://github.com/elastic/kibana/assets/315764/45f7578a-e6ad-4ba9-b71a-05bb1978a384\">\r\n\r\nNote:
we could consider testing these n-1 cases to prevent this kind of\r\nbug
in the future.\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Marta Bondyra
<4283304+mbondyra@users.noreply.github.com>","sha":"28277c25df26796a8aa51cb4b8e82b0483c9cf83"}}]}]
BACKPORT-->
drewdaemon added a commit that referenced this issue Mar 26, 2024
@drewdaemon @dej611 @kibanamachine
[8.13] [ES|QL] Fix some validation misconfiguration (#177783) (#178252)
0377c5b 
# Backport

This will backport the following commits from `main` to `8.13`:
- [[ES|QL] Fix some validation misconfiguration
(#177783)](#177783)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Marco
Liberati","email":"dej611@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-03-07T12:57:27Z","message":"[ES|QL]
Fix some validation misconfiguration (#177783)\n\n##
Summary\r\n\r\nRelated issue #177699\r\n\r\nFix variables logic for
expressions at `stats by ...` level.\r\nFix validation logic for agg
functions within `eval` or `where` scope.\r\nFix validation and
autocomplete logic for nested quoted expressions\r\n * i.e. \r\n
```\r\nfrom index | eval round(numberField) + 1 | eval
`round(numberField) + 1`\r\n+ 1 | eval ```round(numberField) + 1`` + 1`
+ 1 | eval\r\n```````round(numberField) + 1```` + 1`` + 1` + 1 |
eval\r\n```````````````round(numberField) + 1```````` + 1```` + 1`` + 1`
+ 1 |\r\nkeep ```````````````````````````````round(numberField)
+\r\n1```````````````` + 1```````` + 1```` + 1`` + 1`\r\n ```\r\n*
updated `count_distinct` agg definition to have the `precision`
second\r\noptional param.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Stratoula Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"cad276fcbd9fe5b13c18f08fd01bcf0c802b7ec9","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Visualizations","release_note:skip","backport:prev-minor","Feature:ES|QL","v8.13.0","v8.14.0"],"number":177783,"url":"#177783
Fix some validation misconfiguration (#177783)\n\n##
Summary\r\n\r\nRelated issue #177699\r\n\r\nFix variables logic for
expressions at `stats by ...` level.\r\nFix validation logic for agg
functions within `eval` or `where` scope.\r\nFix validation and
autocomplete logic for nested quoted expressions\r\n * i.e. \r\n
```\r\nfrom index | eval round(numberField) + 1 | eval
`round(numberField) + 1`\r\n+ 1 | eval ```round(numberField) + 1`` + 1`
+ 1 | eval\r\n```````round(numberField) + 1```` + 1`` + 1` + 1 |
eval\r\n```````````````round(numberField) + 1```````` + 1```` + 1`` + 1`
+ 1 |\r\nkeep ```````````````````````````````round(numberField)
+\r\n1```````````````` + 1```````` + 1```` + 1`` + 1`\r\n ```\r\n*
updated `count_distinct` agg definition to have the `precision`
second\r\noptional param.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Stratoula Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"cad276fcbd9fe5b13c18f08fd01bcf0c802b7ec9"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","labelRegex":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"#177783
Fix some validation misconfiguration (#177783)\n\n##
Summary\r\n\r\nRelated issue #177699\r\n\r\nFix variables logic for
expressions at `stats by ...` level.\r\nFix validation logic for agg
functions within `eval` or `where` scope.\r\nFix validation and
autocomplete logic for nested quoted expressions\r\n * i.e. \r\n
```\r\nfrom index | eval round(numberField) + 1 | eval
`round(numberField) + 1`\r\n+ 1 | eval ```round(numberField) + 1`` + 1`
+ 1 | eval\r\n```````round(numberField) + 1```` + 1`` + 1` + 1 |
eval\r\n```````````````round(numberField) + 1```````` + 1```` + 1`` + 1`
+ 1 |\r\nkeep ```````````````````````````````round(numberField)
+\r\n1```````````````` + 1```````` + 1```` + 1`` + 1`\r\n ```\r\n*
updated `count_distinct` agg definition to have the `precision`
second\r\noptional param.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Stratoula Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"cad276fcbd9fe5b13c18f08fd01bcf0c802b7ec9"}}]}]
BACKPORT-->

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@stratoula stratoula added Team:ESQL ES|QL related features in Kibana and removed Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Mar 27, 2024
@elasticmachine
Copy link
Contributor

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

@drewdaemon drewdaemon mentioned this issue Mar 29, 2024
Merged
1 task
drewdaemon added a commit that referenced this issue Apr 2, 2024
@drewdaemon @stratoula
[ES|QL] client-side validation: make and and or accept null (#1…
97e554c 
…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
Copy link
Contributor

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

@stratoula
Copy link
Contributor Author

Nice catch!

@dej611
Copy link
Contributor

dej611 commented Apr 16, 2024 •
edited

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
Copy link
Contributor Author

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
Copy link
Contributor

dej611 commented Apr 16, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

dej611 commented Apr 16, 2024

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.

@drewdaemon drewdaemon mentioned this issue May 8, 2024
Merged
1 task
drewdaemon added a commit that referenced this issue May 9, 2024
@drewdaemon
[ES|QL] implicit casting changes (#182989)
c3e1027 
## 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 #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 #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
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue May 9, 2024
@drewdaemon
[ES|QL] implicit casting changes (elastic#182989)
f7e61de 
## 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)
kibanamachine added a commit that referenced this issue May 9, 2024
@kibanamachine @drewdaemon
[8.14] [ES|QL] implicit casting changes (#182989) (#183011)
6a1a267 
# Backport

This will backport the following commits from `main` to `8.14`:
- [[ES|QL] implicit casting changes
(#182989)](#182989)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2024-05-09T05:51:18Z","message":"[ES|QL]
implicit casting changes (#182989)\n\n## Summary\r\n\r\nThis is a
follow-on
from\r\nhttps://github.com/elastic/elasticsearch/pull/107859.\r\n\r\nTwo
main changes to our client-side validation code.\r\n1. comparison
operators like `==` and `>=` now support implicit casting\r\nfrom
strings for `version`, `ip`, and `boolean` types\r\n\r\n2. `in` and `not
in` operators support implicit casting from strings for\r\n`version`,
`ip`, `boolean`, and `date` constants. To address this\r\nquickly, I
have disabled type-checking for array values (e.g. `in
(\r\nversion_field, \"1.2.3\", \"2.3.1\" )`) because our parameter
typing system\r\ndoes not currently support arrays of mixed types which
it will need to\r\nin order to re-enable checking while allowing string
literals. I have\r\nadded this to
#177699 A note on
testing\r\n\r\nThese implicit casting changes may not be on the latest
Elasticsearch\r\nsnapshot. Instead, check out the `8.14` branch in a
local Elasticsearch\r\nrepo and run `yarn es source
--source-path='path/to/elasticsearch'`\r\n\r\nSee
[these\r\ntests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763)\r\nfor
a set of good use cases to try. `to_<type>` functions can be used
if\r\nthe sample data doesn't contain a field of the type you want to
test.\r\n\r\n### A note on string to date casting\r\n\r\nIn
#182856 we started
accepting\r\nstring literals for any function arguments that are
dates.\r\n\r\nBy the same logic, `\"2022\" - 1 day` would work, so our
validator doesn't\r\ncomplain about it. However, it currently fails at
the Elasticsearch\r\nlevel.\r\n\r\nIn a discussion with Fang, we
determined that this is a\r\nvalid use case, so I have
created\r\nhttps://github.com/elastic/elasticsearch/issues/108432 and
determined\r\nnot to tighten the client-side validation since this seems
more like a\r\ncasting \"hole\" that will soon be filled in on the ES
side (though not in\r\n8.14).\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated
or","sha":"c3e1027b2d5da9361251e3af3304098717193ded","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","backport:prev-minor","Feature:ES|QL","v8.14.0","Team:ESQL","v8.15.0"],"title":"[ES|QL]
implicit casting
changes","number":182989,"url":"#182989
implicit casting changes (#182989)\n\n## Summary\r\n\r\nThis is a
follow-on
from\r\nhttps://github.com/elastic/elasticsearch/pull/107859.\r\n\r\nTwo
main changes to our client-side validation code.\r\n1. comparison
operators like `==` and `>=` now support implicit casting\r\nfrom
strings for `version`, `ip`, and `boolean` types\r\n\r\n2. `in` and `not
in` operators support implicit casting from strings for\r\n`version`,
`ip`, `boolean`, and `date` constants. To address this\r\nquickly, I
have disabled type-checking for array values (e.g. `in
(\r\nversion_field, \"1.2.3\", \"2.3.1\" )`) because our parameter
typing system\r\ndoes not currently support arrays of mixed types which
it will need to\r\nin order to re-enable checking while allowing string
literals. I have\r\nadded this to
#177699 A note on
testing\r\n\r\nThese implicit casting changes may not be on the latest
Elasticsearch\r\nsnapshot. Instead, check out the `8.14` branch in a
local Elasticsearch\r\nrepo and run `yarn es source
--source-path='path/to/elasticsearch'`\r\n\r\nSee
[these\r\ntests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763)\r\nfor
a set of good use cases to try. `to_<type>` functions can be used
if\r\nthe sample data doesn't contain a field of the type you want to
test.\r\n\r\n### A note on string to date casting\r\n\r\nIn
#182856 we started
accepting\r\nstring literals for any function arguments that are
dates.\r\n\r\nBy the same logic, `\"2022\" - 1 day` would work, so our
validator doesn't\r\ncomplain about it. However, it currently fails at
the Elasticsearch\r\nlevel.\r\n\r\nIn a discussion with Fang, we
determined that this is a\r\nvalid use case, so I have
created\r\nhttps://github.com/elastic/elasticsearch/issues/108432 and
determined\r\nnot to tighten the client-side validation since this seems
more like a\r\ncasting \"hole\" that will soon be filled in on the ES
side (though not in\r\n8.14).\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated
or","sha":"c3e1027b2d5da9361251e3af3304098717193ded"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#182989
implicit casting changes (#182989)\n\n## Summary\r\n\r\nThis is a
follow-on
from\r\nhttps://github.com/elastic/elasticsearch/pull/107859.\r\n\r\nTwo
main changes to our client-side validation code.\r\n1. comparison
operators like `==` and `>=` now support implicit casting\r\nfrom
strings for `version`, `ip`, and `boolean` types\r\n\r\n2. `in` and `not
in` operators support implicit casting from strings for\r\n`version`,
`ip`, `boolean`, and `date` constants. To address this\r\nquickly, I
have disabled type-checking for array values (e.g. `in
(\r\nversion_field, \"1.2.3\", \"2.3.1\" )`) because our parameter
typing system\r\ndoes not currently support arrays of mixed types which
it will need to\r\nin order to re-enable checking while allowing string
literals. I have\r\nadded this to
#177699 A note on
testing\r\n\r\nThese implicit casting changes may not be on the latest
Elasticsearch\r\nsnapshot. Instead, check out the `8.14` branch in a
local Elasticsearch\r\nrepo and run `yarn es source
--source-path='path/to/elasticsearch'`\r\n\r\nSee
[these\r\ntests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763)\r\nfor
a set of good use cases to try. `to_<type>` functions can be used
if\r\nthe sample data doesn't contain a field of the type you want to
test.\r\n\r\n### A note on string to date casting\r\n\r\nIn
#182856 we started
accepting\r\nstring literals for any function arguments that are
dates.\r\n\r\nBy the same logic, `\"2022\" - 1 day` would work, so our
validator doesn't\r\ncomplain about it. However, it currently fails at
the Elasticsearch\r\nlevel.\r\n\r\nIn a discussion with Fang, we
determined that this is a\r\nvalid use case, so I have
created\r\nhttps://github.com/elastic/elasticsearch/issues/108432 and
determined\r\nnot to tighten the client-side validation since this seems
more like a\r\ncasting \"hole\" that will soon be filled in on the ES
side (though not in\r\n8.14).\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or","sha":"c3e1027b2d5da9361251e3af3304098717193ded"}}]}]
BACKPORT-->

Co-authored-by: Drew Tate <drew.tate@elastic.co>
@sphilipse
Copy link
Member

Index patterns with a negative produce a syntax error, despite working perfectly:
Screenshot 2024-05-15 at 15 12 51

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

@drewdaemon
Copy link
Contributor

@sphilipse thanks for the report

@drewdaemon
Copy link
Contributor

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

@stratoula
Copy link
Contributor Author

We are tracking valid time units as wrong

CleanShot 2024-06-07 at 13 40 44@2x

@drewdaemon
Copy link
Contributor

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

@drewdaemon drewdaemon mentioned this issue Jun 10, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:ES|QL impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@drewdaemon @dej611 @elasticmachine @stratoula @sphilipse