[ES|QL] Fixes in the bucket function signatures

[stratoula]

Summary

This PR:

• Cleanups the buckets hardcoded values and relies on the ES ones
• Merges the time_duration and time_literal. Actually time_literal is not returned from ES while time_duration is.
• Fixes some wrong validations in the bucket function such as:

Checklist

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

[stratoula]

This is the validation check we are sacrificing but it is not an important one

[stratoula]

I also want to raise the importance of these tests. I don't think it is so important to test all these scenarios so possibly a refactoring of the validation tests while re-writing the validation system makes sense @drewdaemon

[drewdaemon]

++ I've been thinking that getting the test suite in order should be a first step or at least a key outcome.

[stratoula]

We were complaining wrongly here

[elasticmachine]

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

[drewdaemon]

Merges the time_duration and time_literal. Actually time_literal is not returned from ES while time_duration is

It is actually a little complex. Elasticsearch supports two datatypes of "timespan" things (link)

1. time_duration — used for shorter spans (3 minutes for example)
2. date_period — used for larger, calendar-aware spans (1 week for example)

Like you say, these are the types we see in the table columns from ES.

Then, there is a syntax in the ES|QL language of a "timespan literal" which is a language construct (e.g. 1 day, 3 hours) that gets translated on the ES side into one of those two types depending on how large the timespan it describes is.

So the question is: how do we deal with all this in our code?

First, we only had time_literal and I think it was roughly supposed to describe the language construct of “timespan literals” while glossing over the Elasticsearch types. This was before Elasticsearch was providing function metadata.

Then, when I looked into it and discovered these two types, I added them to our data types list (see

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

Lines 69 to 70 in b1d3c15

	'time_duration',
	'date_period',

) because I wanted it to describe every type that we could possibly expect to see in the table from Elasticsearch (though I don't think they are "official" field types in the sense that they can be in index mappings... that's why they're not in my field types array, but instead in data types).

And, these two ES types are now in our function definitions because they are scraped from ES. You can see that the docs for bucket differentiate these

(I actually asked the ES team to explore merging these types. They discussed it and decided to keep them separate because they thought it was useful to be able to differentiate between calendar-aware timespans and simple timespans IIRC.)

So, today, we have three things floating around: time_duration, date_period, and time_literal which functions roughly as a union of the other two. And when we detect the type of timespan literal expressions, we don’t make an effort to differentiate between the two types a timespan literal could be (you can see I left it as an open question:

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

Lines 86 to 88 in b1d3c15

	// TODO — consider whether we need to be worried about
	// differentiating between time_duration, and date_period
	// instead of just using time_literal

).

So hopefully that gives a bit of context on why things are like they are today.

I'm not really commenting yes or no on the PR, but I wanted to share some relevant context in case it changes anything.

[stratoula]

Thanx Drew for the context! I admit that I found it complicated to navigate. Which means it needs improvement!

Looking at the codebase I realized we dont need all these types. I preferred to keep the ES types rather than 3. Using time_literal means extra mappings in the script and I dont think we need them. I am exploring further cleanups in another PR. The ES types (although not official ones) should be respected, as they are the source of truth for the signatures, otherwise you need hardcoded signatures and mappings to a kibana type. It also makes it hard to read and understand.

I think this change is towards the right direction. It simplified the code, no need for extra mappings, removes the hardcoded buckets and fixes bugs.

Should we merge them further in the future? Maybe! At some point we also introduce this timeInterval. So there is a meaning of unification at the code. But at this time is a bit of spaggheti. And this PR is moving forward I think. It doesn't introduce any bugs either. (unless I am missing something)

For me the most important thing is to remove the hardcoded buckets (and everything hardcoded in this script) while at the same time make our code simpler.

[drewdaemon]

Makes sense to me, @stratoula.

I'm hearing that we'd like to accept the complexity of the two Elasticsearch types and fully remove the time_literal thing as a type. This may mean eventually differentiating the two Elasticsearch types in getExpressionType. As you say, everything doesn't have to be done in this PR.

I don't think time_duration is technically an adequate substitution for time_literal, since time_literal currently subs for both time_duration and date_period.

However, I can't find a regression! My guess is that the changes here work because every argument in the signatures from ES that accepts a time_duration accepts a date_period as well.

timeInterval is an AST node type, so, in my view, it's doing what is expected. It describes the language construct of timespan literals, not a data type.

I think if we remove time_literal, that will remove the extra complexity in our code, leaving only the complexity of the language. This PR is a good step!

[stratoula]

Thanx Drew! My thoughts exactly!!

[drewdaemon]

Follow-up task for cleaning up the rest of time_literal!

#223037

[kibanamachine]

Starting backport for target branches: 8.19

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

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 3f800bf

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	3.7MB	3.7MB	+12.6KB

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.19	Backport failed because of merge conflicts You might need to backport the following PRs to 8.19: - [ES|QL] Add validations and suggestions for COMPLETION (#222533)

Manual backport

To create the backport manually run:

node scripts/backport --pr 222553

Questions ?

Please refer to the Backport tool documentation

[stratoula]

💚 All backports created successfully

Status	Branch	Result
✅	8.19

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

Questions ?

Please refer to the Backport tool documentation