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.

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

[ES|QL] create an expression type evaluator #195682

Closed
drewdaemon opened this issue Oct 9, 2024 · 1 comment · Fixed by #195149 or #195989
Closed

[ES|QL] create an expression type evaluator #195682

drewdaemon opened this issue Oct 9, 2024 · 1 comment · Fixed by #195149 or #195989
Assignees
Labels
Feature:ES|QL ES|QL related features in Kibana impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented Oct 9, 2024

The what

We should be able to detect the type of any expression in the ES|QL language. For examples

  • TRIM(TO_STRING(1 + 2)) -> keyword
  • 1 + 4 / 2. -> double
  • FLOOR(123::long) -> long

The why

Detecting the type of a variable

during validation and autocomplete. For example, we get this wrong all the time (see #192255 (comment)):

... | EVAL var0 = FLOOR(1 + 23) | var0 (var0 is an int, but we assume it is a double.)

Validation itself (bonus points)

The same tree traversal algorithm we will need to properly compute expression types can be used to validate them. Imagine something like the following:

const validationErrors = [];
getExpressionType(astNode, error => validationErrors.push(error));

The how

Basically, we need to whip the current code into shape and export it for use in the validation engine. This means creating a recursive algorithm (basically a small interpreter) to evaluate an expression (only difference from a real interpreter is that the result will be a type instead of a value).

@drewdaemon drewdaemon added Feature:ES|QL ES|QL related features in Kibana impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana labels Oct 9, 2024
@elasticmachine
Copy link
Contributor

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 11, 2024
## Summary

Closes elastic#191111
Closes elastic#191105

This change cleans up the way variables and fields were being stored and
checked against. We now populate the field and variable cache with
unescaped column names. This means that there are fewer cache misses
because of escaped names checked against unescaped/differently-escaped
names.

It also introduces a [suite of
tests](https://github.com/elastic/kibana/pull/195149/files#diff-6e4802e45f72257ca6f765bedd3ad65d2cbb587adb5befadb5f27ad5ab08a5a6R113)
for variable type detection. Most of those tests are currently skipped,
but they are there to represent what should happen when we resolve
elastic#195682

User-facing improvements
- Validation used to fail for field names with multiple escaped parts
(e.g. `geo`.`dest`)
- elastic#191105
- Variables assigned the result of an inline cast used to get the wrong
type.
- Escaped field/variable suggestions now work with field list
autocomplete

https://github.com/user-attachments/assets/2162f392-3ac3-4bb4-bf37-c73318c7f751

### Checklist

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [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 <[email protected]>
(cherry picked from commit 731c4e4)
@drewdaemon drewdaemon reopened this Oct 11, 2024
kibanamachine added a commit that referenced this issue Oct 11, 2024
…5936)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] Improve variable and field name handling
(#195149)](#195149)

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

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

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-11T14:33:04Z","message":"[ES|QL]
Improve variable and field name handling (#195149)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191111\r\nCloses
https://github.com/elastic/kibana/issues/191105\r\n\r\nThis change
cleans up the way variables and fields were being stored and\r\nchecked
against. We now populate the field and variable cache with\r\nunescaped
column names. This means that there are fewer cache misses\r\nbecause of
escaped names checked against
unescaped/differently-escaped\r\nnames.\r\n\r\nIt also introduces a
[suite
of\r\ntests](https://github.com/elastic/kibana/pull/195149/files#diff-6e4802e45f72257ca6f765bedd3ad65d2cbb587adb5befadb5f27ad5ab08a5a6R113)\r\nfor
variable type detection. Most of those tests are currently
skipped,\r\nbut they are there to represent what should happen when we
resolve\r\nhttps://github.com//issues/195682\r\n\r\nUser-facing
improvements\r\n- Validation used to fail for field names with multiple
escaped parts\r\n(e.g. `geo`.`dest`)\r\n-
https://github.com/elastic/kibana/issues/191105\r\n- Variables assigned
the result of an inline cast used to get the wrong\r\ntype.\r\n- Escaped
field/variable suggestions now work with field
list\r\nautocomplete\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/2162f392-3ac3-4bb4-bf37-c73318c7f751\r\n\r\n\r\n\r\n###
Checklist\r\n\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\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: Elastic Machine
<[email protected]>","sha":"731c4e4aabbacfdc36ae44bce3e10f438b55a4cd","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","backport:prev-minor","Feature:ES|QL","Team:ESQL"],"title":"[ES|QL]
Improve variable and field name
handling","number":195149,"url":"https://github.com/elastic/kibana/pull/195149","mergeCommit":{"message":"[ES|QL]
Improve variable and field name handling (#195149)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191111\r\nCloses
https://github.com/elastic/kibana/issues/191105\r\n\r\nThis change
cleans up the way variables and fields were being stored and\r\nchecked
against. We now populate the field and variable cache with\r\nunescaped
column names. This means that there are fewer cache misses\r\nbecause of
escaped names checked against
unescaped/differently-escaped\r\nnames.\r\n\r\nIt also introduces a
[suite
of\r\ntests](https://github.com/elastic/kibana/pull/195149/files#diff-6e4802e45f72257ca6f765bedd3ad65d2cbb587adb5befadb5f27ad5ab08a5a6R113)\r\nfor
variable type detection. Most of those tests are currently
skipped,\r\nbut they are there to represent what should happen when we
resolve\r\nhttps://github.com//issues/195682\r\n\r\nUser-facing
improvements\r\n- Validation used to fail for field names with multiple
escaped parts\r\n(e.g. `geo`.`dest`)\r\n-
https://github.com/elastic/kibana/issues/191105\r\n- Variables assigned
the result of an inline cast used to get the wrong\r\ntype.\r\n- Escaped
field/variable suggestions now work with field
list\r\nautocomplete\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/2162f392-3ac3-4bb4-bf37-c73318c7f751\r\n\r\n\r\n\r\n###
Checklist\r\n\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\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: Elastic Machine
<[email protected]>","sha":"731c4e4aabbacfdc36ae44bce3e10f438b55a4cd"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195149","number":195149,"mergeCommit":{"message":"[ES|QL]
Improve variable and field name handling (#195149)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191111\r\nCloses
https://github.com/elastic/kibana/issues/191105\r\n\r\nThis change
cleans up the way variables and fields were being stored and\r\nchecked
against. We now populate the field and variable cache with\r\nunescaped
column names. This means that there are fewer cache misses\r\nbecause of
escaped names checked against
unescaped/differently-escaped\r\nnames.\r\n\r\nIt also introduces a
[suite
of\r\ntests](https://github.com/elastic/kibana/pull/195149/files#diff-6e4802e45f72257ca6f765bedd3ad65d2cbb587adb5befadb5f27ad5ab08a5a6R113)\r\nfor
variable type detection. Most of those tests are currently
skipped,\r\nbut they are there to represent what should happen when we
resolve\r\nhttps://github.com//issues/195682\r\n\r\nUser-facing
improvements\r\n- Validation used to fail for field names with multiple
escaped parts\r\n(e.g. `geo`.`dest`)\r\n-
https://github.com/elastic/kibana/issues/191105\r\n- Variables assigned
the result of an inline cast used to get the wrong\r\ntype.\r\n- Escaped
field/variable suggestions now work with field
list\r\nautocomplete\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/2162f392-3ac3-4bb4-bf37-c73318c7f751\r\n\r\n\r\n\r\n###
Checklist\r\n\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\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: Elastic Machine
<[email protected]>","sha":"731c4e4aabbacfdc36ae44bce3e10f438b55a4cd"}}]}]
BACKPORT-->

Co-authored-by: Drew Tate <[email protected]>
@qn895 qn895 closed this as completed in 2173af7 Oct 18, 2024
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 18, 2024
## Summary

Close elastic#195682
Close elastic#195430

Introduces `getExpressionType`, the ES|QL expression type evaluator to
rule them all!

Also, fixes several validation bugs related to the faulty logic that
existed before with variable type detection (some noted in
elastic#192255 (comment)).

### 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 <[email protected]>
(cherry picked from commit 2173af7)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 18, 2024
## Summary

Close elastic#195682
Close elastic#195430

Introduces `getExpressionType`, the ES|QL expression type evaluator to
rule them all!

Also, fixes several validation bugs related to the faulty logic that
existed before with variable type detection (some noted in
elastic#192255 (comment)).

### 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 <[email protected]>
(cherry picked from commit 2173af7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ES|QL ES|QL related features in Kibana impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana
Projects
None yet
2 participants