[ES|QL] Implements `Visitor` pattern for ES|QL AST #189516

vadimkibana · 2024-07-30T14:00:50Z

Summary

Partially addresses #182255

Implements the Visitor pattern for ES|QL AST trees. Unlike the Walker (which automatically traverses the whole tree exactly once), the Visitor pattern allows to control the traversal. The developer has to manually call children "visitor" routines. This manual handling enables:
- The AST tree can be traversed any number of times.
- Only a specific subset of the tree can be travered.
- Each visitor receives a context object, which can provide the global context as well as a linked list to all parent nodes.
  - The context object also provides node-specific read/write functionality.
- Each visitor can receive input from its parent node.
- Each visitor can return output to its parent node.
The visitor nodes are strictly typed: the context object as well as inputs and outputs have specific types. Also the inputs and outputs TypeScript types are inferred automatically from the callback signature the developer specifies and then the correct input/output usage is enforced in other callbacks.
The "scenarios" test file contains real-world usage scenarios, like:
- Changing the LIMIT
- Removing a "filter", a WHERE command

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-07-30T14:01:25Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

elasticmachine · 2024-07-30T14:01:25Z

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

drewdaemon

Very clever/cool API 👏

packages/kbn-esql-ast/src/builder/Builder.ts

packages/kbn-esql-ast/src/visitor/README.md

packages/kbn-esql-ast/src/visitor/__tests__/expressions.test.ts

packages/kbn-esql-ast/src/visitor/__tests__/scenarios.test.ts

packages/kbn-esql-ast/src/visitor/contexts.ts

packages/kbn-esql-ast/src/__tests__/ast_parser.sort.test.ts

stratoula

What an awesome api Vadim, excellent work!

packages/kbn-esql-ast/src/types.ts

packages/kbn-esql-ast/src/visitor/contexts.ts

vadimkibana · 2024-07-31T15:23:54Z

@elasticmachine merge upstream

kibana-ci · 2024-07-31T16:49:30Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: ed53219

Failed CI Steps

Jest Integration Tests #7

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`@kbn/esql-ast`	0	1	+1

Total ESLint disabled count

id	before	after	diff
`@kbn/esql-ast`	1	2	+1

History

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

…validation and autocomplete (#189689) ## Summary Fixed version of #188600 that updates the failed tests [caused by clash with the visitor API tests](#189516). ### Checklist Delete any items that are not applicable to this PR. - [ ] 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) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

implement esql ast visitor

c388c6f

vadimkibana requested a review from a team as a code owner July 30, 2024 14:00

vadimkibana requested review from drewdaemon, stratoula and qn895 July 30, 2024 14:23

drewdaemon approved these changes Jul 30, 2024

View reviewed changes

vadimkibana commented Jul 31, 2024

View reviewed changes

packages/kbn-esql-ast/src/__tests__/ast_parser.sort.test.ts Outdated Show resolved Hide resolved

stratoula approved these changes Jul 31, 2024

View reviewed changes

packages/kbn-esql-ast/src/types.ts Show resolved Hide resolved

packages/kbn-esql-ast/src/visitor/contexts.ts Show resolved Hide resolved

vadimkibana and others added 6 commits July 31, 2024 14:25

Update packages/kbn-esql-ast/src/__tests__/ast_parser.sort.test.ts

daf9fdf

Merge branch 'main' into esql-visitor

4199f76

make file names lower case

fbec948

remove command list from readme

3d2cbf3

add early walk termination test

fc64ac4

fix type error

df106c1

Merge branch 'main' into esql-visitor

ed53219

vadimkibana merged commit e49e89e into elastic:main Jul 31, 2024
20 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jul 31, 2024

qn895 mentioned this pull request Aug 1, 2024

[ES|QL] Improved support for Elasticsearch sub-types in AST for both validation and autocomplete #189689

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Implements `Visitor` pattern for ES|QL AST #189516

[ES|QL] Implements `Visitor` pattern for ES|QL AST #189516

vadimkibana commented Jul 30, 2024 •

edited

Loading

elasticmachine commented Jul 30, 2024

elasticmachine commented Jul 30, 2024

drewdaemon left a comment

stratoula left a comment

vadimkibana commented Jul 31, 2024

kibana-ci commented Jul 31, 2024

ESLint disabled in files

Total ESLint disabled count

[ES|QL] Implements Visitor pattern for ES|QL AST #189516

[ES|QL] Implements Visitor pattern for ES|QL AST #189516

Conversation

vadimkibana commented Jul 30, 2024 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 30, 2024

elasticmachine commented Jul 30, 2024

drewdaemon left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

vadimkibana commented Jul 31, 2024

kibana-ci commented Jul 31, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

ESLint disabled in files

Total ESLint disabled count

History

[ES|QL] Implements `Visitor` pattern for ES|QL AST #189516

[ES|QL] Implements `Visitor` pattern for ES|QL AST #189516

vadimkibana commented Jul 30, 2024 •

edited

Loading