Skip to content

SED-4848 Make time-series aggregations configurable on both axes - #136

Open
jeromecomte wants to merge 7 commits into
masterfrom
SED-4848-make-time-series-aggregations-configurable-on-both-axes
Open

SED-4848 Make time-series aggregations configurable on both axes#136
jeromecomte wants to merge 7 commits into
masterfrom
SED-4848-make-time-series-aggregations-configurable-on-both-axes

Conversation

@jeromecomte

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces configurable group-by (series) and time-window aggregations to the time-series aggregation pipeline, adding a new Aggregation enum (AVG, SUM, COUNT) and updating queries, builders, and bucket models to support these independent aggregation axes. The review feedback highlights several improvement opportunities: making a defensive copy of attributes in BucketBuilder to prevent potential runtime exceptions from immutable maps, optimizing map lookups in TimeSeriesAggregationPipeline by moving a redundant lookup outside an inner loop, and avoiding unnecessary boxing/unboxing by changing a parameter in getBucketEnd from Long to long.

david-stephan
david-stephan previously approved these changes Jul 23, 2026

@david-stephan david-stephan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, because therea re no critical changes required, but added a few comments.

@david-stephan david-stephan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-approved after latest changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants