Skip to content

Add logical/physical plans time-series aggregate #126178

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 3, 2025
Merged

Add logical/physical plans time-series aggregate #126178

merged 9 commits into from
Apr 3, 2025

Conversation

@dnhatn
Copy link
Member

@dnhatn dnhatn commented Apr 3, 2025 •
edited
Loading

We need to store extra information for Aggregate and AggregateExec for time-series aggregations. Previously, I added a type (standard or time_series), but this was not enough. This PR removes it and replaces it with extensions of Aggregate and AggregateExec. I considered adding an extra map of metadata to Aggregate and AggregateExec, but this approach seems simpler.

@dnhatn dnhatn force-pushed the add-metadata-aggregate branch 2 times, most recently from 483d153 to 4a4472c Compare April 3, 2025 05:14
@dnhatn dnhatn force-pushed the add-metadata-aggregate branch from 4a4472c to ec52b30 Compare April 3, 2025 06:33
@dnhatn dnhatn added the :StorageEngine/TSDB You know, for Metrics label Apr 3, 2025
@dnhatn dnhatn marked this pull request as ready for review April 3, 2025 06:33
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Apr 3, 2025

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine labels Apr 3, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Apr 3, 2025

Pinging @elastic/es-storage-engine (Team:StorageEngine)

martijnvg
martijnvg approved these changes Apr 3, 2025
View reviewed changes
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Looks like we need to update TelemetryIT (TIME_SERIES is now replaced by STATS), but otherwise LGTM

We need to store extra information for Aggregate and AggregateExec for time-series aggregations.

Just for my own information, what additional information are we planning to store?

kkrik-es
kkrik-es approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

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

Just a few minor comments.

@dnhatn
Copy link
Member Author

dnhatn commented Apr 3, 2025

We need to store extra information for Aggregate and AggregateExec for time-series aggregations.

Just for my own information, what additional information are we planning to store?

We need to store the time bucket for extrapolation in rate aggregation.

@dnhatn
Copy link
Member Author

dnhatn commented Apr 3, 2025

@martijnvg @kkrik-es Thanks for the review!

@dnhatn dnhatn merged commit b0d7c7d into elastic:main Apr 3, 2025
17 checks passed
@dnhatn dnhatn deleted the add-metadata-aggregate branch April 3, 2025 18:36
andreidan pushed a commit to andreidan/elasticsearch that referenced this pull request Apr 9, 2025
@dnhatn @andreidan
Add logical/physical plans time-series aggregate (elastic#126178)
7011a6b 
We need to store extra information for Aggregate and AggregateExec for 
time-series aggregations. Previously, I added a type (standard or
time_series), but this was not enough. This PR removes it and replaces
it with extensions of Aggregate and AggregateExec. I considered adding
an extra map of metadata to Aggregate and AggregateExec, but this
approach seems simpler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martijnvg martijnvg martijnvg approved these changes

@kkrik-es kkrik-es kkrik-es approved these changes

@not-napoleon not-napoleon Awaiting requested review from not-napoleon

Assignees

No one assigned

Labels

:Analytics/ES|QL AKA ESQL >non-issue :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dnhatn @elasticsearchmachine @martijnvg @kkrik-es