[ES|QL] Separate `EVAL` autocomplete routine #212996

drewdaemon · 2025-03-03T21:58:08Z

Summary

Part of #195418

Gives EVAL autocomplete logic its own home 🏡

Expression suggestions function

This PR also introduces a semi-generic function for generating suggestions within an expression. This is so that the logic can be shared between EVAL and WHERE. It also gets us closer to supporting filtering in STATS (#195363).

To make this happen, I took stock of where we have differences in our expression suggestions between WHERE and EVAL. In some cases, the differences seemed important. In other cases, I felt ok removing them.

EVAL

Behavior	Plan
Suggests pipe and comma after complete column names (`column/` or `column /`)	get rid of it because an expression consisting of just a single column name is essentially useless in `EVAL`
Doesn't suggest fields after an assignment	get rid of it. why act any different than an expression not assigned an alias?
Suggests assignment operator after new column name (`newColumn /`)	keep it
Suggests assignment snippet for empty expression	keep it
Suggests time literal completions after literal number in assignment (`newColumn = 1 /`)	remove it. it doesn't feel that useful and removing it makes it easier to have a generic expression suggestions function. It will still be around in functions and operators (e.g. `1 day + 2 /`).
Supports multiple expressions	keep it

WHERE

Behavior	Plan
Suggests pipe after complete boolean expression (`foo AND bar /`)	keep it, but outside of the expression suggestion function
Suggests boolean operators to make a boolean expression (`timestamp > "2002" AND doubleField /`)	keep it... maybe we're being too smart but we can always remove it later

Other changes

the suggestions for CASE(foo != /) used to differ based on the trigger kind. This seemed inadvertent so I removed the difference.
we now add spaces after fields that are inserted in expressions. E.g. WHERE foo + <insert field><space>. I'm not sure if this is best or not...

Checklist

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

Identify risks

As with any refactor, there's a possibility this will introduce a regression in the behavior of commands. However, all automated tests are passing and I have tested the behavior manually and can detect no regression.

…eparate-enrich

elasticmachine · 2025-03-14T02:41:09Z

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

stratoula

This looks nice, I have added some questions to understand better some decisions

stratoula · 2025-03-14T07:47:08Z

...tform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts

@@ -1036,6 +1037,8 @@ describe('autocomplete', () => {
      'AND $0',
      'NOT',
      'OR $0',
+      ', ',


Why comma and pipe got added here?

"IS NOT N" gets parsed into our AST as a complete "IS NOT NULL" (see #199401). Because the AST is complete, it detects this as a complete expression.

I don't think it will matter to the user, but I will double check.

Ok I guess I do need to fix this

Done in 0e6fc40

I realized this was an existing bug in WHERE too, so they're both fixed now.

stratoula · 2025-03-14T07:49:30Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts

+  // e.g. CASE(field != /)
+  //
+  // So, it is handled as a special branch...
+  if (


Can this branch be moved inside the eval autocomplete as it is an eval function?

I will check... I'm pretty sure you can use it other places.

I do agree that this is not a good long-term home for it but this is actually where it was before. It was just hidden in the generic code. I am doing some thinking about the role of function suggestion routines in relation to the command functions.

Yeah, for example if we moved it to EVAL, we'd lose the right behavior in SORT

WDYT

Mmmm I see 🤔 Yes leave it there but we need to rethink it in the future

I agree. Consider this refactor in flux...

stratoula · 2025-03-14T07:56:40Z

...orm/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/commands/eval/index.ts

+    commandName: 'eval',
+  });
+
+  // EVAL-specific stuff


Don't get this comment here

I got it, these are the code that eval does differently 👍

Done in 0e6fc40

stratoula · 2025-03-14T07:57:42Z

...rm/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/commands/where/index.ts

-
-      break;
-  }
+  // WHERE-specific stuff


Same here 🤔

Update: This is where you differentiate where 👍

I'll remove the comment. I think it is confusing—you're already in where/index.ts—of course it is WHERE-specific

yes and the eval too, I think this is why I got confused in the first place

Done in 0e6fc40

stratoula · 2025-03-14T08:13:17Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/helper.ts

+
+      break;
+
+    case 'empty_expression':


So I understand that this is a very common code between eval and where and why we decided to move everything in one helper. But we still need to differentiate between where and eval as there are functions that do not exist in eval. I wonder if we can keep the split here and share code only when necessary. Wdyt?

Sure, I'm open to duplicating the contents of this function between WHERE and EVAL if that's what you mean?

The reasons I was thinking a shared function with customization through arguments were

suggestions within an expression (by "expression" I mean some values plus some operators) are not very different between the two commands

having a shared function will make it easier to extend this functionality to other expressions within ES|QL

For example, ES|QL allows expressions in

SORT

STATS ... BY

STATS ... WHERE

within function parameters

We don't currently give help in any of the above contexts, with the exception of the first argument in CASE. We know we want to extend to STATS...WHERE and we may want to extend to more of these.

Let me know what you want me to do.

There are yes and I understand why you decided to go that way. It is just the fact that there will always be some differences:

STATS ... WHERE doesnt support full text search

SORT neither

EVAL has a functionality that doesnt exist in WHERE etc

How will this look as the language will evolve? More if else? I don't know, I am mostly wondering if this approach will suit us or we will need to change it in the future

I wasn't really suggesting to duplicate everything. I was just wondering if maybe we can share code but have the differences to be set in the distinct code. So for example:

search functions and the match operator is a where thingy. Can we set them up in the where code and pass it to the shared one in order to make the shared a bit more generic?

Ok then we are on the same page. I agree that the commands should be able to configure expression suggestion behavior based on context and we can do this better than it is happening now. For example, have the suggestion routines build a set of configuration parameters that they pass to the expression suggestions function

getExpressionSuggestions(... { suggestFunctions: () => { only suggest functions supported in this context }, preferredExpressionType: 'boolean'... })

In a similar vein we should definitely

clean up the "function argument suggestions function" as well—it is littered with special cases and command awareness, etc.,

possibly put function suggestions within the command routines instead of as its own separate top-level branch

get the editor marker under control

remove options and settings and other defunct stuff

But I want to scope this PR to just moving EVAL stuff to its own place. When I start diving into side-quests in these PRs (for example, removing all command-awareness from all the sub functions in getExpressionSuggestions), they tend to get very unmanageable. I know the file counts aren’t large here, but untangling the logic is very complicated.   So, I agree with your direction and I want to clean up more, but I would rather focus here on first putting per-command boundaries on the craziness. 🤪

Now that we actually know what all this logic does, we can evolve it however we need to with very little refactoring

ok I am fine with it

You'll like what you see when I take care of #209359

stratoula · 2025-03-14T08:16:40Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/definitions/commands.ts

@@ -243,6 +245,7 @@ export const commandDefinitions: Array<CommandDefinition<any>> = [
    modes: [],
    // Reusing the same validation logic as stats command
    validate: statsValidator,
+    suggest: suggestForStats,


Is this added to add autocomplete support to inlinestats? If yes it doesnt seem to work as expected

Thanks... actually what is the status of inlinestats? I don't see it in our docs...

Inlinestats is paused atm in favor of joins. Let's remove it the suggest here and deal with it when inlinestats is back in the roadmap.

Done in 0e6fc40

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/definitions/types.ts

drewdaemon · 2025-03-17T18:02:08Z

@elasticmachine merge upstream

elasticmachine · 2025-03-17T19:42:56Z

💚 Build Succeeded

Buildkite Build
Commit: 4f25527

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/esql-validation-autocomplete`	186	188	+2

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/esql-validation-autocomplete`	12	13	+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`	3.6MB	3.6MB	-3.2KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/esql-validation-autocomplete`	198	200	+2

Unreferenced deprecated APIs

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

History

💚 Build #283932 succeeded d8305fa

stratoula

Looks nice,k thanx for addressing the comments and the brainstorming

stratoula · 2025-03-18T07:11:44Z

...tform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts

@@ -1008,10 +1009,6 @@ describe('autocomplete', () => {
      'AND $0',
      'NOT',
      'OR $0',
-      // pipe doesn't make sense here, but Monaco will filter it out.


kibanamachine · 2025-03-18T12:25:07Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13923125733

## Summary Part of elastic#195418 Gives `EVAL` autocomplete logic its own home 🏡 ### Expression suggestions function This PR also introduces a semi-generic function for generating suggestions within an expression. This is so that the logic can be shared between `EVAL` and `WHERE`. It also gets us closer to supporting filtering in `STATS` (elastic#195363). To make this happen, I took stock of where we have differences in our expression suggestions between `WHERE` and `EVAL`. In some cases, the differences seemed important. In other cases, I felt ok removing them. #### EVAL | Behavior | Plan | |--------------------------------------------------------------------------|------| | Suggests pipe and comma after complete column names (`column/` or `column /`)| get rid of it because an expression consisting of just a single column name is essentially useless in `EVAL` | | Doesn't suggest fields after an assignment | get rid of it. why act any different than an expression not assigned an alias? | | Suggests assignment operator after new column name (`newColumn /`) | keep it | | Suggests assignment snippet for empty expression | keep it | | Suggests time literal completions after literal number in assignment (`newColumn = 1 /`) | remove it. it doesn't feel that useful and removing it makes it easier to have a generic expression suggestions function. It will still be around in functions and operators (e.g. `1 day + 2 /`). | | Supports multiple expressions | keep it | #### WHERE | Behavior | Plan | |--------------------------------------------------------------------------|------| | Suggests pipe after complete boolean expression (`foo AND bar /`) | keep it, but outside of the expression suggestion function | | Suggests boolean operators to make a boolean expression (`timestamp > "2002" AND doubleField /`) | keep it... maybe we're being too smart but we can always remove it later | ### Other changes - the suggestions for `CASE(foo != /)` used to differ based on the trigger kind. This seemed inadvertent so I removed the difference. - we now add spaces after fields that are inserted in expressions. E.g. `WHERE foo + <insert field><space>`. I'm not sure if this is best or not... ### 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 ### Identify risks - [ ] As with any refactor, there's a possibility this will introduce a regression in the behavior of commands. However, all automated tests are passing and I have tested the behavior manually and can detect no regression. --------- Co-authored-by: Stratoula Kalafateli <[email protected]> Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit 74c31fb)

kibanamachine · 2025-03-18T12:30:47Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Separate `EVAL` autocomplete routine (#212996)](#212996)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Drew Tate <[email protected]>

## Summary Part of elastic#195418 Gives `EVAL` autocomplete logic its own home 🏡 ### Expression suggestions function This PR also introduces a semi-generic function for generating suggestions within an expression. This is so that the logic can be shared between `EVAL` and `WHERE`. It also gets us closer to supporting filtering in `STATS` (elastic#195363). To make this happen, I took stock of where we have differences in our expression suggestions between `WHERE` and `EVAL`. In some cases, the differences seemed important. In other cases, I felt ok removing them. #### EVAL | Behavior | Plan | |--------------------------------------------------------------------------|------| | Suggests pipe and comma after complete column names (`column/` or `column /`)| get rid of it because an expression consisting of just a single column name is essentially useless in `EVAL` | | Doesn't suggest fields after an assignment | get rid of it. why act any different than an expression not assigned an alias? | | Suggests assignment operator after new column name (`newColumn /`) | keep it | | Suggests assignment snippet for empty expression | keep it | | Suggests time literal completions after literal number in assignment (`newColumn = 1 /`) | remove it. it doesn't feel that useful and removing it makes it easier to have a generic expression suggestions function. It will still be around in functions and operators (e.g. `1 day + 2 /`). | | Supports multiple expressions | keep it | #### WHERE | Behavior | Plan | |--------------------------------------------------------------------------|------| | Suggests pipe after complete boolean expression (`foo AND bar /`) | keep it, but outside of the expression suggestion function | | Suggests boolean operators to make a boolean expression (`timestamp > "2002" AND doubleField /`) | keep it... maybe we're being too smart but we can always remove it later | ### Other changes - the suggestions for `CASE(foo != /)` used to differ based on the trigger kind. This seemed inadvertent so I removed the difference. - we now add spaces after fields that are inserted in expressions. E.g. `WHERE foo + <insert field><space>`. I'm not sure if this is best or not... ### 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 ### Identify risks - [ ] As with any refactor, there's a possibility this will introduce a regression in the behavior of commands. However, all automated tests are passing and I have tested the behavior manually and can detect no regression. --------- Co-authored-by: Stratoula Kalafateli <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

drewdaemon and others added 22 commits February 6, 2025 16:42

Merge branch 'main' of github.com:elastic/kibana

0e937a9

Merge branch 'main' of github.com:elastic/kibana

7ffbf4c

Merge branch 'main' of github.com:elastic/kibana

b426318

Merge branch 'main' of github.com:elastic/kibana

511ccaa

Merge branch 'main' of github.com:elastic/kibana

2d71300

add enrich tests

9b10195

get policies and modes working

20105dd

add back ENRICH_MODES 🙄

5ce64ad

suggests subcommands after policy defined

96a3f25

support ON

6cf1f0d

support WITH clause

c912d26

better support for partial strings

3c0f966

Merge branch 'main' of github.com:elastic/kibana into separate-enrich

43076b4

remove with logic

fe50c29

fix bugs with enrichment fields

29dc199

fix a few corner cases

bb5e2e8

Merge branch 'main' of github.com:elastic/kibana into separate-enrich

dda9177

update test

b55412f

Merge branch 'main' into separate-enrich

8aaec06

Merge branch 'main' of github.com:elastic/kibana into separate-enrich

15c9638

Merge branch 'separate-enrich' of github.com:drewdaemon/kibana into s…

fbde129

…eparate-enrich

EVAL

e04ea0d

drewdaemon added release_note:skip Skip the PR/issue when compiling release notes Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Mar 3, 2025

drewdaemon added 2 commits March 4, 2025 14:20

Merge branch 'main' of github.com:elastic/kibana into separate-eval

87594e1

sharing expression suggestion routine

8203ebf

drewdaemon added 10 commits March 13, 2025 12:25

Merge branch 'main' of github.com:elastic/kibana into separate-eval

16b2224

remove deprecated expression suggestion routine

69e3537

fix the definitions

276ae90

fix the definitions

90b43a8

sort imports

90b973d

fix after-assignment scenario

89fb14a

add a few more test cases

97f7725

fix types

67ba3e6

improve CASE suggestions

1c16f1d

a little organization

d8305fa

drewdaemon marked this pull request as ready for review March 14, 2025 02:41

drewdaemon requested a review from a team as a code owner March 14, 2025 02:41

stratoula reviewed Mar 14, 2025

View reviewed changes

drewdaemon added 2 commits March 17, 2025 11:55

PR feedback

0e6fc40

fix ordering issue

7c4a950

Merge branch 'main' into separate-eval

4f25527

drewdaemon requested a review from stratoula March 17, 2025 18:02

stratoula approved these changes Mar 18, 2025

View reviewed changes

drewdaemon merged commit 74c31fb into elastic:main Mar 18, 2025
9 checks passed

kibanamachine mentioned this pull request Mar 18, 2025

[8.x] [ES|QL] Separate EVAL autocomplete routine (#212996) #214968

Merged

drewdaemon mentioned this pull request Mar 25, 2025

[ES|QL] Fields shown below functions in WHERE #196576

Closed

[ES|QL] Separate EVAL autocomplete routine #212996

[ES|QL] Separate EVAL autocomplete routine #212996

Conversation

drewdaemon commented Mar 3, 2025 • edited by kibanamachine Loading

Summary

Expression suggestions function

EVAL

WHERE

Other changes

Checklist

Identify risks

elasticmachine commented Mar 14, 2025

stratoula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stratoula Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewdaemon Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewdaemon commented Mar 17, 2025

elasticmachine commented Mar 17, 2025

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

Page load bundle

API count

Unreferenced deprecated APIs

History

stratoula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 18, 2025

kibanamachine commented Mar 18, 2025

💚 All backports created successfully

Questions ?

[ES|QL] Separate `EVAL` autocomplete routine #212996

[ES|QL] Separate `EVAL` autocomplete routine #212996

drewdaemon commented Mar 3, 2025 •

edited by kibanamachine

Loading

stratoula Mar 14, 2025 •

edited

Loading

drewdaemon Mar 14, 2025 •

edited

Loading