[ES|QL] auto bucket and constant-only parameter improvements by drewdaemon · Pull Request #180509 · elastic/kibana

drewdaemon · 2024-04-10T17:29:51Z

Summary

Close #180061

Key fixes

auto_bucket
- now just called bucket as of ESQL: Rename AUTO_BUCKET to just BUCKET elasticsearch#107197
- now accepts dates for the 3rd and 4th parameters
- now accepts a number field for the first parameter
Constant-only parameters
- We no longer suggest fields or variables for auto-bucket's 2nd, 3rd, and 4th parameters, all of which must be constants
- Validator catches non-constants even if they are nested in layers of functions (e.g. auto_bucket(@timestamp, abs(length(bytes)), "", ""))

Outstanding issues

Validation catches fields in a constant-only parameter spot, but not if they are nested in a function

Validator sees the problem

Validator doesn't see the problem

Checklist

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

…ket-should-accept-date

drewdaemon · 2024-04-11T13:26:54Z

/ci

elasticmachine · 2024-04-11T15:11:37Z

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

drewdaemon · 2024-04-11T15:17:09Z

One open question: how do we want the highlighting and messaging to work in the case of a nested non-constant?

e.g. given auto_bucket(@timestamp, avg(length(bytes)), ...)

only bytes is currently highlighted and the message reads

Argument of [length] must be a constant, received [bytes]

Questions

What do we want to highlight? Just the field or the whole argument to auto_bucket?
What do we want the message to be? I could see the current message being confusing because length isn't actually where the constant-only constraint is coming from.

WDYT @stratoula ?

stratoula · 2024-04-12T06:17:39Z

@drewdaemon thanx, I played with it and seems really nice

What do we want to highlight?

Yes the entire argument, otherwise is weird

The messaging

Yes it is confusing

I suggest something simple as this argument for bucket should be a constant and highlight the entire argument

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

dej611 · 2024-04-12T08:08:23Z

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

  name: string;
  description: string;
-  args?: Array<{ name: string; type: string; value: string; literalOnly?: boolean }>;
+  args?: Array<{ name: string; type: string; value: string; constantOnly?: boolean }>;


why this renaming?

In ES|QL there's the concept of literal but not of constant (which is an Monaco concept).

Actually, I was hoping this change would align us closer with ES since they use "constant" in their error messages. Wasn't aware of the Monaco thing.

[esql] > Unexpected error from Elasticsearch: verification_exception - Found 1 problem line 1:51: second argument of [bucket(bytes, bytes, 30, 20)] must be a constant, received [bytes]

WDYT?

Yes makes sense 👍

drewdaemon · 2024-04-12T20:29:24Z

@stratoula I've condensed the error messages and highlighting so they make more sense.

To your other point

I suggest something simple as this argument for bucket should be a constant

I think this could be a nice improvement.

Right now, all the error messages follow a similar pattern (see packages/kbn-esql-validation-autocomplete/src/validation/errors.ts). Because of this, I think that if we want to revisit this messaging, we should do it holistically as a separate effort. WDYT?

Also, maybe @dej611 has some input on the thinking behind the way the error messages are currently formatted?

dej611 · 2024-04-15T07:36:42Z

I think this could be a nice improvement.

Right now, all the error messages follow a similar pattern (see packages/kbn-esql-validation-autocomplete/src/validation/errors.ts). Because of this, I think that if we want to revisit this messaging, we should do it holistically as a separate effort. WDYT?

In general if there's an existing ES error message for something I think we should adopt their message to avoid confusion for the user. I see the current message is diverging from the current ES one who puts the entire bucket signature within the []:

Argument of [bucket] must be a constant, received [abs(length(bytes))]

vs

Argument of [bucket(bytes,abs(length(bytes)), 20, 30)] must be a constant, received [abs(length(bytes))]

The messaging copy used for the non-ES messages are using the same baseline as the other ES ones.

As for the highlight problem, I have a dilemma there as in terms of visual feedback I think it's better, but the error message provided does not provide the right information to the user. Perhaps nested vs nested scenarios ought to have two distinct messages? I think this should also be discussed with the ES team as well to align.

As for now, I think it is ok to unblock on the full function highlight + error message.

drewdaemon · 2024-04-15T13:57:22Z

@elasticmachine merge upstream

kibana-ci · 2024-04-15T15:15:26Z

💚 Build Succeeded

Buildkite Build
Commit: e98fad4

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	+1.4KB

History

💚 Build #203402 succeeded 0b9c510
💚 Build #203126 succeeded ea07f58
💚 Build #202980 succeeded 7264dd0
💚 Build #202885 succeeded e70fee6

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

stratoula · 2024-04-16T05:58:43Z

@drewdaemon I don't think you can backport, in 8.13 it was auto_bucket. The bug is a client side validation one, it is annoying I agree but I think is ok as we are in tech preview in 8.13. Also there will be no other patch for 8.13 as I can see

If you added this for the FF I think if you merge it today, we are fine!

stratoula

I am fine with the messaging and the highlight, it is much more clear than before. I can see the difference with ES that Marco mentioned, we could align but honestly I find our message more clear so I am fine on proceeding with this small difference. 👍

drewdaemon · 2024-04-16T13:43:43Z

@stratoula the backport was to get it into 8.14. However, I'm glad you mentioned this because it looks like it hasn't been branched yet.

I'm not worried about ff because I consider this a set of bug fixes 👍

drewdaemon added 2 commits April 9, 2024 16:29

augment auto-bucket signature

e8b9c06

don't suggest fields and variables for constant-only function parameters

18881ee

drewdaemon changed the title ~~[ES|QL] auto bucket should accept date~~ [ES|QL] auto bucket and constant-only parameter improvements Apr 10, 2024

Merge branch 'main' of github.com:elastic/kibana into 180061/auto-buc…

51b1635

…ket-should-accept-date

drewdaemon force-pushed the 180061/auto-bucket-should-accept-date branch from 130eff6 to 51b1635 Compare April 10, 2024 19:55

drewdaemon added 3 commits April 10, 2024 14:46

clean up some more literalOnly stuff

bb870da

validate nested constant-only args

ed7063c

fix test

e70fee6

drewdaemon added release_note:fix Team:ESQL ES|QL related features in Kibana t// labels Apr 11, 2024

drewdaemon marked this pull request as ready for review April 11, 2024 15:11

drewdaemon requested a review from a team as a code owner April 11, 2024 15:11

drewdaemon and others added 2 commits April 11, 2024 11:00

auto-bucket -> bucket

7264dd0

Merge branch 'main' into 180061/auto-bucket-should-accept-date

ea07f58

dej611 reviewed Apr 12, 2024

View reviewed changes

consolidate constant-only error messages

66eb96f

use nonNullable instead

0b9c510

drewdaemon added the backport:prev-minor label Apr 15, 2024

Merge branch 'main' into 180061/auto-bucket-should-accept-date

e98fad4

stratoula added Feature:ES|QL ES|QL related features in Kibana and removed backport:prev-minor labels Apr 16, 2024

stratoula added the v8.14.0 label Apr 16, 2024

stratoula approved these changes Apr 16, 2024

View reviewed changes

drewdaemon merged commit 0b9847d into elastic:main Apr 16, 2024

kibanamachine added the backport:skip This PR does not require backporting label Apr 16, 2024

Conversation

drewdaemon commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

drewdaemon commented Apr 11, 2024

Uh oh!

elasticmachine commented Apr 11, 2024

Uh oh!

drewdaemon commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratoula commented Apr 12, 2024

Uh oh!

Uh oh!

dej611 Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

drewdaemon Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

stratoula Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

drewdaemon commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dej611 commented Apr 15, 2024

Uh oh!

drewdaemon commented Apr 15, 2024

Uh oh!

kibana-ci commented Apr 15, 2024

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

Uh oh!

stratoula commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

drewdaemon commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

drewdaemon commented Apr 10, 2024 •

edited

Loading

drewdaemon commented Apr 11, 2024 •

edited

Loading

drewdaemon commented Apr 12, 2024 •

edited

Loading

stratoula commented Apr 16, 2024 •

edited

Loading