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] Separate ENRICH autocomplete routine #211657

Merged
merged 21 commits into from
Mar 3, 2025
Merged

[ES|QL] Separate ENRICH autocomplete routine #211657

merged 21 commits into from
Mar 3, 2025

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Feb 18, 2025 •
edited by kibanamachine
Loading

Summary

Part of #195418

Gives ENRICH autocomplete logic its own home 🏡

Checklist

Identify risks

  • As with any refactor, there's a possibility this will introduce a regression in the behavior of commands. However, all automated tests are passing and I have tested the behavior manually and can detect no regression.

@drewdaemon drewdaemon added release_note:skip Skip the PR/issue when compiling release notes Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Feb 18, 2025
@drewdaemon drewdaemon mentioned this pull request Feb 12, 2025
Open
15 tasks
@drewdaemon drewdaemon marked this pull request as ready for review February 28, 2025 20:08
@drewdaemon drewdaemon requested a review from a team as a code owner February 28, 2025 20:08
@elasticmachine
Copy link
Contributor

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

stratoula
stratoula approved these changes Mar 3, 2025
View reviewed changes
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.

Seems great, I also did a brief testing to ensure that it works as expected!

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts
Comment on lines 181 to +184
(astContext.type === 'option' && astContext.command?.name === 'join') ||
(astContext.type === 'option' && astContext.command?.name === 'dissect') ||
(astContext.type === 'option' && astContext.command?.name === 'from')
(astContext.type === 'option' && astContext.command?.name === 'from') ||
(astContext.type === 'option' && astContext.command?.name === 'enrich')
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this is temporary but maybe it makes sense for now to create an array with the converted commands and have one check here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I can do this!

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/factories.ts
command: TRIGGER_SUGGESTION_COMMAND,
}));

/** @deprecated — options will be removed */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why will we remove options? 🤔

Copy link
Contributor Author

@drewdaemon drewdaemon Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So... what is an "option?"

It is a construct in our code with no grounding anywhere in the grammar or in Elasticsearch (that I know of anyway...). It essentially means any capitalized keyword after the command name. For example STATS... >BY<, DISSECT... >APPEND_SEPARATOR<

There's this idea in the current architecture that these are uniform enough to deserve to be their own special first-class citizen. But it breaks...

For example APPEND_SEPARATOR is not a keyword with an expression after it... it is a variable APPEND_SEPARATOR=":"... or filtering in stats.... STATS AVG(bytes) >WHERE< .... so is WHERE an option now?

Don't even start on FORK...

So basically, I don't think it deserves to be treated as anything other than part of a command. That's why I'm gradually gutting the getSuggestionsForOption function... it will all happen within the command.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaaaah it makes sense +++ Thanx for explaining!

@drewdaemon drewdaemon enabled auto-merge (squash) March 3, 2025 20:07
@drewdaemon drewdaemon merged commit f2a9173 into elastic:main Mar 3, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
esql 206 208 +2
lists 437 439 +2
securitySolution 7106 7108 +2
unifiedSearch 394 396 +2
total +8

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.6MB 3.6MB +1.1KB
Unknown metric groups

References to deprecated APIs

id before after diff
@kbn/monaco 1 3 +2

History

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 3, 2025
@drewdaemon
[ES|QL] Separate ENRICH autocomplete routine (elastic#211657)
cd2d6ca 
## Summary

Part of elastic#195418

Gives `ENRICH` autocomplete logic its own home 🏡

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

### Identify risks

- [ ] As with any refactor, there's a possibility this will introduce a
regression in the behavior of commands. However, all automated tests are
passing and I have tested the behavior manually and can detect no
regression.

---------

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
(cherry picked from commit f2a9173)
@kibanamachine kibanamachine mentioned this pull request Mar 3, 2025
Merged
@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 Mar 3, 2025
@kibanamachine @drewdaemon
[8.x] [ES|QL] Separate ENRICH autocomplete routine (#211657) (#212979)
088e356 
# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] Separate `ENRICH` autocomplete routine
(#211657)](#211657)

<!--- Backport version: 9.6.6 -->

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

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2025-03-03T20:24:23Z","message":"[ES|QL]
Separate `ENRICH` autocomplete routine (#211657)\n\n## Summary\n\nPart
of https://github.com/elastic/kibana/issues/195418\n\nGives `ENRICH`
autocomplete logic its own home 🏡\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n\n### Identify
risks\n\n- [ ] As with any refactor, there's a possibility this will
introduce a\nregression in the behavior of commands. However, all
automated tests are\npassing and I have tested the behavior manually and
can detect no\nregression.\n\n---------\n\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"f2a91732d8f8d20a22bf761bfe9ec85e8a8e1c0c","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Feature:ES|QL","Team:ESQL","backport:version","v9.1.0","v8.19.0"],"title":"[ES|QL]
Separate `ENRICH` autocomplete
routine","number":211657,"url":"https://github.com/elastic/kibana/pull/211657","mergeCommit":{"message":"[ES|QL]
Separate `ENRICH` autocomplete routine (#211657)\n\n## Summary\n\nPart
of https://github.com/elastic/kibana/issues/195418\n\nGives `ENRICH`
autocomplete logic its own home 🏡\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n\n### Identify
risks\n\n- [ ] As with any refactor, there's a possibility this will
introduce a\nregression in the behavior of commands. However, all
automated tests are\npassing and I have tested the behavior manually and
can detect no\nregression.\n\n---------\n\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"f2a91732d8f8d20a22bf761bfe9ec85e8a8e1c0c"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/211657","number":211657,"mergeCommit":{"message":"[ES|QL]
Separate `ENRICH` autocomplete routine (#211657)\n\n## Summary\n\nPart
of https://github.com/elastic/kibana/issues/195418\n\nGives `ENRICH`
autocomplete logic its own home 🏡\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n\n### Identify
risks\n\n- [ ] As with any refactor, there's a possibility this will
introduce a\nregression in the behavior of commands. However, all
automated tests are\npassing and I have tested the behavior manually and
can detect no\nregression.\n\n---------\n\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"f2a91732d8f8d20a22bf761bfe9ec85e8a8e1c0c"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Drew Tate <drew.tate@elastic.co>
SoniaSanzV pushed a commit to SoniaSanzV/kibana that referenced this pull request Mar 4, 2025
@kibanamachine @drewdaemon @SoniaSanzV
[8.x] [ES|QL] Separate ENRICH autocomplete routine (elastic#211657) (…
d7dc5b1 
…elastic#212979)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] Separate `ENRICH` autocomplete routine
(elastic#211657)](elastic#211657)

<!--- Backport version: 9.6.6 -->

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

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2025-03-03T20:24:23Z","message":"[ES|QL]
Separate `ENRICH` autocomplete routine (elastic#211657)\n\n## Summary\n\nPart
of https://github.com/elastic/kibana/issues/195418\n\nGives `ENRICH`
autocomplete logic its own home 🏡\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n\n### Identify
risks\n\n- [ ] As with any refactor, there's a possibility this will
introduce a\nregression in the behavior of commands. However, all
automated tests are\npassing and I have tested the behavior manually and
can detect no\nregression.\n\n---------\n\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"f2a91732d8f8d20a22bf761bfe9ec85e8a8e1c0c","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Feature:ES|QL","Team:ESQL","backport:version","v9.1.0","v8.19.0"],"title":"[ES|QL]
Separate `ENRICH` autocomplete
routine","number":211657,"url":"https://github.com/elastic/kibana/pull/211657","mergeCommit":{"message":"[ES|QL]
Separate `ENRICH` autocomplete routine (elastic#211657)\n\n## Summary\n\nPart
of https://github.com/elastic/kibana/issues/195418\n\nGives `ENRICH`
autocomplete logic its own home 🏡\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n\n### Identify
risks\n\n- [ ] As with any refactor, there's a possibility this will
introduce a\nregression in the behavior of commands. However, all
automated tests are\npassing and I have tested the behavior manually and
can detect no\nregression.\n\n---------\n\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"f2a91732d8f8d20a22bf761bfe9ec85e8a8e1c0c"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/211657","number":211657,"mergeCommit":{"message":"[ES|QL]
Separate `ENRICH` autocomplete routine (elastic#211657)\n\n## Summary\n\nPart
of https://github.com/elastic/kibana/issues/195418\n\nGives `ENRICH`
autocomplete logic its own home 🏡\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n\n### Identify
risks\n\n- [ ] As with any refactor, there's a possibility this will
introduce a\nregression in the behavior of commands. However, all
automated tests are\npassing and I have tested the behavior manually and
can detect no\nregression.\n\n---------\n\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"f2a91732d8f8d20a22bf761bfe9ec85e8a8e1c0c"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Drew Tate <drew.tate@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

Assignees
No one assigned
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.19.0 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@drewdaemon @elasticmachine @kibanamachine @stratoula