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] separate WHERE autocomplete routine #198832

Merged

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Nov 4, 2024

Summary

Part of #195418

NOTES

Checklist

...buildConstantsDefinitions(['true', 'false']),
];
}
return [];
};

export const getBuiltinCompatibleFunctionDefinition = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with getOperatorSuggestions

@@ -860,7 +864,8 @@ export function getExpressionType(
const param = getParamAtPosition(signature, i);
return (
param &&
(param.type === argType ||
(param.type === 'any' ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a small bug-fix. Soon, probably in #188978, we'll abandon any in operator and function definitions, but it's there for now.

@drewdaemon drewdaemon added Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana v9.0.0 v8.17.0 release_note:skip Skip the PR/issue when compiling release notes labels Nov 8, 2024
Comment on lines -538 to -548
await assertSuggestions('from a | eval a = 1 day + 2 /', [',', '| ']);
await assertSuggestions(
'from a | eval 1 day + 2 /',
[
...dateSuggestions,
...getFunctionSignaturesByReturnType('eval', 'any', { builtin: true, skipAssign: true }, [
'integer',
]),
],
{ triggerCharacter: ' ' }
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1 day + 2 is an invalid expression

@stratoula stratoula marked this pull request as ready for review November 8, 2024 13:39
@stratoula stratoula requested a review from a team as a code owner November 8, 2024 13:39
@elasticmachine
Copy link
Contributor

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

@stratoula stratoula added the backport:version Backport to applied version labels label Nov 8, 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.

Looks great and I can't find any regression. I am merging this.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 6131 6133 +2
unifiedSearch 371 373 +2
total +4

Async chunks

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

id before after diff
securitySolution 13.4MB 13.4MB +74.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 +1.1KB

History

@stratoula stratoula merged commit 2466a17 into elastic:main Nov 11, 2024
21 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 11, 2024
## Summary

Part of elastic#195418

**NOTES**
- need to make sure these don't regress
  - elastic#195771
  - elastic#197139
  - suggesting variables after binary operator (e.g. `field + <suggest>`
- I've noticed that incomplete null statements such as `is n` are
corrected in our syntax tree. This sends the autocomplete down a
"completed operator expression" route as opposed to an unknown operator
or "to right of column" route. So, `... | EVAL foo IS N/` is interpreted
as `... | EVAL foo IS NULL /`.<br><br>It accidentally works (lol)
because the logic for `<operator-expression> <suggest>` suggests
operators that accept the return type of the existing operator
expression as their left-hand argument. Since `foo IS NULL` is of type
`boolean` and `IS NULL` accepts boolean values, it gets included in the
suggestion list which Monaco then filters by the actual prefix (`IS N`).
🤣 <br><br>([issue](elastic#199401))

### 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 <[email protected]>
(cherry picked from commit 2466a17)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
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 Nov 11, 2024
… (#199595)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] separate &#x60;WHERE&#x60; autocomplete routine
(#198832)](#198832)

<!--- 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-11-11T08:41:38Z","message":"[ES|QL]
separate `WHERE` autocomplete routine (#198832)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/195418\r\n\r\n**NOTES**\r\n-
need to make sure these don't regress\r\n -
https://github.com/elastic/kibana/pull/195771\r\n -
https://github.com/elastic/kibana/pull/197139\r\n - suggesting variables
after binary operator (e.g. `field + <suggest>`\r\n- I've noticed that
incomplete null statements such as `is n` are\r\ncorrected in our syntax
tree. This sends the autocomplete down a\r\n\"completed operator
expression\" route as opposed to an unknown operator\r\nor \"to right of
column\" route. So, `... | EVAL foo IS N/` is interpreted\r\nas `... |
EVAL foo IS NULL /`.<br><br>It accidentally works (lol)\r\nbecause the
logic for `<operator-expression> <suggest>` suggests\r\noperators that
accept the return type of the existing operator\r\nexpression as their
left-hand argument. Since `foo IS NULL` is of type\r\n`boolean` and `IS
NULL` accepts boolean values, it gets included in the\r\nsuggestion list
which Monaco then filters by the actual prefix (`IS N`).\r\n🤣
<br><br>([issue](https://github.com/elastic/kibana/issues/199401))\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
<[email protected]>","sha":"2466a172af66452bdd939dddc56506faba7ffb7a","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Feature:ES|QL","Team:ESQL","backport:version","v8.17.0"],"title":"[ES|QL]
separate `WHERE` autocomplete
routine","number":198832,"url":"https://github.com/elastic/kibana/pull/198832","mergeCommit":{"message":"[ES|QL]
separate `WHERE` autocomplete routine (#198832)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/195418\r\n\r\n**NOTES**\r\n-
need to make sure these don't regress\r\n -
https://github.com/elastic/kibana/pull/195771\r\n -
https://github.com/elastic/kibana/pull/197139\r\n - suggesting variables
after binary operator (e.g. `field + <suggest>`\r\n- I've noticed that
incomplete null statements such as `is n` are\r\ncorrected in our syntax
tree. This sends the autocomplete down a\r\n\"completed operator
expression\" route as opposed to an unknown operator\r\nor \"to right of
column\" route. So, `... | EVAL foo IS N/` is interpreted\r\nas `... |
EVAL foo IS NULL /`.<br><br>It accidentally works (lol)\r\nbecause the
logic for `<operator-expression> <suggest>` suggests\r\noperators that
accept the return type of the existing operator\r\nexpression as their
left-hand argument. Since `foo IS NULL` is of type\r\n`boolean` and `IS
NULL` accepts boolean values, it gets included in the\r\nsuggestion list
which Monaco then filters by the actual prefix (`IS N`).\r\n🤣
<br><br>([issue](https://github.com/elastic/kibana/issues/199401))\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
<[email protected]>","sha":"2466a172af66452bdd939dddc56506faba7ffb7a"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198832","number":198832,"mergeCommit":{"message":"[ES|QL]
separate `WHERE` autocomplete routine (#198832)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/195418\r\n\r\n**NOTES**\r\n-
need to make sure these don't regress\r\n -
https://github.com/elastic/kibana/pull/195771\r\n -
https://github.com/elastic/kibana/pull/197139\r\n - suggesting variables
after binary operator (e.g. `field + <suggest>`\r\n- I've noticed that
incomplete null statements such as `is n` are\r\ncorrected in our syntax
tree. This sends the autocomplete down a\r\n\"completed operator
expression\" route as opposed to an unknown operator\r\nor \"to right of
column\" route. So, `... | EVAL foo IS N/` is interpreted\r\nas `... |
EVAL foo IS NULL /`.<br><br>It accidentally works (lol)\r\nbecause the
logic for `<operator-expression> <suggest>` suggests\r\noperators that
accept the return type of the existing operator\r\nexpression as their
left-hand argument. Since `foo IS NULL` is of type\r\n`boolean` and `IS
NULL` accepts boolean values, it gets included in the\r\nsuggestion list
which Monaco then filters by the actual prefix (`IS N`).\r\n🤣
<br><br>([issue](https://github.com/elastic/kibana/issues/199401))\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
<[email protected]>","sha":"2466a172af66452bdd939dddc56506faba7ffb7a"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Drew Tate <[email protected]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Nov 11, 2024
## Summary

Part of elastic#195418

**NOTES**
- need to make sure these don't regress
  - elastic#195771
  - elastic#197139
  - suggesting variables after binary operator (e.g. `field + <suggest>`
- I've noticed that incomplete null statements such as `is n` are
corrected in our syntax tree. This sends the autocomplete down a
"completed operator expression" route as opposed to an unknown operator
or "to right of column" route. So, `... | EVAL foo IS N/` is interpreted
as `... | EVAL foo IS NULL /`.<br><br>It accidentally works (lol)
because the logic for `<operator-expression> <suggest>` suggests
operators that accept the return type of the existing operator
expression as their left-hand argument. Since `foo IS NULL` is of type
`boolean` and `IS NULL` accepts boolean values, it gets included in the
suggestion list which Monaco then filters by the actual prefix (`IS N`).
🤣 <br><br>([issue](elastic#199401))

### 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 <[email protected]>
tkajtoch pushed a commit to tkajtoch/kibana that referenced this pull request Nov 12, 2024
## Summary

Part of elastic#195418

**NOTES**
- need to make sure these don't regress
  - elastic#195771
  - elastic#197139
  - suggesting variables after binary operator (e.g. `field + <suggest>`
- I've noticed that incomplete null statements such as `is n` are
corrected in our syntax tree. This sends the autocomplete down a
"completed operator expression" route as opposed to an unknown operator
or "to right of column" route. So, `... | EVAL foo IS N/` is interpreted
as `... | EVAL foo IS NULL /`.<br><br>It accidentally works (lol)
because the logic for `<operator-expression> <suggest>` suggests
operators that accept the return type of the existing operator
expression as their left-hand argument. Since `foo IS NULL` is of type
`boolean` and `IS NULL` accepts boolean values, it gets included in the
suggestion list which Monaco then filters by the actual prefix (`IS N`).
🤣 <br><br>([issue](elastic#199401))

### 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 <[email protected]>
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:skip Skip the PR/issue when compiling release notes Team:ESQL ES|QL related features in Kibana v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants