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] AST traversal API #182255

Closed
drewdaemon opened this issue May 1, 2024 · 8 comments · Fixed by #189957
Closed

[ES|QL] AST traversal API #182255

drewdaemon opened this issue May 1, 2024 · 8 comments · Fixed by #189957
Assignees
Labels
DX Issues related to Developer Experience enhancement New value added to drive a business result impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented May 1, 2024

Finding things in an AST (and possibly changing them) requires a tree-traversal algorithm. Developers working with ES|QL queries shouldn't have to worry about implementing this themselves. Instead, we should provide a nice API.

Ideas

const nodes = findAll(ast, 'function', ['==', '!=']);
...

Or maybe

const matcher = (node, ctx) => ctx.depth == 1
  && isFunctionNode(node) 
  && ['==', '!='].includes(node.name);

const nodes = findAll(ast, matcher);

...

Or for manipulation, something like

const negateComparisons = (ast) => {
  walk(ast, (node) => {
    if (isFunctionNode(node) && node.name === '==') {
      node.name = '!=';
    }
  });
}
...

Could also provide typed shortcut methods like

const walker = new Walker(ast);
const functions = walker.findAllFunctions();
const constants = walker.findAllConstants();
const source = walker.findSource()

Each of those could return typed AST nodes, removing the need for consumers to run their own type checks.

Or we could implement a visitor like recast's

visit(ast, {
  onCommand: (node, ctx) => {
    messages.push(...validateCommand(node, ctx));
  },
  onFunction: ...
});

Some comparables

Some potential adoption points

@drewdaemon drewdaemon added enhancement New value added to drive a business result Team:ESQL ES|QL related features in Kibana labels May 1, 2024
@elasticmachine
Copy link
Contributor

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

@drewdaemon drewdaemon added the DX Issues related to Developer Experience label May 1, 2024
@stratoula stratoula added the impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. label May 1, 2024
@drewdaemon
Copy link
Contributor Author

It would be great to use these walker APIs in our own validation engine as well.

@mattkime
Copy link
Contributor

mattkime commented May 6, 2024

I agree that this looks very useful, but I think its something that might live inside a more opinionated service that takes the whole ESQL string and provides getters and mutators.

@drewdaemon
Copy link
Contributor Author

@mattkime I think I get what you're saying, but I think a couple examples of how you would use the opinionated service to accomplish a given task would be helpful here. One recent task we had was adding a new where clause if it doesn't exist, or edit it if it does. How would you accomplish this with the service you're envisioning? What would the getters and mutators look like?

@vadimkibana
Copy link
Contributor

vadimkibana commented Jun 21, 2024

There is an initial implementation of the Walker now available, it is missing:

  • Traversal of all node types
    • For example, option nodes
  • The context object

@stratoula
Copy link
Contributor

@vadimkibana also a Readme (documentation) when this is more mature would be very nice too.

@drewdaemon
Copy link
Contributor Author

We may also want to consider giving the user more control over the traversal. Recast does this with this.traverse(), this.abort(), and return false;

@vadimkibana
Copy link
Contributor

Acorn AST walker for inspiration: https://github.com/acornjs/acorn/tree/master/acorn-walk

vadimkibana added a commit that referenced this issue Jul 22, 2024
## Summary

Partially addresses #182255

- This PR add support for all ES|QL AST node types in the `Walker`
class, which means all nodes from any query now will be visited.


### Checklist

Delete any items that are not applicable to this PR.

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

### For maintainers

- [x] 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)
vadimkibana added a commit that referenced this issue Jul 31, 2024
## 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`](https://github.com/elastic/kibana/pull/189516/files#diff-571e21fd50dbdb664e71297e2edd72c1a1b2b96f346248f0360558ef8ceb75f7R20)
- [Removing a "filter", a `WHERE`
command](https://github.com/elastic/kibana/pull/189516/files#diff-571e21fd50dbdb664e71297e2edd72c1a1b2b96f346248f0360558ef8ceb75f7R57)
 


### Checklist

Delete any items that are not applicable to this PR.

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


### For maintainers

- [x] 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: Elastic Machine <elasticmachine@users.noreply.github.com>
vadimkibana added a commit that referenced this issue Aug 6, 2024
## Summary

Closes #182255

Implements high-level AST node finding APIs, which satisfy requirements
in #182255

- Introduces `"visitAny"` callback, which capture any AST node (not
captured by a more specific callback).
- `Walker.find(ast, predicate)` &mdash; finds the first AST node that
satisfies a `predicate` condition.
- `Walker.findAll(ast, predicate)` &mdash; finds the all AST nodes that
satisfies a `predicate` condition.
- `Walker.match(ast, template)` &mdash; finds the first AST node that
*matches* the provided node `template`.
- `Walker.matchAll(ast, template)` &mdash; finds all AST nodes that
*match* the provided node `template`.


For example, here is how you find all `agg1` and `agg2` function nodes:

```ts
const nodes = Walker.matchAll(ast, { type: 'function', name: ['agg1', 'agg2'] });
```

### Checklist

Delete any items that are not applicable to this PR.

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials

### For maintainers

- [x] 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues related to Developer Experience enhancement New value added to drive a business result impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants