-
Notifications
You must be signed in to change notification settings - Fork 25.4k
ESQL: FIRST/LAST #132603
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
ESQL: FIRST/LAST #132603
Conversation
Allows aggregation function tests to process the input `Page` instead of the first `Block`. This should allow us to write tests for elastic#108385.
Fixes a bug in the ungrouped FIRST/LAST implementation when it received timestamps always less than 0 (greater than 0 for LAST). We were always returning `0` as the value....
🔍 Preview links for changed docs |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
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.
LGTM. Thanks Nik!
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.
LGTM! Just 1 small thing
null | two | ||
; | ||
|
||
double_by_null |
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 this file is missing the null_by_timestamp
and has two of the double_by_null
tests?
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.
👍
Adds `toString` checking for aggregators to the generic aggs test cases so we can make sure they spit out sensible looking results. We have this for scalar functions but it isn't plugged in for aggs and I noticed it while working on elastic#132603 where I stuck `asdf` for the toString thinking I'd fix it when the test failed. It didn't. There's to many changes to grab this in one go so I've made a hook that tests can opt into. We'll drop the hook once everything has opted into it.
public First( | ||
Source source, | ||
@Param(name = "value", type = { "long", "integer", "double" }, description = "Values to return") Expression field, | ||
@Param(name = "sort", type = { "date", "date_nanos" }, description = "Sort key") Expression sort |
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.
NIT: should this be order
instead?
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 was thinking "sort key" or "sort by".
) | ||
public First( | ||
Source source, | ||
@Param(name = "value", type = { "long", "integer", "double" }, description = "Values to return") Expression field, |
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.
supplier
below also declares float. Should it be included here and in returnType above?
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.
float
can't be even if the function supports it. Because we don't want the docs to say float
until we actually have float in the system everywhere as a real type. But that means incrementally adding float to everything. I haven't been doing that normally but the LastOverTime
stuff had it so I carried it along.
) | ||
public Last( | ||
Source source, | ||
@Param(name = "value", type = { "long", "integer", "double" }, description = "Values to return") Expression field, |
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.
Same here, is it intentionally skipping float?
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.
Yeah.
Thanks folks! @idegtiarenko |
Prototype implementation of FIRST and LAST. FIRST returns the value with the earliest timestamp. LAST returns the values with the latest timestamp.
Looks like:
SNAPSHOT only for now while we resolve the remaining open questions in #108385.
Important: FIRST/LAST in this PR only return a single value. In the example above, if the value of
network.bytes_in
with the latest@timestamp
isnull
then we get the value with the next highest timestamp. If the value ofnetwork.bytes_in
with the latest@timestamp
is multivalued we'll get a random value from the top values. Some folks want that behavior, but surely not everyone. We'll figure out what it should do soon. But we can get this in under snapshot and folks can play with it.In this prototype, if two documents tie in
@timestamp
then you'll get a value from one of them. Which one is undefined.