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] Advanced validation and autocomplete #170071

Merged
merged 57 commits into from
Dec 7, 2023
Merged

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Oct 27, 2023

Summary

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

List of tasks:

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.

dej611 and others added 5 commits October 26, 2023 09:51
## Summary

Fixes #166242
This is a PR to use an higher level AST instead of the raw ParserTree to
compute all Monaco features for ES|QL.

## Grammar

This PR drops the existing grammar in favour of the ES one, which a tiny
tweak in order to support any case for the query.
I think we could fully adopt the ES grammar without tweaks once [this
issue](tunnelvisionlabs/antlr4ts#535) is
resolved in the antlr4ts dependency.

The adoption of the ES grammar helped also to resolve #166173 .

<img width="407" alt="Screenshot 2023-09-21 at 12 16 45"
src="https://github.com/elastic/kibana/assets/924948/38aa507b-8371-4387-ab8a-95423cdab684">
<img width="427" alt="Screenshot 2023-09-21 at 12 16 40"
src="https://github.com/elastic/kibana/assets/924948/462d9b57-24cf-45f8-bcf1-070bbdb67817">
<img width="444" alt="Screenshot 2023-09-21 at 12 16 29"
src="https://github.com/elastic/kibana/assets/924948/a6b5a904-c16f-40c6-a377-4e3e2005975f">
<img width="415" alt="Screenshot 2023-09-21 at 12 16 23"
src="https://github.com/elastic/kibana/assets/924948/e7732f91-17b2-4af5-9d52-0eb96fa1999e">
<img width="404" alt="Screenshot 2023-09-21 at 12 16 17"
src="https://github.com/elastic/kibana/assets/924948/99311f65-1390-4a90-8bb6-12b263d808fd">

## AST generator

The AST is a subset of the ESQL grammar parser tree, which is easier to
navigate.
Tests will be provided to the tool in order to test that the tool can
translate even the craziest query string.
**Note** that this is not preventing the query to be submitted to ES:
even if a validation case fails here or the AST fails to translate part
of the expression ES will always have the last word about error and
warnings, which will be promptly notified to the user.

Commands coverage:
* `ROW`
  * [x] `ROW a=1`
  * [x] `ROW a=1, b=1`
* `FROM`
  * [x] `FROM a`
  * [x] `FROM a, b`
  * [x] `FROM a [METADATA ...]`
  * [x] `FROM a, b [METADATA ...]` 
    * `METADATA` won't have indentifiers yet
* `SHOW`
  * [x] `INFO`
  * [x] `FUNCTIONS`
* `EVAL`
  * [x] `EVAL a = b + 1`
  * [x] `EVAL a = fnA( ... ) + fnB( ... )`
  * [x] `EVAL b + 1`
  * [x] `EVAL fnA( ... ) + fnB( ... )`
* `STATS`
  *  [x] `STATS a = fn(b) + fn(c)`
  *  [x] `STATS a = fn(b) + fn(c) BY group`
  *  [x] `STATS a = fn(b) + fn(c) BY groupA, groupB`
  *  [x] `STATS fn(b) + fn(c)`
  *  [x] `STATS fn(b) + fn(c) BY groupA`
  *  [x] `STATS fn(b) + fn(c) BY groupA, groupB`
* [x] `WHERE`
* [x] `LIMIT` 
* [x] `KEEP`
* [x] `PROJECT`
* [x] `DROP`
* `RENAME`. 
  * [x] `RENAME a as b`  
  * [x] `RENAME `a + 1 / 5` as `myField`
* [x] `DISSECT`
* [x] `GROK`
* [x] `ENRICH`
* [x] `MV_EXPAND`
* [x] `SORT`

Collect all user defined variables
  * [x] Basic support for assigned variables (i.e. `eval a = ...`)
  * [x] Field type shadowing with assignment
  * [x] Advanced support for non-assigned cases? (i.e. `eval a + 1`)

## Validation

With this new approach it would be possible to catch most of warnings
and errors while typing, preventing users to submit invalid queries to
ES.
Note that here the validation function can detect multiple errors and
warnings at once, while ES only returns one error at the time -
improving the current sub-optimal editing experience provided.


![esql_clientside_validation](https://github.com/elastic/kibana/assets/924948/c02f057c-efb9-40b6-ad4a-9642a11c8367)

Tasks
* Create definitions
  * [x] Define function definitions
  * [x] Define aggs definitions
  * [x] Define command definitions
  * [x] Define command options definitions
* Create basic validation walker function
  * [x] Support both errors and warnings
  * [x] Validate commands
  * [x] Validate command options
  * [x] Validate functions
  * [x] Validate source/columns (I phase - just formally)
  * [x] Validate policies (I phase - just formally)
  * [x] Validate literals
  * [x] Validate duration/math syntax
  * [x] Validate variables
* Improve validation work with high level checks
  * [x] Validate source/columns (II phase - leverage ES metadata)
  * [x] Validate policies (II phase - leverage ES metadata)
  * [x] Optimize queries
    * phase I: just make queries strictly when required
* Write tests
  * [x] Source commands
    * [x] Options
  * [x] Processing commands
    * [x] Options
  * [x] Sorting commands
    * [x] All options, combinations
  * [x] Enrich commands
    * [x] Options
  * [x] Warnings cases
* [x] Integrate all of it as Monaco diagnostics module
* [x] Solve issue with long queries and single line mode
* Provide better sync between client and server side error management
  * [x] Show server side warnings only on absence of errors
  * [x] Clear up server side errors on code change

## Autocomplete

Using the same provider here to provide better contextual suggestions.
First example giving better suggestions (filtered field based on types
and functions based on returned type):


![better_suggestions_types](https://github.com/elastic/kibana/assets/924948/bffe158f-bb31-47cf-9cd6-8bdcade41409)

* Commands
  * [x] Show all commands on empty string
  * [x] Show all but source commands after first pipe
  * [x] Suggest option at the right moment

## Hover

Basic hover information about used functions:
<img width="688" alt="Screenshot 2023-10-16 at 16 22 16"
src="https://github.com/elastic/kibana/assets/924948/e623c71d-4bbc-4511-8bb8-d818e465e554">
<img width="533" alt="Screenshot 2023-10-16 at 16 21 54"
src="https://github.com/elastic/kibana/assets/924948/af7a244f-be5b-46a0-8704-09c1ffc48a9e">

# Things not covered in this PR

* Load function definitions from `SHOW FUNCTIONS` at language bootstrap
* Make `validate` function an independent module?
* Provide a "formal" validation option (i.e. mute `Unknown <entity>
[...]` messages)
* Have `autocomplete` on par with the current state in `main`

## Validation cases not covered yet

* `... | eval var = 1 year` => ❌ it should report an error
* `... | eval 1 + [1, 2, 3]` => ⚠️ it should report a null warning
  * same applies for `-`, `*`, `/`, `%`
* any location aware field/variable check
* `... | stats a = avg(field) | keep b` => ❌ `b` should be reported as
unknown
  * `... | keep a | eval b` => ❌  `b` should be reported as unknown
* `... | rename a as b, d as c, c as b` => ❌ `c` should be reported as
unknown
* `... | enrich policy on fieldFromEnrich` => ❌ `fieldFromEnrich` is not
reported as unknown
* `... | eval a = round(b), b = round(a)` => ❌ `b` should be reported an
unknown
* `from ... [metadata <list of meta fields>` => ❌ `meta fields` won't be
validated as there's currently no available API to get them

## Hover

Advanced usage of hover feature.

## Signature

TBD

---------

Co-authored-by: kibanamachine <[email protected]>
@dej611 dej611 added release_note:enhancement Feature:ES|QL ES|QL related features in Kibana labels Nov 1, 2023
dej611 and others added 10 commits November 1, 2023 10:32
## Summary

This PR extends the validation logic to cover also wildcase scenarios
for various commands:

* wildcards are now supported by `drop` and `keep`:

<img width="482" alt="Screenshot 2023-10-27 at 12 21 19"
src="https://github.com/elastic/kibana/assets/924948/c49126e8-d20f-42cd-b950-a9c73df560dc">
<img width="483" alt="Screenshot 2023-10-27 at 12 21 41"
src="https://github.com/elastic/kibana/assets/924948/3cb3042d-f87b-4983-8dc2-cb88dc730dfd">

* the special case of `drop *` is marked as error:

<img width="468" alt="Screenshot 2023-10-27 at 12 21 12"
src="https://github.com/elastic/kibana/assets/924948/9f973dbd-91bb-4c79-a396-c1e88fe18133">

* a fuzzy search is performed over `fields` and `variables` for wildcard
names, and errors are reported if no match is found:

<img width="477" alt="Screenshot 2023-10-27 at 12 21 29"
src="https://github.com/elastic/kibana/assets/924948/accb541d-338c-4e49-8096-e95807ad75de">

* handled also the case of `drop @timestamp` with a warning:

<img width="534" alt="Screenshot 2023-10-27 at 12 40 59"
src="https://github.com/elastic/kibana/assets/924948/e5c9f84d-e89a-4c42-ba97-9994ad4d48cb">

Notes:
* `by*tes` `*bytes` and `bytes*` will all match `bytes` from my tests on
ES, therefore the validation will make them pass as valid if there's a
`bytes` field.

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


![esql_autosuggest_1](https://github.com/elastic/kibana/assets/924948/5e30cb85-4b72-4234-a4fc-098b8360ed98)

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.

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

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
@dej611 dej611 added v8.12.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure ui-copy Review of UI copy with docs team is recommended labels Nov 27, 2023
@dej611
Copy link
Contributor Author

dej611 commented Nov 27, 2023

cc @elastic/docs here's all the error messages with their relative source (Elasticsearch or Kibana) to review:

## Summary

Address again the shared bundle size.
…1866)

## Summary

This PR adds a cache at query level refreshed every 10 minutes for
fields queries used in both validation and autocomplete.

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

This PR adds some more hover feature on policies hovering, showing some
useful details:

<img width="531" alt="Screenshot 2023-11-24 at 18 02 18"
src="https://github.com/elastic/kibana/assets/924948/b47e957e-a9eb-4c20-bd85-05f69fb350b1">

Initially I thought about adding also the signature for `command` and
`option` on hover, but there's enough space there for examples, which
are kind of important together with the signature.
I'd wait for #171720 to be merged,
and then leverage the `HTML` support in markdown together with some
additional nice features in the editor with the newer version.


### Checklist

- [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)
… part of regular validation flow (#171968)

## Summary

This PR removes the warning highlights:

<img width="605" alt="Screenshot 2023-11-27 at 11 41 51"
src="https://github.com/elastic/kibana/assets/924948/c3f8ecf4-5a58-4e4b-a779-68f9b48b3bf2">

Also, syntax errors were implicitly handled by the old flow within the
editor only, but now it falls into the regular validation flow:

<img width="569" alt="Screenshot 2023-11-27 at 11 41 56"
src="https://github.com/elastic/kibana/assets/924948/9df97d3c-ac1f-4a6e-8de2-7881a8128902">

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
@dej611 dej611 marked this pull request as ready for review November 27, 2023 14:47
@dej611 dej611 requested review from a team as code owners November 27, 2023 14:47
@elasticmachine
Copy link
Contributor

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

@stratoula
Copy link
Contributor

@dej611 here it is. You have to try on the dashboard editors.

I cannot reproduce this bug. Can you reproduce with a sample dataset?

image

@stratoula
Copy link
Contributor

I can still reproduce this:

The suggestions do not hit if I go back to the stats agg function and change the field for example

Example on how it works in main:
esql

While on this PR:
esql-nonreleased

Everything else seems to work great! 🎉

@stratoula stratoula added release_note:feature Makes this part of the condensed release notes backport:skip This commit does not require backporting and removed release_note:enhancement labels Dec 4, 2023
@stratoula
Copy link
Contributor

@dej611 can you also add to the description the list of things we are closing with this PR and also a brief release note with the things that are being added (to help our docs team with the release notes)

@dej611
Copy link
Contributor Author

dej611 commented Dec 4, 2023

I can still reproduce this:

The suggestions do not hit if I go back to the stats agg function and change the field for example

Example on how it works in main: ...

While on this PR: ...

Everything else seems to work great! 🎉

Ok, so I think I misunderstood the previous case. I've added a fix for this with f86dc89

@dej611 here it is. You have to try on the dashboard editors.

I cannot reproduce this bug. Can you reproduce with a sample dataset?

...

🤔 I do not have it

esql_dashboard_warning

@dej611
Copy link
Contributor Author

dej611 commented Dec 4, 2023

Ok, managed to reproduce the empty warning issue on dashboard query editing and pushed a fix with 3127c44

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.

Changes LGTM! Marco thanx for your patience and devotion to this PR!
It works great!

  • Can you create a followup issue with the things you have on your description?
  • I am not 100% sure if we should merge it now (8.12) or after the FF (8.13). Let's discuss offline.

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Retested, everything works as expected, couldn't notice any regression after the fixes! 💯 Amazing job!

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Shared-ux code changes lgtm

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, review only.

@stratoula stratoula added v8.13.0 and removed v8.12.0 labels Dec 4, 2023
@stratoula
Copy link
Contributor

stratoula commented Dec 4, 2023

Thanx Vadim and Anton! We are going to merge after the FF so this is going to be released in 8.13. As many applications are using the ES|QL editor and is going to be added to more we want this to be tested internally first.

@dej611 dej611 mentioned this pull request Dec 6, 2023
5 tasks
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/monaco 76 96 +20

Async chunks

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

id before after diff
textBasedLanguages 151.9KB 152.5KB +611.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

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

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.2MB 2.3MB +57.0KB
Unknown metric groups

API count

id before after diff
@kbn/monaco 78 98 +20

History

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

@dej611 dej611 merged commit ff49526 into main Dec 7, 2023
36 checks passed
@dej611 dej611 deleted the feature/esql-validation branch December 7, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure ui-copy Review of UI copy with docs team is recommended v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ES|QL] Simplify grammar vs improve validation checks on top of it
9 participants