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] Add support for command settings #175114

Merged
merged 11 commits into from Jan 23, 2024
Merged

[ES|QL] Add support for command settings #175114

merged 11 commits into from Jan 23, 2024

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jan 18, 2024

Summary

Sync up with elastic/elasticsearch#103949 grammar changes.

Command definitions now have a new mode list of argument, with related ESQLCommandMode at AST level.

Validation now accepts settings and validate the specific ENRICH one (error message is taken from the ES codebase).
Autocomplete doesn't trigger by default on settings, but it only trigger when user starts to type a setting with the [ trigger char:

enrich_modes

Note that multiple settings are supported, but shadowing will trigger a warning.

ccq.mode value name and descriptions have been taken from the linked ES PR.

Checklist

@dej611 dej611 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:ES|QL v8.13.0 labels Jan 18, 2024
@dej611
Copy link
Contributor Author

dej611 commented Jan 22, 2024

/ci

@dej611
Copy link
Contributor Author

dej611 commented Jan 22, 2024

/ci

@dej611 dej611 marked this pull request as ready for review January 23, 2024 09:03
@dej611 dej611 requested review from a team as code owners January 23, 2024 09:03
@elasticmachine
Copy link
Contributor

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

@stratoula
Copy link
Contributor

/ci

stratoula
stratoula approved these changes Jan 23, 2024
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.

This is great, tested it locally and works fine. It would be awesome if we could give some love to our script with some comments/documentation. I am approving because except from that everything works great 👏

packages/kbn-monaco/scripts/esql_update_ast_script.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a readme here about what this script does, how someone can run it etc? I am also happy if the readme is part of the file itself!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will add some more context there.
In theory the script should run automatically when the grammar is compiled, together with some existing file renaming script. So no "independent" run should make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that! Some context doesnt hurt though! No many details, just an explanation about what it does and why we are using it. It can be helpful for future reference.

@dej611
Copy link
Contributor Author

dej611 commented Jan 23, 2024

/ci

vadimkibana
vadimkibana approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@vadimkibana vadimkibana left a comment

Choose a reason for hiding this comment

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

SharedUX code changes LGTM.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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 2.3MB 2.3MB +3.6KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dej611 dej611 merged commit 9e2caed into elastic:main Jan 23, 2024
17 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 23, 2024
lcawl pushed a commit to lcawl/kibana that referenced this pull request Jan 26, 2024
@dej611 @stratoula @lcawl
[ES|QL] Add support for command settings (elastic#175114)
09ba811 
## Summary

Sync up with elastic/elasticsearch#103949
grammar changes.

Command definitions now have a new `mode` list of argument, with related
`ESQLCommandMode` at AST level.

Validation now accepts settings and validate the specific ENRICH one
(error message is taken from the ES codebase).
Autocomplete doesn't trigger by default on settings, but it only trigger
when user starts to type a setting with the `[` trigger char:


![enrich_modes](https://github.com/elastic/kibana/assets/924948/8882361d-2fc7-44ab-bc8e-3994fc3e733d)

Note that multiple settings are supported, but shadowing will trigger a
warning.

`ccq.mode` value name and descriptions have been taken from the linked
ES PR.

### 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: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@dej611 @stratoula @CoenWarmer
[ES|QL] Add support for command settings (elastic#175114)
647c319 
## Summary

Sync up with elastic/elasticsearch#103949
grammar changes.

Command definitions now have a new `mode` list of argument, with related
`ESQLCommandMode` at AST level.

Validation now accepts settings and validate the specific ENRICH one
(error message is taken from the ES codebase).
Autocomplete doesn't trigger by default on settings, but it only trigger
when user starts to type a setting with the `[` trigger char:


![enrich_modes](https://github.com/elastic/kibana/assets/924948/8882361d-2fc7-44ab-bc8e-3994fc3e733d)

Note that multiple settings are supported, but shadowing will trigger a
warning.

`ccq.mode` value name and descriptions have been taken from the linked
ES PR.

### 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: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@dej611 @stratoula @CoenWarmer
[ES|QL] Add support for command settings (elastic#175114)
4582e34 
## Summary

Sync up with elastic/elasticsearch#103949
grammar changes.

Command definitions now have a new `mode` list of argument, with related
`ESQLCommandMode` at AST level.

Validation now accepts settings and validate the specific ENRICH one
(error message is taken from the ES codebase).
Autocomplete doesn't trigger by default on settings, but it only trigger
when user starts to type a setting with the `[` trigger char:


![enrich_modes](https://github.com/elastic/kibana/assets/924948/8882361d-2fc7-44ab-bc8e-3994fc3e733d)

Note that multiple settings are supported, but shadowing will trigger a
warning.

`ccq.mode` value name and descriptions have been taken from the linked
ES PR.

### 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: Stratoula Kalafateli <efstratia.kalafateli@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

@vadimkibana vadimkibana vadimkibana approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:ES|QL release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dej611 @elasticmachine @stratoula @kibana-ci @vadimkibana @kibanamachine