[ES|QL] Use custom AST for suggestion, validation, etc... #166185

dej611 · 2023-09-11T15:48:03Z

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 is resolved in the antlr4ts dependency.

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

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:

Collect all user defined variables

Basic support for assigned variables (i.e. eval a = ...)
Field type shadowing with assignment
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.

Tasks

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):

Commands
- Show all commands on empty string
- Show all but source commands after first pipe
- Suggest option at the right moment

Hover

Basic hover information about used functions:

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

stratoula

Some notes from me (I haven't checked the code yet)

For some reason I get a 0 warning icon

This works ok in main ( I should see 1 warning and also the warning on the editor footer but I don't)

I see the error for unknown index but is wrong (I have multiple logstash indices and I want to use the wildcard)

I think this is important, I want to see the list of aggregations here.

Here also

If we want to work on autocomplete on a follow up PR can we merge this in a feature branch instead and not main. I feel that the regressions in autocomplete are important and we should not merge them in main.

Seems the validation hasn't run properly here

Here I expect var1 to be in the list of suggestions as it is on stats

…into feature/esql-to-ast

dej611 · 2023-10-18T09:43:51Z

Thanks for your quick feedback @stratoula .
I'd prefer to split validation or hover issue from autocomplete as the latter deserve a more focused approach and there's already a lot of content here. I'm ok to have a feature branch to merge in before main and work on a separate autocomplete PR as follow up.

Will reply inline here about validation and UI issues, will defer the autocomplete ones for a later PR:

For some reason I get a 0 warning icon

This works ok in main ( I should see 1 warning and also the warning on the editor footer but I don't)

Can reproduce as well, fixed with 9acd436

I see the error for unknown index but is wrong (I have multiple logstash indices and I want to use the wildcard)

This is a bug as I was not validating fuzzy strings for indices. Added the check and a unit test for it with 8ec2226

Seems the validation hasn't run properly here

Can reproduce as well, fixed with 9acd436

dej611 · 2023-10-18T12:28:05Z

It seems that this PR is actually reducing the bundlesize of the kbn-monaco bundle rather than increase it.

Enabling detailed stats on the kbn-monaco webpack optimizer I get this numbers:

[ibazel]     Hash: 8f992c2e72951cec4d98
[ibazel]     Time: 1797ms
[ibazel]     Built at: 10/18/2023 12:43:36 PM
[ibazel]                         Asset      Size  Chunks                   Chunk Names
[ibazel]         esql.editor.worker.js  1.75 MiB    main  [emitted]        main
[ibazel]     esql.editor.worker.js.map  1.85 MiB    main  [emitted] [dev]  main
[ibazel]     Entrypoint main = esql.editor.worker.js esql.editor.worker.js.map
[ibazel]     chunk {main} esql.editor.worker.js, esql.editor.worker.js.map (main) 1.56 MiB [entry] [rendered]

While on main I get this:

[ibazel]     Hash: 2b4a5e316705efa60531
[ibazel]     Time: 2294ms
[ibazel]     Built at: 10/18/2023 12:45:28 PM
[ibazel]                         Asset      Size  Chunks                   Chunk Names
[ibazel]         esql.editor.worker.js  2.04 MiB    main  [emitted]        main
[ibazel]     esql.editor.worker.js.map  2.15 MiB    main  [emitted] [dev]  main
[ibazel]     Entrypoint main = esql.editor.worker.js esql.editor.worker.js.map
[ibazel]     chunk {main} esql.editor.worker.js, esql.editor.worker.js.map (main) 1.79 MiB [entry] [rendered]

so kbn-monaco get ~200kb lighter from the stats.

After looking at the shared-bundle-src detailed view maybe I get what's going on here:

[ibazel]                                 Asset       Size                  Chunks                          Chunk Names
[ibazel]  a1cef3d530e1adb0f52b2f62994a2aca.ttf  69.3 KiB                          [emitted]       
- [ibazel]     kbn-ui-shared-deps-src.chunk.0.js   1.37 MiB                       0  [emitted]        [big]  
+ [ibazel]     kbn-ui-shared-deps-src.chunk.0.js   466 KiB                       0  [emitted]        [big]  
- [ibazel] kbn-ui-shared-deps-src.chunk.0.js.map   1.42 MiB                       0  [emitted] [dev]   
+ [ibazel] kbn-ui-shared-deps-src.chunk.0.js.map   438 KiB                       0  [emitted] [dev]              
- [ibazel]     kbn-ui-shared-deps-src.chunk.1.js    466 KiB                       1  [emitted]        [big]     
+ [ibazel]     kbn-ui-shared-deps-src.chunk.1.js  14.6 KiB                       1  [emitted]             
- [ibazel] kbn-ui-shared-deps-src.chunk.1.js.map    438 KiB                       1  [emitted] [dev]     
+ [ibazel] kbn-ui-shared-deps-src.chunk.1.js.map  14.2 KiB                       1  [emitted] [dev]         
- [ibazel]     kbn-ui-shared-deps-src.chunk.2.js   14.6 KiB                       2  [emitted]            
+ [ibazel]     kbn-ui-shared-deps-src.chunk.2.js  4.94 KiB                       2  [emitted]                 
- [ibazel] kbn-ui-shared-deps-src.chunk.2.js.map   14.2 KiB                       2  [emitted] [dev]     
+ [ibazel] kbn-ui-shared-deps-src.chunk.2.js.map  4.35 KiB                       2  [emitted] [dev]       
- [ibazel]     kbn-ui-shared-deps-src.chunk.3.js   1.33 KiB                       3  [emitted]                    
+ [ibazel]     kbn-ui-shared-deps-src.chunk.3.js  7.61 KiB                       3  [emitted]               
- [ibazel] kbn-ui-shared-deps-src.chunk.3.js.map  124 bytes                       3  [emitted] [dev]       
+ [ibazel] kbn-ui-shared-deps-src.chunk.3.js.map  7.87 KiB                       3  [emitted] [dev]        
- [ibazel]     kbn-ui-shared-deps-src.chunk.4.js   4.94 KiB                       4  [emitted]               
- [ibazel] kbn-ui-shared-deps-src.chunk.4.js.map   4.35 KiB                       4  [emitted] [dev]         
- [ibazel]     kbn-ui-shared-deps-src.chunk.5.js   7.61 KiB                       5  [emitted]               
- [ibazel] kbn-ui-shared-deps-src.chunk.5.js.map   7.87 KiB                       5  [emitted] [dev]         
[ibazel]            kbn-ui-shared-deps-src.css   92.2 KiB  kbn-ui-shared-deps-src  [emitted]               kbn-ui-shared-deps-src
[ibazel]        kbn-ui-shared-deps-src.css.map    120 KiB  kbn-ui-shared-deps-src  [emitted] [dev]         kbn-ui-shared-deps-src
- [ibazel]             kbn-ui-shared-deps-src.js   7.47 MiB  kbn-ui-shared-deps-src  [emitted]        [big]  kbn-ui-shared-deps-src
+ [ibazel]             kbn-ui-shared-deps-src.js  8.97 MiB  kbn-ui-shared-deps-src  [emitted]        [big]  kbn-ui-shared-deps-src
- [ibazel]         kbn-ui-shared-deps-src.js.map    6.5 MiB  kbn-ui-shared-deps-src  [emitted] [dev]         kbn-ui-shared-deps-src
+ [ibazel]         kbn-ui-shared-deps-src.js.map  8.01 MiB  kbn-ui-shared-deps-src  [emitted] [dev]         kbn-ui-shared-deps-src

Apparently part of the chunk.0 and chunk.3 in this PR gets incorporated into the kbn-ui-shared-deps-src.js initial bundle. I'll investigate whether it's possible to move it back to an async bundle the kbn-monaco package, but I wonder if its actual shrink may have played some role in here.

The good news is that overall this PR adds a whopping 12.8 Kb overall to the shared bundle.
The bad news is that this increase seems to be shifted on the wrong side.

dej611 · 2023-10-19T07:25:59Z

Managed to have a view of what is going on here and why the bundle changed.

This PR moved both esql and antlr4ts into the main bundle:

While in main both modules are in the async chunk (not tracked by the github metrics):

I'll try to move back both modules back to the async realm.

dej611 · 2023-10-19T12:29:17Z

Managed to solve the bundle issue and will report here some things I found during the investigation which might be useful if somebody is facing the same problem with the shared bundle:

Mind your index.ts "barrel files"
- this is not something strictly a performance problem as described in this article, but rather a dependency graph problem. The tree-shaking power of a bundler is limited and adding more depth in the analysis between two modules with middle men like "barrel files" might make it harder to optimized (for the bundler) and debug (for the developer) this type of problems.
if you can't see it, it's hard to debug
- it's really hard to debug something you do not see, especially when you have to track down dependencies and where a package is to determine the final bundle size. Right now it is not easy to analyse bundle size of something that is not a plugin, as well documented in the Kibana documentation. So for this specific use case it was required to manual tweak the Bazel + Webpack configuration in order to emit a stats.json file that can be visualized.
- Tools to visualize here are fundamental to understand the relationships of various packages, and the combination of several representation is key to find the problem: Webpack bundle analyzer was useful to understand in which chunk the package kbn-monaco, antlr4ts and more were stored, while the Webpack analyse > Modules app was super useful to track the type of imports (async vs sync) between files.
Mind static and class-based properties. This code got me some headache to refactor in a way that was treeshaking properly:

const symbolsLookup: Record<number, string> = Object.entries(esql_parser)
  .filter(([k, v]) => typeof v === 'number' && !/RULE_/.test(k) && k.toUpperCase() === k)
  .reduce((memo, [k, v]: [string, number]) => {
    memo[v] = k;
    return memo;
  }, {} as Record<number, string>);

The problem here was that the compiler didn't know what to keep and what to drop as the code was iterating thru everything there. The code was needed to map rule numbers to their string equivalent, but luckily there's a static property to use which makes the tree-shake work performantly:

esql_parser.VOCABULARY.getSymbolicName(<tokenId>);

if you have a big dependency, moving the logic who depends on that on a Web Worker moves the big dependency on an async chunk.
- in this case generating the simplified AST is not much code (ast_factory.ts + ast_walker.ts) per se, but it requires the esql_parser full dependency, which in turns requires the full antlr4ts library, which is worth 1 Mb. Moving the AST generation on the web worker makes the architecture and the flow a little bit more convoluted (see the new packages/kbn-monaco/src/esql/lib/monaco/esql_ast_provider.ts adapter) but it ensures that the 1 Mb dependency is required only within the web worker as async chunk.

dej611 · 2023-10-23T07:37:02Z

As discussed offline with @stratoula this PR now targets the new feature branch feature/esql-validation where next iterations with autocomplete and fetching optimizations will land.

Once this PR will get reviewed (validation & hover) and merged into the new feature branch the following follow ups will be pursued:

fix autocomplete
- in particular put stats and eval on par with current main state
fix number of requests for validation
- not allow it to make requests actively and only enable field validation when the cache gets populated by autocomplete
- same for hover
- restore muted tests and add new ones

kibana-ci · 2023-10-23T08:45:51Z

💚 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	94	+18

Async chunks

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

id	before	after	diff
`textBasedLanguages`	148.8KB	149.3KB	+570.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	+47.5KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/monaco`	78	96	+18

ESLint disabled line counts

id	before	after	diff
`@kbn/text-based-editor`	2	3	+1

Total ESLint disabled count

id	before	after	diff
`@kbn/text-based-editor`	2	3	+1

History

💔 Build #169426 failed a573197
💔 Build #169395 failed c293b55
💔 Build #169021 failed 121a6df
💔 Build #168936 failed d7c4b75
💛 Build #168602 was flaky b30f77b

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

elasticmachine · 2023-10-23T08:51:33Z

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

davismcphee · 2023-10-23T19:59:21Z

test/functional/apps/discover/group3/_request_counts.ts

@@ -223,7 +223,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
      });
    });

-    describe('ES|QL mode', () => {
+    // @TODO: fix this in a follow up


I didn't realize this was going into a feature branch at first and was surprised to see these tests skipped, but assuming they'll be addressed and unskipped before making it to main, this LGTM 👍

Davis correct, we found some performance problems so this is not going to be merged. Also autocomplete doesnt work as expected. But as this is a big PR already we want to move this into a feature branch and then continue with the investigation and the development

stratoula

As discussed with Marco this is not ready to be merged and this is why we decided to create a feature branch instead. Specifically:

autocomplete needs some refinement
We should see what we are going to about making the client side validation performant. Currently we are hitting ES every 250ms and also we add another api call to Discover initialization

As a first step the code looks ok to me so I am approving the merge to the feature branch

Dosant

I think it would be great if you'd make your team the owner of kbn-monaco/src/esql :D

## 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 <[email protected]> Co-authored-by: Stratoula Kalafateli <[email protected]> Co-authored-by: Abdon Pijpelink <[email protected]>

✨ Initial draft of esql ast

5e42f97

dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:ES|QL ES|QL related features in Kibana labels Sep 11, 2023

dej611 added 7 commits September 12, 2023 10:13

✨ Merge errors from client and server

cc01d6d

Merge remote-tracking branch 'upstream/main' into feature/esql-to-ast

62e1ecc

🚧 AST WIP

a44b130

Merge remote-tracking branch 'upstream/main' into feature/esql-to-ast

2c02a9f

✨ Add client side ESQL validation

4d7b2bb

🏷️ Refactor error logic to leverage types

8caf186

🔥 Remove unused types

c3ecc29

dej611 added the ci:build-webpack-bundle-analyzer label Sep 21, 2023

dej611 and others added 19 commits September 21, 2023 15:12

Merge branch 'main' into feature/esql-to-ast

26d104c

🔥 Remove unused commands

1eece8a

🔥 Remove type

4a7d221

✨ Plug autocomplete to the new system

64f57c5

✨ Include syntax errors within the regular editor flow

db10a70

Merge remote-tracking branch 'upstream/main' into feature/esql-to-ast

3d03335

✨ Improve validation

5d0f732

✨ Tested all combinations

4ef449c

🔧 Tweak namespace

257bcab

🐛 Fix type and load ast handler at runtime

e966cc4

Merge remote-tracking branch 'upstream/main' into feature/esql-to-ast

a7b5b2a

🐛 Fix test for mv_expand

fac6743

✨ Autocomplete ast-based with initial tests

6ba78ce

🏷️ Fix types

185e60d

🔥 Remove unused provider

cc14363

🏷️ Reduce non-type imports

56dd04c

✨ Add treeshake

7671dc0

🏷️ Fix type

a6cf75c

🐛 Fix check issues

5bca6f2

dej611 added the v8.12.0 label Oct 18, 2023

stratoula reviewed Oct 18, 2023

View reviewed changes

dej611 added 4 commits October 18, 2023 10:45

🔥 Shrink more code

4219638

Merge branch 'feature/esql-to-ast' of https://github.com/dej611/kibana …

c0927d9

…into feature/esql-to-ast

🐛 Fix fuzzy match on indices

8ec2226

🐛 Fix debounce and warning UI bugs

9acd436

Merge remote-tracking branch 'upstream/main' into feature/esql-to-ast

d7c4b75

♻️ Shuffle a bit bundles to have more async

121a6df

♻️ Shuffle the architecture to restore previous chunk organization

c293b55

dej611 added 2 commits October 19, 2023 14:31

🐛 Fix tests

a573197

🔇 Mute the test for now

4e4222b

dej611 changed the base branch from main to feature/esql-validation October 23, 2023 07:28

Merge branch 'feature/esql-validation' into feature/esql-to-ast

5f5c9d2

dej611 marked this pull request as ready for review October 23, 2023 08:51

dej611 requested review from a team as code owners October 23, 2023 08:51

davismcphee approved these changes Oct 23, 2023

View reviewed changes

stratoula approved these changes Oct 24, 2023

View reviewed changes

Dosant approved these changes Oct 25, 2023

View reviewed changes

dej611 merged commit e793907 into elastic:feature/esql-validation Oct 26, 2023
1 check passed

dej611 mentioned this pull request Nov 27, 2023

[ES|QL] Advanced validation and autocomplete #170071

Merged

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Use custom AST for suggestion, validation, etc... #166185

[ES|QL] Use custom AST for suggestion, validation, etc... #166185

dej611 commented Sep 11, 2023 •

edited

Loading

stratoula left a comment •

edited

Loading

dej611 commented Oct 18, 2023

dej611 commented Oct 18, 2023

dej611 commented Oct 19, 2023

dej611 commented Oct 19, 2023

dej611 commented Oct 23, 2023 •

edited

Loading

kibana-ci commented Oct 23, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

elasticmachine commented Oct 23, 2023

davismcphee Oct 23, 2023

stratoula Oct 24, 2023

stratoula left a comment •

edited

Loading

Dosant left a comment

[ES|QL] Use custom AST for suggestion, validation, etc... #166185

[ES|QL] Use custom AST for suggestion, validation, etc... #166185

Conversation

dej611 commented Sep 11, 2023 • edited Loading

Summary

Grammar

AST generator

Validation

Autocomplete

Hover

Things not covered in this PR

Validation cases not covered yet

Hover

Signature

stratoula left a comment • edited Loading

Choose a reason for hiding this comment

dej611 commented Oct 18, 2023

dej611 commented Oct 18, 2023

dej611 commented Oct 19, 2023

dej611 commented Oct 19, 2023

dej611 commented Oct 23, 2023 • edited Loading

kibana-ci commented Oct 23, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

elasticmachine commented Oct 23, 2023

davismcphee Oct 23, 2023

Choose a reason for hiding this comment

stratoula Oct 24, 2023

Choose a reason for hiding this comment

stratoula left a comment • edited Loading

Choose a reason for hiding this comment

Dosant left a comment

Choose a reason for hiding this comment

dej611 commented Sep 11, 2023 •

edited

Loading

stratoula left a comment •

edited

Loading

dej611 commented Oct 23, 2023 •

edited

Loading

stratoula left a comment •

edited

Loading