[ES|QL] Fix autocomplete with incompatible args #180874

dej611 · 2024-04-16T08:45:40Z

Summary

Fixes #180730

I've improved the autocomplete logic a bit here with the new features from main:

the constantOnly check has been pushed down rather than depend on an empty string
autosuggestion takes into account the previous argument types now to propose the next one

Checklist

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

elasticmachine · 2024-04-16T08:45:43Z

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

drewdaemon · 2024-04-16T13:54:24Z

Awesome—glad to see this. But, you may have to pull in main because auto_bucket looks quite different now.

dej611 · 2024-04-16T15:37:12Z

Synced with main and improved the logic there with b70e9bf.
Description updated.

drewdaemon

The original bug is definitely fixed. Many thanks for that.

I'm not so sure on the new suggestions. On the one hand, I like that they are another indicator of the type of the parameter.

On the other hand, I'm concerned they'll be difficult to interpret.

Take these number suggestions here

Does this mean that those are the only acceptable values? That's what the suggestions often mean, including when the user types mv_sort(field, and gets asc and desc.

Why 10, 20, 30? Are these good choices for bucket? What about for other functions in the future?

Or here, where did this date come from? Why is it a good suggestion? Does it matter which of the two I pick?

Given this, I would prefer we scope this PR to the bugfix #180730 so we can get that in for 8.14.

dej611 · 2024-04-17T07:07:37Z

Does this mean that those are the only acceptable values? That's what the suggestions often mean, including when the user types mv_sort(field, and gets asc and desc.

We had a similar logic for limit where we propose only 3 values. I think user understands that the options are not limited for that. Same applies for date math where only the single unit is proposed, rather than any multiple of them.

ASC and DESC are an enum case like chrono_literals.

Or here, where did this date come from? Why is it a good suggestion? Does it matter which of the two I pick?

This is implementing a similar logic as in formula absolute time shift. But here I've limited to the current time rather than propose any possible shift. I can see how this is a bit more opinionated, but while testing it I found it convenient as a quick placeholder rather than typing the full ISO date manually.

dej611 · 2024-04-17T07:09:26Z

The problem with removing the suggestions is that without them it feels the autocomplete is broken as nothing will be then suggested for those argument, as with the fix the constantsOnly is enforced in the suggestion system.

drewdaemon · 2024-04-17T13:51:17Z

The problem with removing the suggestions is that without them it feels the autocomplete is broken as nothing will be then suggested for those argument, as with the fix the constantsOnly is enforced in the suggestion system.

I see what you mean. We suggest for pretty much everything else, so this is more jarring. But is it the end of the world? The user still has the inline documentation. The validation is still correct.

I'm not saying that constant suggestions are a bad idea. I'm just saying, why not take them as an enhancement request separate from this bugfix that we want in 8.14? That way, we can have time to evaluate the request, and if we decide to add, we can think through the cases and add them with some polish.

I think that there are other solutions to this problem as well. For example, #180528 would also address this, by giving the user a suggestion of the data type, if not the actual values.

But, cc @stratoula for a second opinion.

dej611 · 2024-04-17T14:00:07Z

I think that there are other solutions to this problem as well. For example, #180528 would also address this, by giving the user a suggestion of the data type, if not the actual values.

I've implemented the signature help in the past (another issue here) - it's just 3 lines of code - and it doesn't solve the problem as it's only available for multiline editor (due to #166085 ).

drewdaemon · 2024-04-17T14:55:25Z

I've implemented the signature help in the past (another issue #166089) - it's just 3 lines of code - and it doesn't solve the problem as it's only available for multiline editor (due to #166085 ).

All decisions can be reversed 🤷

drewdaemon · 2024-04-17T15:10:52Z

Synced offline and decided with @stratoula and @dej611 to pursue the constant suggestions but to split them out into their own PR. This will allow us the time to discuss the particulars while allowing this bug to be fixed.

drewdaemon

Works great in the browser but I think the tests are passing by accident.

You can see the problem by adding a conditional breakpoint (statement === 'from a | eval st_intersects(field, )') at autocomplete.test.ts:253.

The expected suggestions should be scoped down to either cartesian or geo points but instead we expect everything. The reason the test passes is that it generates no suggestions [].

All the assertions are nested in a for loop on suggestions so it being empty causes no assertions to be run. That makes the test pass.

We should probably

update the expected suggestions to be correct
figure out why the suggest fn is returning an empty array
add an array length assertion so that we don't get any more of these quiet failures

Another kind of strange thing (maybe not related to the PR) is that when I add a second comma, I get a list of everything as a suggestion

Screen.Recording.2024-04-18.at.7.59.05.AM.mov

drewdaemon · 2024-04-18T13:30:31Z

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

+          // @TODO: improve this to inherit the constant flag from the outer function
+          functions: !shouldBeConstant,
+          fields: !shouldBeConstant,
          variables: variablesExcludingCurrentCommandOnes,
+          literals: shouldBeConstant,


No, I think it still makes sense to pass it down.

drewdaemon · 2024-04-18T13:31:10Z

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

+    const shouldBeConstant = validSignatures.some(({ params }) => params[argIndex]?.constantOnly);
+


dej611 · 2024-04-18T14:26:32Z

All the assertions are nested in a for loop on suggestions so it being empty causes no assertions to be run. That makes the test pass.

Good catch. Will fix it.

dej611 · 2024-04-18T14:59:14Z

Addressed all issues raised.
I'm not really a fan of the current experience with bucket which leaves the user on its own after the first argument, in particular for the 3rd and 4th arg.

drewdaemon · 2024-04-18T15:38:12Z

Sorry, I still don't think the test is working correctly... maybe I'm missing something.

The signatures for st_intersects are the following

So, cartesian and geo are never mixed.

If you look at the expected suggestions that are generated for "from a | eval st_intersects(field, )", they include both cartesian and geo, even though the first field has already been populated.

Shouldn't we have just one or the other (geo or cartesian) depending on the type of field?

I'm not really a fan of the current experience with bucket which leaves the user on its own after the first argument, in particular for the 3rd and 4th arg.

Agreed. Are you still planning on adding the new suggestions in another PR?

dej611 · 2024-04-18T16:32:15Z

Shouldn't we have just one or the other (geo or cartesian) depending on the type of field?

The bug is in the test, as the passed field in the from a | eval st_intersects(field, ) doesn't hold any type information, so the existingTypes is always empty. Adding it here is not something quick, so maybe in a followup is best. I think validation testing has already some code to do it that can be borrowed.

Agreed. Are you still planning on adding the new suggestions in another PR?

It depends on the time frame and how much it would be quick to be defined and added.

drewdaemon

Okay, I will fix the test in another PR. Thanks!

kibana-ci · 2024-04-22T09:37:27Z

💚 Build Succeeded

Buildkite Build
Commit: e94490c

Metrics [docs]

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.9MB	2.9MB	+392.0B

History

💚 Build #204666 succeeded 1d354fc
💚 Build #204570 succeeded e3d8bba
💔 Build #204547 failed a500c9f
💚 Build #204138 succeeded 5e68215
💚 Build #203927 succeeded 5401ce6

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

## Summary Fixes elastic#180730 I've improved the autocomplete logic a bit here with the new features from `main`: * the `constantOnly` check has been pushed down rather than depend on an empty string * autosuggestion takes into account the previous argument types now to propose the next one ### 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 (cherry picked from commit eadc173)

kibanamachine · 2024-04-22T10:39:45Z

💚 All backports created successfully

Status	Branch	Result
✅	8.14

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

Questions ?

Please refer to the Backport tool documentation

…1289) # Backport This will backport the following commits from `main` to `8.14`: - [[ES|QL] Fix autocomplete with incompatible args (#180874)](#180874)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

dej611 added 2 commits April 16, 2024 10:44

🐛 Suggest only compatible signature args

7ee39c2

✅ Add tests

5401ce6

dej611 added release_note:fix Feature:ES|QL ES|QL related features in Kibana v8.14.0 Team:ESQL ES|QL related features in Kibana labels Apr 16, 2024

dej611 requested a review from a team as a code owner April 16, 2024 08:45

dej611 added 4 commits April 16, 2024 16:07

Merge remote-tracking branch 'upstream/main' into fix/180730

3f34d82

✨ Improve literals suggestions

b70e9bf

✅ Improve checks

2673ea0

Merge remote-tracking branch 'upstream/main' into fix/180730

5e68215

drewdaemon reviewed Apr 16, 2024

View reviewed changes

dej611 added backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) v8.15.0 labels Apr 18, 2024

dej611 and others added 3 commits April 18, 2024 09:36

👌 Integrate feedback

258c230

🔥 Removed new suggestions

a500c9f

Merge branch 'main' into fix/180730

e3d8bba

drewdaemon reviewed Apr 18, 2024

View reviewed changes

dej611 added 2 commits April 18, 2024 16:57

🐛 Fix issues on following args

d3d1fdc

✅ Fix test

1d354fc

drewdaemon approved these changes Apr 18, 2024

View reviewed changes

Merge branch 'main' into fix/180730

e94490c

dej611 merged commit eadc173 into elastic:main Apr 22, 2024
16 checks passed

dej611 deleted the fix/180730 branch April 22, 2024 10:35

kibanamachine mentioned this pull request Apr 22, 2024

[8.14] [ES|QL] Fix autocomplete with incompatible args (#180874) #181289

Merged

szaffarano mentioned this pull request Jul 12, 2024

[8.14] [Telemetry][Security Solution] Use the proper index to query builtin alerts (#187859) #188233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Fix autocomplete with incompatible args #180874

[ES|QL] Fix autocomplete with incompatible args #180874

dej611 commented Apr 16, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Apr 16, 2024

drewdaemon commented Apr 16, 2024

dej611 commented Apr 16, 2024

drewdaemon left a comment •

edited

Loading

dej611 commented Apr 17, 2024

dej611 commented Apr 17, 2024

drewdaemon commented Apr 17, 2024 •

edited

Loading

dej611 commented Apr 17, 2024

drewdaemon commented Apr 17, 2024

drewdaemon commented Apr 17, 2024 •

edited

Loading

drewdaemon left a comment

drewdaemon Apr 18, 2024

dej611 Apr 18, 2024

drewdaemon Apr 18, 2024

dej611 commented Apr 18, 2024

dej611 commented Apr 18, 2024

drewdaemon commented Apr 18, 2024 •

edited

Loading

dej611 commented Apr 18, 2024

drewdaemon left a comment

kibana-ci commented Apr 22, 2024

kibanamachine commented Apr 22, 2024

		const shouldBeConstant = validSignatures.some(({ params }) => params[argIndex]?.constantOnly);

[ES|QL] Fix autocomplete with incompatible args #180874

[ES|QL] Fix autocomplete with incompatible args #180874

Conversation

dej611 commented Apr 16, 2024 • edited by kibanamachine Loading

Summary

Checklist

elasticmachine commented Apr 16, 2024

drewdaemon commented Apr 16, 2024

dej611 commented Apr 16, 2024

drewdaemon left a comment • edited Loading

Choose a reason for hiding this comment

dej611 commented Apr 17, 2024

dej611 commented Apr 17, 2024

drewdaemon commented Apr 17, 2024 • edited Loading

dej611 commented Apr 17, 2024

drewdaemon commented Apr 17, 2024

drewdaemon commented Apr 17, 2024 • edited Loading

drewdaemon left a comment

Choose a reason for hiding this comment

drewdaemon Apr 18, 2024

Choose a reason for hiding this comment

dej611 Apr 18, 2024

Choose a reason for hiding this comment

drewdaemon Apr 18, 2024

Choose a reason for hiding this comment

dej611 commented Apr 18, 2024

dej611 commented Apr 18, 2024

drewdaemon commented Apr 18, 2024 • edited Loading

dej611 commented Apr 18, 2024

drewdaemon left a comment

Choose a reason for hiding this comment

kibana-ci commented Apr 22, 2024

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

kibanamachine commented Apr 22, 2024

💚 All backports created successfully

Questions ?

dej611 commented Apr 16, 2024 •

edited by kibanamachine

Loading

drewdaemon left a comment •

edited

Loading

drewdaemon commented Apr 17, 2024 •

edited

Loading

drewdaemon commented Apr 17, 2024 •

edited

Loading

drewdaemon commented Apr 18, 2024 •

edited

Loading