Skip to content

[ESQL] New and improved autosuggestion#171664

Merged
dej611 merged 26 commits intoelastic:feature/esql-validationfrom
dej611:fix/esql-autocomplete-ast
Nov 24, 2023
Merged

[ESQL] New and improved autosuggestion#171664
dej611 merged 26 commits intoelastic:feature/esql-validationfrom
dej611:fix/esql-autocomplete-ast

Conversation

@dej611
Copy link
Contributor

@dej611 dej611 commented Nov 21, 2023

Summary

esql_autosuggest_1

With this PR the autocomplete feature for ESQL is set at the same/better level than the previous one in main.
In particular:

  • New "shared" grammar should be supported for autosuggest
  • the suggestion logic is now based on the AST data structure and the logic is mostly generic*
  • the suggestion logic is now aware of types when proposing fields, functions and variables
  • the suggestion logic has automatic re-suggest on specific types (i.e. + or other builtin functions)
  • previous tests have been mainly retained to verify feature-parity and in most part extended**
  • some suggestions are more location-aware than others, and can automatically inject , together with the selected argument
  • new variables are automatically injected with = where it make sense

File hierarchy has changed as well in this PR to accomodate better modularization for different services: ast, validation, autocomplete, hover and signature.

Also various bug fixes have been provided for existing features/services:

  • rename AST walker had some bugs for partial/incomplete commands
  • rename validation had some bugs due to the bug above
  • ON option for ENRICH was set as mandatory, now it's set to optional
  • ENRICH options had several bugs on location extents computation. This was due to the specific grammar definition that didn't make it automatic as other commands to define correct location. It should be fixed now.
  • some AST functions/options contained empty values which were confusing for services built upon it, and all occurency of this type of bug has been fixed
  • fixed various function/command definitions

* there are still some edge cases that were required to be handled specifically. But most of the times the logic goes thru the definition data structure.
** some tests - i.e. suggest open brackets - have been removed as now functions are proposed entirely. For the "closing brackets" suggestion the editor is now configured to automatically open/close them.

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added enhancement New value added to drive a business result Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:ES|QL ES|QL related features in Kibana v8.12.0 release_note:feature Makes this part of the condensed release notes and removed enhancement New value added to drive a business result labels Nov 21, 2023
@dej611 dej611 marked this pull request as ready for review November 22, 2023 10:39
@dej611 dej611 requested a review from a team as a code owner November 22, 2023 10:39
@elasticmachine
Copy link
Contributor

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

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.

@dej611 this looks fantastic. I found some cases where autocomplete can be improved:

  1. On stats after selecting comma, comma is again suggested instead of a new variable
image
  1. The fourth option here is wrong. (We have a bug in Discover table and list and for some reason we display the column with underscores while it is not correct but I will create an issue). Can we not suggest it? Also the third is correct but unfortunately should be like avg(bytes) to work properly. Can we add the `` when the user clicks it?
image
  1. Not autocomplete related but this is red while it should not. This is a valid syntax
image
  1. Eval function arguments suggestions are wrong
image
  1. Nothing is suggested here :(
image

@stratoula
Copy link
Contributor

@dej611 we have this problem also in main but it is more prominent here. It seems that we error out on the capitalized version of the BY operator.

image

Also not related to autocomplete. We can fix on a separate PR.

@ninoslavmiskovic
Copy link
Contributor

@dej611 this is great work ! Huge improvement! So nice to e.g get suggestions on index nr 2,3,4 and the meta field .

@dej611
Copy link
Contributor Author

dej611 commented Nov 24, 2023

@dej611 we have this problem also in main but it is more prominent here. It seems that we error out on the capitalized version of the BY operator.

Fixed by b07f157 and 819e40c (added unit test as well)

On stats after selecting comma, comma is again suggested instead of a new variable

Fixed by 985486f (added unit test as well)

Eval function arguments suggestions are wrong

Fixed by e9a514d (added unit test that checks no consecutive fn suggested)

Solving this issue avoiding duplicates has a side effect the fix also for the tooltip scroll and now the scroll is moved on top to fields again.

Nothing is suggested here :(

Fixed by 2816c95 (added unit tests as well)

Not autocomplete related but this is red while it should not. This is a valid syntax

Fixed by 304621d (added unit tests as well)

The fourth option here is wrong. (We have a bug in Discover table and list and for some reason we display the column with underscores while it is not correct but I will create an issue). Can we not suggest it?

Fixed by d448041 (added unit tests as well)

Also the third is correct but unfortunately should be like avg(bytes) to work properly. Can we add the `` when the user clicks it?

Fixed by 43d8d84 (added unit tests as well)

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.

Supercalifragilisticexpialidocious! Bugs fixed, changes LGTM!

@kibana-ci
Copy link

💚 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
textBasedLanguages 150.9KB 151.0KB +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 2.3MB 2.7MB ⚠️ +379.0KB

History

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

@dej611 dej611 merged commit bac92d3 into elastic:feature/esql-validation Nov 24, 2023
dej611 added a commit that referenced this pull request Dec 7, 2023
## Summary

Fixes #166242 , #166876, #166173
, part of #166092, #166084

List of tasks:

* [x] AST work ( #166185 )
* [x] Basic validate work ( #166185 )
* [x] Hover feature ( #166185 )
* [x] Initial autocomplete work with new AST ( #166185 )
* Complete validation feature for MVP
  * [x] wildcards support ( #170014 )
  * [x] remote index validation support ( #171996 )
  * [x] wildcard support as `count` argument  ( #172054 )
  * [x] Aggressive caching for field queries:
* cache as much as possible the `FROM` queries - possible clear the
cache every 10/15 minutes?
    * do not fire a query when code == submitted code
* cache as much as possible the custom `FROM` built from `ENRICH`
policies - same clear policy as above
  * [x] Add unsupported fields warning messages
  * [x] Notify usage of `project` command with deprecation `warning`
* Complete autocomplete work ( #171664 )
  * [x] `stats`
  * [x] `where`
  * [x] `eval`
    * `math syntax`
  * [x] Aggressive cache for fields queries? ( #171866 )
  * [x] Fix when cursor is not at the end position ( #172060 ) 
* [x] Revisit copy messages
  * Label Kibana-only messages
* [x] Extend hover feature ( #171940 )
* [x] Disable editor query highlight for warnings ( #171968 )
* Fix editor highlight with new grammar
  * [x] on multi-line ( #172080 )
  * [x] for functions ( #172287 )

## Release notes

Enhance ES|QL query editing experience with client side validation.
Enhance ES|QL suggestions experience with more in context suggestions
leveraging field and variable types.
Show meta informations on ES|QL query hover on policy names.
Show function signature on ES|QL query hover on function text.


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [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
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Abdon Pijpelink <abdon.pijpelink@elastic.co>
@stratoula stratoula added v8.13.0 and removed v8.12.0 labels Dec 11, 2023
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 release_note:feature Makes this part of the condensed release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.13.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants