[ES|QL] Validation and autocomplete code improvements

[drewdaemon]

That the validation and autocomplete engines work today is a feat in and of itself. However, there is a lot of room for simplification and understandability improvements.

[drewdaemon]

Separate AST traversal from validation code

Our current validation routine is essentially a top-down AST traversal that runs various node-type-specific validation routines. For example, there’s a routine for function arguments, and one for commands, and one for options, etc.
Right now, it’s essentially a set of calls between these routines. For example, the command routine calls the function routine, passes down some context, and then the function routine may call itself if there’s a nested function before finally checking the args, etc.
There’s a messages array that gets passed by reference through the call stack and collects errors and warnings that are generated.
What if, instead, we separated the validation from the traversal logic and ended up with a single-node-centric validation pattern.
const messages = [];

walk(ast, (node) => {
  if (isCommand(node)) {
    /* validate command */
    messages.push(...validateCommand(node));
  }
  if (isFunction(node)) {
    /* validate function */
    messages.push(...validateFunction(node));
  }
  ...
}

return messages;

This might require that the AST have bidirectional references since you might have to “reach up” to the parent of a node to perform a proper validation.

But, I think this could be a great separation of concerns (separating tree traversal and checking against definitions). I also have a feeling that it would promote type specificity since the node-type checks will happen at the top level and there will be no casting or checking within the subroutines….

[vadimkibana]

What do you think about traversal API, where for each node type an explicit function is defined (could be slightly more performant and simpler to read)?

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

or

const commands: string[] = [];

walk(ast, {
  onCommand: ({name}) => commands.push(name),
});

[drewdaemon]

Good point on the serialization. I like your idea of passing parent references in the context object.

As far as the specifics for the traversal APIs, I like the design here. It is similar to recast's visitor API which I used for the validation test generator.

I have added this idea to the parent issue

[dej611]

What's the use case for this API?
So far validation is the only one performing a DFS over the AST. How that can improve the code there?

All the other services/features are usually provided 2 inputs: AST and position, and in there's already the getASTContext for this use case (which already define a context type interface). They do some tiny traversal for one level, but usually direct parent access.

I guess one case can be made for a reverse query function (ast: AST) => string, but even in that case writing a DFS is pretty simple for a tree.

[drewdaemon]

Marco take a look at #182255 for more info. The discussion here centers around using the API for the validation engine. That issue centers around the design and thinking behind the API

[drewdaemon]

Also, #182257 for

reverse query function (ast: AST) => string

[drewdaemon]

Handle literal suggestions uniformly and declaratively

Right now we have two ways to suggest literal values. One is the literalOptions property that lives on a functions parameter definition. The other is getCompatibleLiterals in packages/kbn-esql-validation-autocomplete/src/autocomplete/factories.ts

Does this make sense? I would vote for literal suggestions always to be defined in the definition of a parameter. If they need to be generated based on dynamic context, the logic could be housed in a callback attached to the parameter definition, just like we do with custom validation.

At the moment, getCompatibleLiterals makes me nervous. It does checks based on both command name and parameter type and feels like it will be difficult to maintain/grow.

[drewdaemon]

Improve names

• isEqualType -> checkArgTypeMatchesDefinition
• getColumnHit -> getColumnByName
• literal -> constant
• literalOptions -> acceptedConstants
• builtin function -> operation
• ESQLAstBaseItem.name -> kind
• eval functions should be called scalar functions

[vadimkibana]

Yes, all functions would be just kind = function, name = my-function, but every command could have its own kind. There can be infinite number of functions, but all commands are built into the language.

[vadimkibana]

Another naming improvement I would like to suggest is to use the same methodology as in the ANTLR grammar.

---

For example, the BY clause in METRICS and STATS commands is surrounded by aggregates and grouping fields ... <aggregates> BY <grouping>, but in in the current AST we call the BY clause an "option" and the grouping fields we call "args" which have type of "...Item".

That is just one example, I'm sure there is more. Essentially, my point is to stick to the ANTLR grammar namings.

---

• options 👉 grouping
• args/items 👉 fields

[dej611]

From experience, those names change quite often. I would not stick to those.

[vadimkibana]

• ESQLMessage 👉 ESQLError (ESQLMessage is actually an error object, which contains a message inside)

[vadimkibana]

Introduce an ESQLAstNode type, which is a union of all possible AST nodes. And make sure that all nodes, which have different shape, have a different discriminator field, like .type or .kind. This will allow to type-safely narrow down AST node types, just by conditioning on that discriminator field.

node // ESQLAstNode

switch (node.kind) {
  case 'source': { // ESQLAstSource
    // ...
  }
  case 'function': { // ESQLAstFunction
    // ...
  }
  case 'from': { // ESQLAstFromCommand
    node.sources
    node.metadata
  }
  case 'metrics': { // ESQLAstMetricsCommand
    node.sources
    node.aggregates
    node.grouping
  }
  // ...
}

[drewdaemon]

Revisit expression autocomplete logic

#181916 (comment)

[dej611]

That was just a convenience shortcut due to a relaxed check on the where command in a previous version in ES.
Then it got stricter. The problem is that nothing has been documented anywhere. Probably I'd wait a little bit here as it's not harmful for the user (and easy to fix) and it still falls within the "known limitation" of the client-side validation.

[drewdaemon]

Evaluate AST structure for ease of traversal

Does it make sense to handle arrays like this? type ESQLAstItem = ESQLSingleAstItem | ESQLAstItem[];

Does it make sense to add bidirectional references between parents and children?

Does it make sense to define parent-child relationships as args or should they simply be generic?

[dej611]

Does it make sense to handle arrays like this? type ESQLAstItem = ESQLSingleAstItem | ESQLAstItem[];

There's a single case of an ESQLAstItem being an array, and that's the assign (=) operator.
That was a design decision to keep the AST type definition small and compact, and avoid the type explosion (yet another type just for the assign scenario).
If it can be solved without assigning an ad-hoc type then the array type can be removed, making the whole logic way easier to handle.

[drewdaemon]

There's a single case of an ESQLAstItem being an array, and that's the assign (=) operator.

I think it also pops up in ...| eval 1 in (1,2,3).

Not sure if that should be something hardcoded into the AST semantics or rather defined into the commands definition.

I'm not certain either. But I think Vadim's idea is worth exploring.

A unified definition construct certainly makes sense for functions because they all look the same. But, I don't know that I like the command definition being stretched further as Elasticsearch continues to get creative.

I expect the things we have to add to the command definition interface to be used in as few as a single command with no guarantee that other commands will use the same constructs. This may bloat the definitions with "generic" features that are irrelevant for most commands.

Because of this, I think that writing custom validation and autocomplete logic, as well as custom AST node types for each command may end up being preferable to continuing to expand the command definition construct.

[dej611]

I think it also pops up in ...| eval 1 in (1,2,3).

True, but I think in that scenario it actually makes more sense than for assign.

A unified definition construct certainly makes sense for functions because they all look the same....

That was exactly the situation before AST.
For node specific logic there's already the ANTLR grammar parser hook. And it provides also a walker interface too.
I've tried multiple times to leverage that before designing the AST and went nowhere.
But I would recommend to go to work on the ANTLR grammar parser level rather than expand the AST types if you want to go into the specialized route.

The AST was designed with the few specific properties:

1. simple type definitions.

• I can describe it in a sentence: Commands, Options and Functions have args, the rest of the types are values.
• this makes reason about the tree easier and resilient to the grammar changes.
• traverse is simple as there's zero knowledge required to walk.

2. easy to serialize. This has the nice side effect to make it easier to debug.

The main problem to stay closer to the grammar is that any major change at the grammar level is not necessarily considered a breaking change, as long as the user experience doesn't change. This makes it really hard to work with it to provide services.
Of course there's some challenge into the design the definitions type, but there are also some upsides as they are more flexible than the AST as callbacks can be provided.
Not being in control, nor a stakeholder, in the grammar design process brings some challenges.
Probably it is worth discussing changes at source rather than trying to bend at will once all has been already discussed.

[drewdaemon]

Thanks for the background, that's helpful.

I think there are a couple of desirable outcomes here, both of which vote in favor of the AST.

1. we manage the complexity of the language — we want ES|QL to be a joy to work with. We also need to consider the DX of working with ES|QL for teams across Kibana who will be novices, not just us.
   The AST is IMO a nice simplification of the ANTLR parse tree. It pairs it down to its most relevant information and is much easier to work with. This is good for debugging, for writing validation code, and for manipulating queries which we expect to be able to do via AST manipulation instead of string parsing once [ES|QL] AST pretty-printing #182257 lands.

2. we're shielded from non-meaningful grammar changes from the ES team — we should keep the amount of work we have to do as the language evolves to a minimum. As you pointed out, we have limited control over grammar changes and to some degree our work will be reactionary for the foreseeable future.
   A level of abstraction from the grammar is a key benefit of the AST.

Because I think the AST plays nicely into both of these goals, the question in my mind is more about the degree of abstraction the AST should provide, not whether it should exist.

Different language constructs can have a different levels of abstraction in the AST. There's nothing that forces us to be uniform.

I'm not sure on the perfect level of abstraction for commands, or even for particular commands. But, I don't foresee bespoke command nodes erasing the value of the AST.

As far as the number of node types growing, I can appreciate fewer types being simpler, but I haven't had a bad experience working with Javascript syntax trees which contain many more node types than ES|QL could ever have. Typescript really helps us here.

At the end of the day, any decision is reversible. I'm just saying I'm open to exploring Vadim's idea :)

[vadimkibana]

One way of looking at it is that, ES|QL commands are like statements in other languages. For examples, JavaScript has ExpressionStatement, BreakStatemenet, ReturnStatement, IfStatement, etc.. All parsers I've seen generate "bespoke" AST node kind for every statement. One big reason for that different statements have different types of child nodes with specific meanings, assigned by the language. (Unlike functions, where there can be infinite number of functions, arguments are always just a list and meaning of arguments is unknown at parsing time).

[drewdaemon]

Constant-only vs literal types

Right now, we have two ways to say "this parameter shouldn't accept fields." One is via the constantOnly flag, and the other is via time_literal and chrono_literal types which are supposed to be treated as if constantOnly were set to true by default. Are we okay with having two ways to do one thing?

[drewdaemon]

Experimenting with removing chrono_literal here.

[drewdaemon]

Evaluate handling of array types

The way validation and the AST handle array types is a blindspot for me (Drew) and makes me nervous.

E.g. | eval 1 in (1, 2, 3)

[drewdaemon]

Can we remove optional?

If you think about it, marking a parameter as optional is just a way of declaring two call signatures.

Say I have func(string, [number]), that is equivalent to declaring two call signatures

• func(string)
• func(string, number)

If we always expanded optional parameters into multiple call signatures, we could potentially simplify our code and better handle signatures where the optional parameter comes first (e.g. DATE_FORMAT([format,] date)).

I don't think we handle those cases very well today.

[dej611]

To me, they are not the same, at least when talking about their representation in the shared definition files.
It may make more sense as an intermediate format when using them into validation or autocomplete:

func(string, [number]) => [ func(string), func(string, number) ]

But still it requires to hold the same reference to the initial definition.
In some contexts, i.e. validation, the two representations can be overlapped, but when presenting meta information to the user I think:

func(string, [number])

is different from presenting:

func(string);
func(string, number);

I mean, one could argue that it is possible to compress the two above on-the-fly when presenting them to the user, but then what about conflicting signatures?

func(string);
func(string, number);
func(number);

Also here, it is still possible to provide some additional logic to make it understand that the first two can be compressed together while the third one is "different". But at this point the initial complexity has moved just from one place to another, perhaps becoming more "opaque".

[drewdaemon]

Yeah, nice examples @dej611 . I think it's wise to differentiate between what is shown to the user and potential validation/autocomplete logic simplification.

[vadimkibana]

Today, both—validation and autocomplete—receive a query string and parse it each into an AST. Would it be possible to generate the AST only once?

[dej611]

The query passed to the AST generator is often different in the two scenarios.
One can memoize the parser function (as it happens in tinyMath parser method), but given the average length of a ES|QL query I would imagine not particular gains.

I would propose an initial investigation on this before going this route.

Just to explain a bit, take a very simple query scenario with 2 commands:

from index

In this case both validation will parse the same thing. 1 cache hit.
Now the user keeps on typing:

from index | where field > 0

At this point validation parses the raw query.
Autocomplete at this stage probably has parsed at least 3/4 times the query with a single cache hit:

• from index | => from index cache hit?
• from index | where EDITOR_MARKER
• from index | where field => maybe cache hit? (it depends on validation execution)
• from index | where field > => from index | where field > EDITOR_MARKER
• from index | where field > 0 => cache hit

At least half the time, even in this simple scenario are a cache miss.
Depending on the caching mechanism (memoize vs memoizeOne) this can lead even to worse scenarios.

[vadimkibana]

Do we need getMessageFromId helper method in validation? What do we gain from it? Does it provide something besides error object construction?

We could use plain JS functions to construct errors, that would simplify the code (would not need TypeScript code to extract types) and work better with IDE (our editor can natively pick up argument types and provide intellisense), for example, instead of:

getMessageFromId({
  messageId: 'wrongArgumentType',
  values: {
    name: astFunction.name,
    argType: argDef.type,
    value: actualArg.name,
    givenType: columnHit!.type,
  },
  locations: actualArg.location,
})

there could simply be error helper functions:

errors.createWrongArgumentType({
  name: astFunction.name,
  argType: argDef.type,
  value: actualArg.name,
  givenType: columnHit!.type,
})

[vadimkibana]

They would still be in one place, and arguably more maintainable. Unless there is some benefit that extra code and big switch-statement provides, there is no need for it.

---

How I see it, if you have a couple of functions, you can just use them:

const foo = () => {
  // FOO
};

const bar = () => {
  // BAR
};

Unless there is some benefit for it, we don't just put them in a big switch-statement and use them indirectly:

const runFunction = (fn) => {
  switch (fn) {
    case 'foo': {
      // FOO
    }
    case 'bar': {
      // BAR
    }
  }
};

[dej611]

I think that makes it easier to extract those functions into different module, scattering the error knowledge base elsewhere.
We're trading a marginal code improvement to reviewer responsibility to make sure nobody moves them outside of a module.
This is basically a non-problem with the big switch version.

I would argue that to create an error, using a single function entry-point, and scrolling thru the function content to understand what's the best id to pick is way easier than pick a function thru intellisense.
I see how this is subjective, but here the previous point makes it even worse.

[vadimkibana]

... to make sure nobody moves them outside of a module.

We can keep all error functions in one module in either case.

... scrolling thru the function content to understand what's the best id to pick is way easier than pick a function thru intellisense.

Really? You would prefer to scroll through 400 LOC of a switch statement to find an error ID, instead of intellisense giving it to you with all the correct types and docs:

---

It is not just that tradeoff. I think it is also less code, less types, and making everything compact and readable in one file. Plus, all the autocomplete and code navigation would work. (Right now one has to do string search across files, to find where some errors are used or where it is defined—this is exactly what TypeScript was created to fix.)

[dej611]

Really? You would prefer to scroll through 400 LOC of a switch statement to find an error ID, instead of intellisense giving it to you with all the correct types and docs:

Intellisense won't tell me anything about the right id, it's just up to the naming author to give a meaningful name there. And most of the time it doesn't work.
If autocomplete is the only concern, you get the same autocomplete with the changes I've linked before:

The type inference works the same way.
That's not an unusual pattern in Typescript: it's actually recommend in TS as branded types.

[vadimkibana]

If autocomplete is the only concern ...

No, it is more than that:

It is not just that tradeoff. I think it is also less code, less types, and making everything compact and readable in one file. Plus, all the autocomplete and code navigation would work. (Right now one has to do string search across files, to find where some errors are used or where it is defined—this is exactly what TypeScript was created to fix.)

---

That's not an unusual pattern in Typescript: it's actually recommend in TS as branded types.

All of this has nothing to do with branded types.

[vadimkibana]

In AST nodes make location and text optional. As those are only available when AST is generated from parsing an ES|QL query text. If we will generate AST node programmatically, those will not be available.

[drewdaemon]

Good point

[vadimkibana]

Remove CommandOption AST node type. Instead, commands should have a dedicated "shape". For example:

// FROM
interface ESQLAstFromCommand {
  type: 'from';
  sources: ESQLAstSources[];
  metadata: ESQLAstMetadataItem[];
}

// METRICS
interface ESQLAstMetricsCommand {
  type: 'metrics';
  sources: ESQLAstSources[];
  aggregates?: ESQLAstItem[];
  grouping?: ESQLAstItem[];
}

// STATS
interface ESQLAstStatsCommand {
  type: 'stats';
  aggregates: ESQLAstItem[];
  grouping?: ESQLAstItem[];
}

// ...

[vadimkibana]

This will then allow to substitute ESQLAstItem by more precise types.

[vadimkibana]

In general, I think the rule should be that whatever is in the ES|QL grammar should be one-to-one represented in the AST. For example, functions are dynamic, there can be any number of functions; hence, there is just one entity in the grammar that represents functions, and in AST we represent it by a single type:

interface ESQLAstFunction {
  type: 'funciton';
  name: string;
}

But, commands are: (1) hard-coded in the grammar, there is a finite set of them; plus, (2) command child nodes have different semantic meanings, so for each command we need a separate type:

export type {
  ESQLAstFromCommand,
  ESQLAstMetricsCommand,
  ESQLAstStatsCommand,
  // ...
};

[drewdaemon]

Linking the discussion happening on another thread: #182393 (reply in thread)

[vadimkibana]

This might not be worth the investment, but something to keep in mind when we revisit the AST: Currently, we are missing the concept of an expression, all the different expression types and functions are represented in the AST as "function". We could have some expression types like UnaryExpression, BinaryExpression, VariadicExpression, FunctionExpression.

---

Not having a separation between functions and expressions means now and going forward we could run into collisions where an expression and a function have the same name. For example, the and boolean expression parses to an interface .name = "and", but also one can construct a function where function name parses to .name = "and" such as in METRICS foo and(1, 2, 3).

Essentially, now functions and expressions can collide. Going forward, users will be able to define custom functions, the names of custom functions might collide with the names of expressions.

[vadimkibana]

Run validation integration tests as Kibana Jest Integration tests instead of Kibana Functional tests (FTR). Currently, to run the validation tests agains the real ES instance test case fixtures are collected into a .json file during unit test run. Then an FTR test is run to re-run the same test cases by agains the a real ES instance.

I propose we run those integration tests using the Jest Integration test ability (yarn test:jest_integration ..), instead. This has two big advantages:

1. Those tests still use the Jest test runner, so we can reuse the existing Jest unit tests.
2. We will not have to generate a .json file of all test cases.

[vadimkibana]

I would be curious to hear how you would encode that logic into the integration test suite directly. How would you use the same test cases for an integration test and a Kibana validation test?

I was thinking to do it like this: here is the Jest Integration test. Note that the test suites validationFromCommandTestSuite, validationMetricsCommandTestSuite, and validationStatsCommandTestSuite are Jest test collections which without any modifications run as: (1) Jest Unit tests (here); and as (2) Jest Integration tests (in validation_consistency.test.ts).

[vadimkibana]

More on the FTR test topic:

Since last Thursday learning that the unit tests automatically populate a .json file, which is then used in FTR tests. I've been looking into how we can have separate smaller Jest unit test files for validation, yet still support running that FTR test.

1. I've tried making each Jest unit test generate its own .json of test cases and the the FTR would pick up all the files. The writeFixture generates a .json file for each separate unit test suite.
2. I've tried running each individual unit test suite inside the big validation.test.ts test suite, so that the big test suite would collect all test cases automatically as it does now.
3. I've looked into re-using the Jest test suite to run as Jest Unit tests and as Jest Integration tests, without even needing to generate the .json file of test cases. I think this is the best approach:

• This way we don't need to autogenerate the .json file.
• This way we can use the Jest Integration test runner, instead of the FTR.
• And it cleans up many things in the testing code overall, making it more readable.

On test runners: we are currently running these integration tests as FTR API integration tests. Normally those are used for HTTP API testing, which is not what we do. It looks like a better tool for us would be to use the Jest Integration tests.

[dej611]

As long as it works, I guess it's ok.
I've tried that route before but the ES endpoint was too flaky to process all those queries at once, so I had to revert to the "slower" FTR option.
I've reported that issue back to the ES team, who I guess must have fixed it.

[vadimkibana]

Another observation: I think if we want to keep a big .json file with all validation test cases we should have it the other way around. The unit tests should not auto-generate and store the file on test execution, instead we should have the file—the fixture—be the source and the unit tests (as well as the FTR tests) should just import it and loop through all the test cases.

[drewdaemon]

@vadimkibana thanks for the context. I like what you've done in your PR. I don't see a reason to keep the JSON files around anymore.

[drewdaemon]

Remove node.text properties from the AST

The node.text property is sometimes useful for understanding the portion of the query represented by any particular AST node.

But there are a couple problems with it

• it doesn't include whitespace (it's populated using the parse tree getText method instead of the TokenStream.getText — ref)
• it takes up memory

Instead, we're thinking of removing .text property and instead relying on .location whenever we need to look up a portion of the original query based on an AST node.

[vadimkibana]

Some potential problems with ANTLR grammar or our AST generation:

1. Incomplete assignment expressions are accepted and incomplete flag is set to false, for example a query like METRICS source agg1 = produces an AST where the assignment expression is a function with a single argument:

{
  type: 'function',
  name: '=',
  args: [
    { type: 'column', name: 'agg1' }
  ],
}

I would expect the assignment expression to always have exactly two operands.

---

2. In addition expression a + b, when literal is the second operand, the grammar parses it just fine, when the literal is the first argument, however, it fails to parse the query. For example, this query parses:

METRICS source agg1 + 1

but this query fails to parse:

METRICS source 1 + agg1

[drewdaemon]

ignoreAsSuggestion and alias do the same thing

Function definitions can include ignoreAsSuggestion which prevents them from being suggested in autocomplete. That is exactly the same reason we keep some functions in an array of aliases: to prevent them from being suggested. We could potentially remove the aliases in favor of ignoreAsSuggestion.

One reason not to: maybe it's nice to have the association clear between functions.

[vadimkibana]

For unary arithmetic expressions, say -column, the constructed AST is modified to create a binary expression where column is multiplied by -1. So, -column is transformed to -1 * column. Instead we should just create a unary expression function call node. We are already doing that for NOT unary expressions, like NOT column. (And for IS [NOT] NULL postfix unary expressions.)

[vadimkibana]

When IN or regex LIKE or RLIKE operators are prefixed by NOT modifier, we should keep the original operator name and add the "not" flag to the AST node, instead for changing the operator name.

Currently LIKE and NOT LIKE operators produce two different operator names:

{type: 'function', name: 'like'} // column LIKE "abc"
{type: 'function', name: 'not_like'} // column NOT LIKE "abc"

Instead we could use the same operator name but attach a not flag:

{type: 'function', name: 'like'} // column LIKE "abc"
{type: 'function', name: 'like', not: true} // column NOT LIKE "abc"

This way, when pretty printing ES|QL query from AST, we could just print the name property:

return `${node.not ? 'NOT ' : ''}${node.name}`

Now we have to loop through all possible cases:

switch (node.name) {
  case 'like': return 'LIKE';
  case 'not_like': return 'NOT LIKE';
  case 'rlike': return 'RLIKE';
  case 'not_rlike': return 'NOT RLIKE';
}

---

The same holds for the IN and NOT IN binary operators.

[vadimkibana]

There is an inconsistency where NOT IN, NOT LIKE, and NOT RLIKE binary expressions result into function names with underscore _ concatenation: not_in, not_like, and not_rlike.

{type: 'function', name: 'not_in'}
// ...

However, IS NULL and IS NOT NULL postfix unary expressions, result into function names with spaces preserved:

{type: 'function', name: 'is null'}
{type: 'function', name: 'is not null'}

[vadimkibana]

The identifier parameter in SHOW and META commands is parsed as a function with zero arguments. That is:

SHOW <identifer>
SHOW info
META <identifier>
META functions

It is parsed as

{
  type: 'command',
  name: 'show',
  args: [
    {type: 'function', name: 'info', args: []}
  ]

Note that "info" identifier is parsed as function {type: 'function'}.

This is problematic when pretty-printing, normally funciton has parenthesis after its identifier:

SHOW info()

Another problem, it does not feel right semantically for those nodes to be of type function, no? Seem those should be some form of identifier, we use column or source node types for that.

[vadimkibana]

Missing OrderExpression. In ES|QL documentation and grammar there is clearly defined OrderExpression. Used in SORT command:

FROM a | SORT <order-expression>

like

FROM a | SORT b [ ASC / DESC ] [ NULLS FIRST / NULLS LAST ]

for example

FROM a | SORT b ASC NULLS FIRST

However in Kibana ES|QL there is no such OrderExpression nodes at all. Instead the OrderExpression components are parsed as separate string literal expressions:

[
  { type: 'command', name: 'from' },
  { type: 'command', name: 'sort', args: [
    { type: 'column', name: 'b' },
    { type: 'literal', value: 'ASC' },
    { type: 'literal', value: 'NULLS' },
    { type: 'literal', value: 'FIRST' },
  ]},
]

Specifically ASC and NULLS FIRST modifiers are parsed as string literals. Also note, that the NULLS FIRST modifier is parsed as two separate NULLS and FIRST string literals.

---

The parser should parse it as a single OrderExpression node instead:

{
  type: 'order',
  order: 'asc' | 'desc',
  nulls: 'first' | 'last',
  column: {
    type: 'column',
    name: 'b',
  },
}

[vadimkibana]

We may want to introduce a root level QueryExpression node, which contains a list of commands. Currently, at the root level we just have an array of commands ESQLCommand[]. Hence, in the Visitor class we has to use this array as the root node, instead of an object with type property {type: ''} like for (almost) all other nodes.

What will this give us:

• We will standartize all nodes to be of the shape {type: ''}.
• This will allow for easier query mutation operations, for example, for inserting "filters" (adding WHERE commands). Because currently the query node is just an array, we have to mutate it inline. Once we have a QueryExpression node {type: 'query', commands: ESQLCommadn[]} we will be able to add/remove commands by overwriting the commands property, instead of now doing array slicing.
• We are currently lacking a syntax for sub-queries.
  • This will allow us to properly parse the EXPLAIN <query> command, which receives as argument a sub-query. Currently, we are not able to parse it. The parser skips the EXPLAIN command and parses the <query> instead.
  • In the future, this will let us support sub-queries in other places.

[drewdaemon]

Makes sense to me.

[vadimkibana]

In the RENAME command we do not parse out the list of rename expressions:

RenameCommand = "RENAME" RenameExpression ("," RenameExpression)*

RenameExpression = Identifier "AS" Identifier

Which would have the AST:

{
  type: 'command',
  name: 'rename',
  args: [
    { type: 'rename-expression' },
    { type: 'rename-expression' },
    // ...
  ]
}

Instead we are parsing the rename expressions as many AS command options. However, those are not command options, and there should normally be a single command option per name.

Like

FROM ... METADATA ...
STATS ... BY ...

Above METADATA and BY options are specific to the command and are allowed to be defined only once.

But in the RENAME command we get many AS options.

---

If FROM and STATS command would follow the logic from the RENAME command, then in FROM command its list of arguments instead of sources (columns) would be a list of METADATA options.

Instead of:

{
  type: 'command',
  name: 'from',
  args: [
    { type: 'column' },
    // ...
  ]
}

it would be:

{
  type: 'command',
  name: 'from',
  args: [
    { type: 'option', name: 'metadata', args: [
      { type: 'column' }
    ]},
    // ...
  ]
}

Similar for the STATS command.

[vadimkibana]

I will try to explain it more: each command has a default list of arguments—lets call it <default>—and can have named lists of arguments (we call named lists options).

For example the below FROM command

FROM index1, index2 METADATA _id, _source

would have the <default> and METADATA argument lists:

{
  '<default>': ['index1', 'index2'],
 'METADATA': ['_id', '_source'],
}

This is the case for all commands, as far as I know. With the only exception being the RENAME command:

1. It does not have the default list of RenameExpression arguments.
2. Instead it has many lists of named AS arguments.

So, the following RENAME command

RENAME a AS b, c AS d

would have many AS lists, and no default list, something like this:

[
  {
    '<default>': [],
   'AS': ['a', 'b'],
  },
  {
    '<default>': [],
   'AS': ['c', 'd'],
  }
]

Instead, it should be just a single <default> list of RenameExpression`s:

{
  '<default>': [
    { type: 'rename-expression', args: ['a', 'b'] },
    { type: 'rename-expression', args: ['c', 'd'] },
  ],
}

[drewdaemon]

The grammar around sources and field names is very loose, leaving a lot of cases to cover. Some cases exist because they were anticipated to be useful to the user. Others exist because it was easier that way in the ANTLR grammar. So, we should support likely cases, and only invest in supporting accidental edge cases if it is super easy or a natural consequence of an architectural improvement.

High priority

event.dataset
`event.dataset`
`event`.`dataset`
remote:cluster
"remote:cluster"
"""remote:cluster"""

Low priority

`event` . `dataset`
event . dataset
event . /* comment */ dataset
remote : cluster
remote : /* comment */ cluster

[dej611]

It would be nice to have some ability to extend the validation engine with custom rules, either loaded statically from a registry or at runtime.

That would be to improve the ES|QL usability within specific use cases as follow:

• when creating a visualization a special validation rule may detect queries who lead to "non-visualizable" result
  • i.e. from index | stats count() by @timestamp <= this creates a time series with a 1ms bucketing for the x axis
  • it would be nice highlight
• when creating an alert the query result would likely be a boolean/number?

It would be nice to be able to specify the validation rule together with a "fixing" logic as well: take the visualization query above, a "Quick fix" action already in place would make it easier for the user to get the right result without going too deep into both ES|QL and charting knowledge.

[drewdaemon]

I could see this being useful in autocomplete as well. E.g. when building a visualization you get useful snippet suggestions.

[dej611]

Yes, agree.
In the visualization query above, after by probably bucket suggestion weight can get a boost given the context rather than other things.

[drewdaemon]

In Elasticsearch, stages of query processing are cleanly separated. The ones relevant to us are

• Parsing — Using ANTLR to build an AST (they call it a "logical plan")
• Pre-analysis — fetching data about the index and available fields
• Analysis — resolving identifiers (like field names) to a type, detecting unresolved identifiers
• Verification — make sure all types are compatible with the expressions, etc

We could take inspiration from this architecture. We could resolve all fields and variables in the AST, giving each a type in the AST itself. Then, the fields map would no longer need to be passed around the validation code.

[drewdaemon]

Hide the editor marker

The editor marker is appended to queries to make them valid-enough to be parsed into AST. However, it is then present in the AST sometimes as its own node, sometimes as an appendage to another, valid node (e.g. field_nameesql_editor_marker

There is some existing code that attempts to walk the AST and remove the editor marker, but it doesn't cover all cases and was built before the Walker interface.

We should use Walker.find to detect the presence of the editor marker right after the AST is created and remove it. This will prevent logic around the marker leaking into various areas of the autocomplete code.

[stratoula]

This has been decided as something we want to do. Here is the issue #215186

[drewdaemon]

Reconsider function classifications

=, WHERE (when used within STATS), INFO (as in SHOW INFO, and AS (in RENAME and JOIN) are all parsed as functions in our AST. This makes it so that we have to have operator definitions for them because otherwise the validator sees them as unknown functions.

I think these should be recategorized.

• WHERE and INFO should be options
• AS could be an option
• = — not sure, maybe it should stay as an operator or should be a special expression type AssignmentExpression

cc @vadimkibana for thoughts.

[drewdaemon]

Boolean expressions always collected as arrays

For unknown reasons, "boolean expressions" are always collected as arrays in our AST. We shouldn't do this.

kibana/src/platform/packages/shared/kbn-esql-ast/src/parser/walkers.ts

Lines 513 to 526 in 8203ebf

	export function collectBooleanExpression(ctx: BooleanExpressionContext | undefined): ESQLAstItem[] {
	const ast: ESQLAstItem[] = [];
	if (!ctx) {
	return ast;
	}
	return ast
	.concat(
	collectLogicalExpression(ctx),
	collectRegexExpression(ctx),
	collectIsNullExpression(ctx),
	collectDefaultExpression(ctx)
	)
	.flat();
	}