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 expression type evaluator #195989

Merged
merged 18 commits into from
Oct 18, 2024

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Oct 11, 2024

Summary

Close #195682
Close #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 #192255 (comment)).

Checklist

@drewdaemon drewdaemon added release_note:fix backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana labels Oct 16, 2024
@qn895
Copy link
Member

qn895 commented Oct 17, 2024

Confirmed this PR also fixes the wrapped boolean expression are not always handled with their final type (part of #196618) 🥳

@qn895 qn895 marked this pull request as ready for review October 17, 2024 21:10
@qn895 qn895 requested a review from a team as a code owner October 17, 2024 21:10
@elasticmachine
Copy link
Contributor

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

@qn895 qn895 changed the title [ES|QL] create expression type evaluator [ES|QL] Create expression type evaluator Oct 17, 2024
@stratoula
Copy link
Contributor

@drewdaemon I assumed you want to backport also in 8.16 so I will update the backport labels. Let me know if I assumed wrongly

@stratoula stratoula added v9.0.0 v8.16.0 and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 18, 2024
@stratoula stratoula added v8.17.0 backport:version Backport to applied version labels labels Oct 18, 2024
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice fix, and makes the code more readable imho. I tested it quite thorougly I think. I cant find any regression so from me we are good to go. Nice job Drew.

Copy link
Member

@qn895 qn895 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also tested and LGTM 🎉

@qn895
Copy link
Member

qn895 commented Oct 18, 2024

@elasticmachine merge upstream

@qn895 qn895 enabled auto-merge (squash) October 18, 2024 14:42
@qn895 qn895 merged commit 2173af7 into elastic:main Oct 18, 2024
21 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11407243987

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 20.7MB 20.7MB -505.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kbnUiSharedDeps-srcJs 3.4MB 3.4MB -295.0B

History

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request 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 pull request 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
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.16
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 18, 2024
# Backport

This will backport the following commits from `main` to `8.16`:
- [[ES|QL] Create expression type evaluator
(#195989)](#195989)

<!--- 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-18T16:15:11Z","message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\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 added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","Feature:ES|QL","Team:ESQL","v8.16.0","backport:version","v8.17.0"],"title":"[ES|QL]
Create expression type
evaluator","number":195989,"url":"https://github.com/elastic/kibana/pull/195989","mergeCommit":{"message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\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 added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195989","number":195989,"mergeCommit":{"message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\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 added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Drew Tate <[email protected]>
kibanamachine added a commit that referenced this pull request Oct 18, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] Create expression type evaluator
(#195989)](#195989)

<!--- 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-18T16:15:11Z","message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\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 added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","Feature:ES|QL","Team:ESQL","v8.16.0","backport:version","v8.17.0"],"title":"[ES|QL]
Create expression type
evaluator","number":195989,"url":"https://github.com/elastic/kibana/pull/195989","mergeCommit":{"message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\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 added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195989","number":195989,"mergeCommit":{"message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\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 added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Drew Tate <[email protected]>
drewdaemon added a commit that referenced this pull request Oct 23, 2024
## Summary

Follow-up from #195989.

We discussed as a team and decided to show validation errors when an
unknown variable is used as an argument to subsequent functions.

**Before**
<img width="589" alt="Screenshot 2024-10-22 at 1 41 08 PM"
src="https://github.com/user-attachments/assets/872d3302-ddfe-415f-9c98-e2c682344189">


**After**
<img width="570" alt="Screenshot 2024-10-22 at 1 41 29 PM"
src="https://github.com/user-attachments/assets/b7e29c2d-ee40-4730-b1ab-43d95dfd264c">
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 23, 2024
## Summary

Follow-up from elastic#195989.

We discussed as a team and decided to show validation errors when an
unknown variable is used as an argument to subsequent functions.

**Before**
<img width="589" alt="Screenshot 2024-10-22 at 1 41 08 PM"
src="https://github.com/user-attachments/assets/872d3302-ddfe-415f-9c98-e2c682344189">

**After**
<img width="570" alt="Screenshot 2024-10-22 at 1 41 29 PM"
src="https://github.com/user-attachments/assets/b7e29c2d-ee40-4730-b1ab-43d95dfd264c">

(cherry picked from commit e1c0cef)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 23, 2024
## Summary

Follow-up from elastic#195989.

We discussed as a team and decided to show validation errors when an
unknown variable is used as an argument to subsequent functions.

**Before**
<img width="589" alt="Screenshot 2024-10-22 at 1 41 08 PM"
src="https://github.com/user-attachments/assets/872d3302-ddfe-415f-9c98-e2c682344189">

**After**
<img width="570" alt="Screenshot 2024-10-22 at 1 41 29 PM"
src="https://github.com/user-attachments/assets/b7e29c2d-ee40-4730-b1ab-43d95dfd264c">

(cherry picked from commit e1c0cef)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels Feature:ES|QL ES|QL related features in Kibana release_note:fix Team:ESQL ES|QL related features in Kibana v8.16.0 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ES|QL] create an expression type evaluator [ES|QL] simplify literal handling
5 participants