-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Drop rewriting in date_histogram #57836
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
The `date_histogram` aggregation had an optimization where it'd rewrite `time_zones` who's offset from UTC is fixed across the entire index. This rewrite is no longer needed after elastic#56371 because we can tell that a time zone is fixed lower down in the aggregation. So this removes it.
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
I'm benchmarking this now just to be sure it doesn't change performance. It shouldn't. |
The performance doesn't change with this PR. |
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. Love net negative code PRs :)
The `date_histogram` aggregation had an optimization where it'd rewrite `time_zones` who's offset from UTC is fixed across the entire index. This rewrite is no longer needed after elastic#56371 because we can tell that a time zone is fixed lower down in the aggregation. So this removes it.
The `date_histogram` aggregation had an optimization where it'd rewrite `time_zones` who's offset from UTC is fixed across the entire index. This rewrite is no longer needed after #56371 because we can tell that a time zone is fixed lower down in the aggregation. So this removes it.
Thanks for the review @not-napoleon! These were hard-fought negative lines, that is for sure! |
The
date_histogram
aggregation had an optimization where it'd rewritetime_zones
who's offset from UTC is fixed across the entire index.This rewrite is no longer needed after #56371 because we can tell that a
time zone is fixed lower down in the aggregation. So this removes it.