-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add time-series aggregation operator #125537
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
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
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 prepares to way to do the smooth the rate accross buckets.
LGTM, thanks Nhat!
| @Override | ||
| public Operator get(DriverContext driverContext) { | ||
| return new HashAggregationOperator(aggregators, () -> { | ||
| if (aggregatorMode.isInputPartial()) { |
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.
Double check, currently the input is not partial during the initial grouping by tsid on data node, right?
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.
Yes, that's correct.
| * An interface indicates that this is a time-series aggregator and it requires time-series source | ||
| */ | ||
| public interface ToTimeSeriesAggregator extends ToAggregator { | ||
|
|
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.
Is the smoothing going to be added to implementations of this interface or to TimeSeriesAggregationOperator?
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 interface serves as a placeholder for detecting time-series aggregations that require _tsid and @timestamp, such as rate (last, ...).
|
Thanks @martijnvg. |
| true // we can enable optimizations as the inputs are vectors | ||
| ); | ||
| } else { | ||
| return new TimeSeriesBlockHash(timestampGroup.channel(), timestampGroup.channel(), driverContext.blockFactory()); |
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.
Shouldn't the first arg be tsidGroup?
Fix the channel in TimeSeriesAggregationOperator. Relates #125537
We will need custom logic in the time-series aggregation operator, such as smoothing the rate across buckets. To address this, this PR introduces a TimeSeriesAggregationOperator that extends HashAggregationOperator to support the addition logic.
Fix the channel in TimeSeriesAggregationOperator. Relates elastic#125537
We will need custom logic in the time-series aggregation operator, such as smoothing the rate across buckets. To address this, this PR introduces a TimeSeriesAggregationOperator that extends HashAggregationOperator to support the addition logic.