ESQL: BUCKET: allow numerical spans as whole numbers #111874

bpintea · 2024-08-14T09:14:09Z

This laxes the check on numerical spans to allow them be specified as whole numbers. So far it was required that they be provided as a double.

This also expands the tests for date ranges to include string types.

Resolves #109340, resolves #104646, resolves #105375.

This laxes the check on numerical spans to allow them be specified as whole numbers. So far it was required that they be provided as a double. This also expands the tests for date ranges to include string types.

github-actions · 2024-08-14T09:14:20Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-08-14T09:14:32Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-08-14T09:14:33Z

Pinging @elastic/kibana-esql (ES|QL-ui)

elasticsearchmachine · 2024-08-14T09:14:33Z

Hi @bpintea, I've created a changelog YAML for you.

elasticsearchmachine · 2024-08-14T09:18:31Z

Hi @bpintea, I've updated the changelog YAML for you.

alex-spies

Generally LGTM, but I couldn't find tests for the type validation in case a BUCKET query is invalid. (Though, it's been a while since I looked at BUCKET and only made a cursory search for the tests.) Should they actually be missing, maybe we should add them.

alex-spies · 2024-08-14T15:23:05Z

docs/reference/esql/functions/kibana/definition/bucket.json

+        },
+        {
+          "name" : "to",
+          "type" : "keyword",


A bit out of scope, but: @fang-xing-esql introduced a mechanism for implicit casting. to and from arguments of type keyword and text actually only apply to datetime literals, right?

If so, maybe we can slim down BUCKETs huge type table, and slim down this JSON here as well by moving the handling of datetime strings out of BUCKET and into ImplicitCasting?

(Something for a follow up, but wanted to throw it out there as maybe a nicer long term solution.)

alex-spies · 2024-08-14T15:30:25Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/bucket.csv-spec

@@ -314,6 +314,21 @@ FROM sample_data
 3              |2025-10-01T00:00:00.000Z
 ;

+bucketByYearLowBucketCount#[skip:-8.13.99, reason:BUCKET extended in 8.14]


Is this test related to this PR, or is it just adding a test for something that existed?

Is the skip accurate, or should this use a capability instead?

Not directly: it just notes that we're not really respecting the contract of providing up to the number of buckets requested, once the bucket size reaches a year.
The skip is accurate, the test isn't using any newly introduced change.

alex-spies · 2024-08-14T15:36:10Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/bucket.csv-spec

+bucketNumericMixedTypesIntegerSpans
+required_capability: bucket_whole_number_as_span
+ROW long = TO_LONG(100), double = 99., int = 100
+| STATS BY b1 = BUCKET(long, double::int), b2 = BUCKET(double, long), b3 = BUCKET(int, 49.5)


Btw: How does that work when the buckets parameter is obtained from a field? Doesn't it have to be a constant for the STATS to make sense?

Doesn't it have to be a constant for the STATS to make sense?

It doesn't have to be a constant, but it has to be a foldable expression (i.e. a field declared with ROW).

row is a good source of constants, but we can also define foldable values in eval (and used with from). Just wanted to point out that "foldable expression" isn't always something that comes from row.

alex-spies · 2024-08-14T15:45:59Z

...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Bucket.java

@@ -176,20 +174,20 @@ public Bucket(
        ) Expression field,
        @Param(
            name = "buckets",
-            type = { "integer", "double", "date_period", "time_duration" },
+            type = { "integer", "long", "double", "date_period", "time_duration" },
            description = "Target number of buckets."


Pardon my ignorance, but should that be

Suggested change

description = "Target number of buckets."

description = "Target number of buckets, or desired bucket size if `from` and `to` parameters are omitted."

?

alex-spies · 2024-08-14T16:18:08Z

...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Bucket.java

-                : isNumeric(buckets, sourceText(), SECOND).and(checkArgsCount(2));
+            return isNumeric(buckets, sourceText(), SECOND).and(() -> {
+                if (bucketsType.isRationalNumber()) {
+                    return checkArgsCount(2);


Do we have (verifier) test cases for when the 2nd argument is rational but we provide more than 2 arguments?

I've added new tests around incorrect arguments, both in number and type, thanks.
(This should have been done in my earlier change already.)

alex-spies · 2024-08-14T16:20:02Z

...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Bucket.java

-            return bucketsType.isWholeNumber()
-                ? checkArgsCount(4).and(() -> isNumeric(from, sourceText(), THIRD)).and(() -> isNumeric(to, sourceText(), FOURTH))
-                : isNumeric(buckets, sourceText(), SECOND).and(checkArgsCount(2));
+            return isNumeric(buckets, sourceText(), SECOND).and(() -> {


Do we have (verifier) test cases for when the first argument is numeric but the second is not?

alex-spies · 2024-08-14T16:21:19Z

...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Bucket.java

+                        resolution = checkArgsCount(4).and(() -> isNumeric(from, sourceText(), THIRD))
+                            .and(() -> isNumeric(to, sourceText(), FOURTH));


Do we have (verifier) test cases for when the args count is higher than 4, or the 3rd and 4th are not numeric?

Added some.
Not for a higher than 4 count, though, since this would be a general parser exception, raised when resolving the function.

alex-spies

Thanks for the iteration @bpintea !

alex-spies · 2024-08-20T08:44:19Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

@@ -374,6 +374,66 @@ public void testGroupingInsideGrouping() {
        );
    }

+    public void testInvalidBucketCalls() {


Thank you! I think the validation for BUCKET is complex enough that we'll be very happy about this test when we make changes in the future.

alex-spies · 2024-08-20T08:51:02Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

@@ -3514,6 +3514,49 @@ public void testBucketWithAggExpression() {
        assertThat(agg.groupings().get(0), is(ref));
    }

+    public void testBucketWithNonFoldingArgs() {


nit: wouldn't these be better placed in the VerifierTests? I wouldn't know to look here for these tests (except when they're failing :) )

It turns out we do another validation past optimisations, to be able to take the folding into account. (Not sure if we'll ever need this for anything else than supporting ROW, but that's how it now works.)

This laxes the check on numerical spans to allow them be specified as whole numbers. So far it was required that they be provided as a double. This also expands the tests for date ranges to include string types. Resolves elastic#109340, resolves elastic#104646, resolves elastic#105375.

Allow numerical spans as whole numbers

d36b67b

This laxes the check on numerical spans to allow them be specified as whole numbers. So far it was required that they be provided as a double. This also expands the tests for date ranges to include string types.

bpintea added >enhancement :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI v8.16.0 labels Aug 14, 2024

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 14, 2024

bpintea added 2 commits August 14, 2024 11:14

Update docs/changelog/111874.yaml

59db3e2

Update docs/changelog/111874.yaml

8d24521

bpintea added 3 commits August 14, 2024 14:52

Test fixes

d8c7004

Test fix

71b4aec

Merge branch 'main' into enh/bucket_span_as_whole

d6a2e9a

bpintea added the test-update-serverless label Aug 14, 2024

alex-spies approved these changes Aug 14, 2024

View reviewed changes

bpintea added 4 commits August 15, 2024 14:50

Merge branch 'main' into enh/bucket_span_as_whole

1523e7e

Merge branch 'main' into enh/bucket_span_as_whole

86512ae

Review comments

8f81ad2

Spotless

95e41c5

alex-spies approved these changes Aug 20, 2024

View reviewed changes

bpintea merged commit dd49c33 into elastic:main Aug 20, 2024
16 checks passed

bpintea deleted the enh/bucket_span_as_whole branch August 20, 2024 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: BUCKET: allow numerical spans as whole numbers #111874

ESQL: BUCKET: allow numerical spans as whole numbers #111874

bpintea commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

alex-spies left a comment

alex-spies Aug 14, 2024

alex-spies Aug 14, 2024

bpintea Aug 20, 2024

alex-spies Aug 14, 2024

bpintea Aug 20, 2024

astefan Aug 20, 2024

alex-spies Aug 14, 2024

alex-spies Aug 14, 2024

bpintea Aug 20, 2024

alex-spies Aug 14, 2024

bpintea Aug 20, 2024

alex-spies Aug 14, 2024

bpintea Aug 20, 2024

alex-spies left a comment

alex-spies Aug 20, 2024

alex-spies Aug 20, 2024

bpintea Aug 20, 2024 •

edited

Loading

	description = "Target number of buckets."
	description = "Target number of buckets, or desired bucket size if `from` and `to` parameters are omitted."

		resolution = checkArgsCount(4).and(() -> isNumeric(from, sourceText(), THIRD))
		.and(() -> isNumeric(to, sourceText(), FOURTH));

ESQL: BUCKET: allow numerical spans as whole numbers #111874

ESQL: BUCKET: allow numerical spans as whole numbers #111874

Conversation

bpintea commented Aug 14, 2024 • edited Loading

github-actions bot commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

elasticsearchmachine commented Aug 14, 2024

alex-spies 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

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

bpintea commented Aug 14, 2024 •

edited

Loading

bpintea Aug 20, 2024 •

edited

Loading