-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Add avg_over_time #126572
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
Add avg_over_time #126572
Conversation
ca9a217
to
38c95b3
Compare
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
MAX_OVER_TIME(Build.current().isSnapshot()), | ||
|
||
/** | ||
* Support avg_over_time aggregation |
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: add ", that gets evaluated per time-series".
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.
++ see 24ee2d3
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.
So simple, well done.
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 also played around with it locally. LGTM
@kkrik-es @martijnvg Thanks for the reviews. |
This change adds the
avg_over_time
aggregation for time series indices. Similar to other time series aggregations, we need to translateavg_over_time
into regular aggregations. There are two options for this translation:avg_over_time
toEVAL div(sum_over_time, count_over_time)
, then translatesum_over_time
andcount_over_time
tosum
andcount
.avg_over_time
directly toavg
, and then todiv(sum, count)
.This PR chooses the latter approach. Below is an example:
translates to:
and then:
Since we need to substitute
AVG
withSUM
andCOUNT
after translation, we need to callSubstituteSurrogates
twice inLogicalPlanOptimizer
. If there is a performance impact, we can move this rule toTranslateTimeSeriesAggregate
.