-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ES|QL: Add TBUCKET function #131449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ES|QL: Add TBUCKET function #131449
Changes from 3 commits
27941a7
fe6dd2b
a5fe0fa
45be0fe
e72467f
5bd6f36
e4a2e04
cc16a42
84717ba
b378d10
466e563
06cd95a
6626682
8cfca3c
ad7fba6
be418fc
b0d5599
f934369
28f0bea
d255687
7216d45
de606d1
a57929e
2eceb51
2fbd13e
0dff032
36063f0
8a90391
a28a651
bfb23c3
117eaac
03f45d9
cbe757b
bc23cdb
014fd83
8bf1c07
91d75a0
86b8d06
83bfd2a
ed92d11
92b02d1
2bf9111
24e8870
2e2ea88
ee5bfc0
3d52fbb
5603fae
e74f6ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
This file was deleted.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// TBUCKET-specific tests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unable to write such a test because BUCKET on a numeric field returns a numeric value, which can legally participate in arithmetic in the STATS select list, even if the same BUCKET expression is also used as the grouping key. That is what the test mentioned is testing. In case of TBUCKET, the error occurs because a grouping function that appears in the BY clause is treated as a grouping key and cannot simultaneously be used as an aggregate expression in the same STATS statement unless it’s referenced exactly the same way or via its alias from the BY list. ES|QL treats BUCKET as a grouping function; once it’s in BY, it can’t be re-used as an aggregate expression unless it’s just referenced as the exact same expression or via the alias. That triggers the verification exception about a grouping function being used as an aggregate after being declared in BY. To confirm this I rewrote the test with BUCKET which led to the same verification exception:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you try this query to see if
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to test it missing the "year" after "1" 🤦🏻♂️ My bad. Your example works in both cases - tbucket and bucket. The test has been added to tbucket.csv-spec. Thanks for that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It still didn't work due to the implicit nature of the @timestamp field and inability to resolve such fields. I found a workaround for that by creating a set of functions that require implicit timestamps, then recursively moving through the plan, checking if the functions require a timestamp and adding it to the nameList. See the changes in the class FieldNameUtils. Please, let me know what you think about this change and if there are better ways to implement this. |
||
|
||
docsTBucketByTimeDuration#[skip:-9.1.99,reason:new grouping function added in 9.2] | ||
required_capability: implicit_casting_string_literal_to_temporal_amount | ||
|
||
// tag::docsTBucketByTimeDuration[] | ||
FROM sample_data | ||
| STATS min = MAX(@timestamp), max = MAX(@timestamp) BY bucket = TBUCKET(30 minutes) | ||
leontyevdv marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| SORT min | ||
// end::docsTBucketByTimeDuration[] | ||
; | ||
|
||
// tag::docsTBucketByTimeDuration-result[] | ||
min:date | max:date | bucket:date | ||
2023-10-23T12:27:28.948Z|2023-10-23T12:27:28.948Z|2023-10-23T12:00:00.000Z | ||
2023-10-23T13:55:01.543Z|2023-10-23T13:55:01.543Z|2023-10-23T13:30:00.000Z | ||
// end::docsTBucketByTimeDuration-result[] | ||
; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is expected. I've added a test to the union_types.csv-spec. Thank you! |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,6 @@ | |
public class GroupingWritables { | ||
|
||
public static List<NamedWriteableRegistry.Entry> getNamedWriteables() { | ||
return List.of(Bucket.ENTRY, Categorize.ENTRY); | ||
return List.of(Bucket.ENTRY, Categorize.ENTRY, TBucket.ENTRY); | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to also include a test or two for
TBUCKET
in an eval; something like| EVAL key = TBUCKET(1 hour) | STATS minimum = MIN(whatever) BY key