Daylight Savings Time Optimization Followups #56124
Assignees
Labels
:Analytics/Aggregations
Aggregations
Meta
Team:Analytics
Meta label for analytical engine team (ESQL/Aggs/Geo)
Comments
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
elasticmachine
added
the
Team:Analytics
The kind of cache that we'd need to build makes this maybe not something we should do. We'd get terrible hit rate if we just used the range and zone id as the key and java.util.time isn't that slow to give us the data that we need. I'll think about this one some more. |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this issue
Jun 8, 2020
This deprecates `Rounding#round` and `Rounding#nextRoundingValue` in favor of calling ``` Rounding.Prepared prepared = rounding.prepare(min, max); ... prepared.round(val) ``` because it is always going to be faster to prepare once. There are going to be some cases where we won't know what to prepare *for* and in those cases you can call `prepareForUnknown` and stil be faster than calling the deprecated method over and over and over again. Ultimately, this is important because it doesn't look like there is an easy way to cache `Rounding.Prepared` or any of its precursors like `LocalTimeOffset.Lookup`. Instead, we can just build it at most once per request. Relates to elastic#56124
nik9000
added a commit
that referenced
this issue
Jun 9, 2020
This deprecates `Rounding#round` and `Rounding#nextRoundingValue` in favor of calling ``` Rounding.Prepared prepared = rounding.prepare(min, max); ... prepared.round(val) ``` because it is always going to be faster to prepare once. There are going to be some cases where we won't know what to prepare *for* and in those cases you can call `prepareForUnknown` and stil be faster than calling the deprecated method over and over and over again. Ultimately, this is important because it doesn't look like there is an easy way to cache `Rounding.Prepared` or any of its precursors like `LocalTimeOffset.Lookup`. Instead, we can just build it at most once per request. Relates to #56124
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this issue
Jun 9, 2020
This deprecates `Rounding#round` and `Rounding#nextRoundingValue` in favor of calling ``` Rounding.Prepared prepared = rounding.prepare(min, max); ... prepared.round(val) ``` because it is always going to be faster to prepare once. There are going to be some cases where we won't know what to prepare *for* and in those cases you can call `prepareForUnknown` and stil be faster than calling the deprecated method over and over and over again. Ultimately, this is important because it doesn't look like there is an easy way to cache `Rounding.Prepared` or any of its precursors like `LocalTimeOffset.Lookup`. Instead, we can just build it at most once per request. Relates to elastic#56124
nik9000
added a commit
that referenced
this issue
Jun 9, 2020
This deprecates `Rounding#round` and `Rounding#nextRoundingValue` in favor of calling ``` Rounding.Prepared prepared = rounding.prepare(min, max); ... prepared.round(val) ``` because it is always going to be faster to prepare once. There are going to be some cases where we won't know what to prepare *for* and in those cases you can call `prepareForUnknown` and stil be faster than calling the deprecated method over and over and over again. Ultimately, this is important because it doesn't look like there is an easy way to cache `Rounding.Prepared` or any of its precursors like `LocalTimeOffset.Lookup`. Instead, we can just build it at most once per request. Relates to #56124
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this issue
Jun 10, 2020
When reducing `auto_date_histogram` we were using `Rounding#round` which is quite a bit more expensive than ``` Rounding.Prepared prepared = rounding.prepare(min, max); long result = prepared.round(date); ``` when rounding to a non-fixed time zone like `America/New_York`. This stops using the former and starts using the latter. Relates to elastic#56124
nik9000
added a commit
that referenced
this issue
Jun 10, 2020
When reducing `auto_date_histogram` we were using `Rounding#round` which is quite a bit more expensive than ``` Rounding.Prepared prepared = rounding.prepare(min, max); long result = prepared.round(date); ``` when rounding to a non-fixed time zone like `America/New_York`. This stops using the former and starts using the latter. Relates to #56124
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this issue
Jun 10, 2020
When reducing `auto_date_histogram` we were using `Rounding#round` which is quite a bit more expensive than ``` Rounding.Prepared prepared = rounding.prepare(min, max); long result = prepared.round(date); ``` when rounding to a non-fixed time zone like `America/New_York`. This stops using the former and starts using the latter. Relates to elastic#56124
nik9000
added a commit
that referenced
this issue
Jun 11, 2020
… (#57958) When reducing `auto_date_histogram` we were using `Rounding#round` which is quite a bit more expensive than ``` Rounding.Prepared prepared = rounding.prepare(min, max); long result = prepared.round(date); ``` when rounding to a non-fixed time zone like `America/New_York`. This stops using the former and starts using the latter. Relates to #56124
|
I think we're mostly comfortable where we are for now. I think we will end up plugging composite into these optimization eventually but that isn't a short term thing. The others don't seem to be a problem. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
After we merge #55559 we'll have a few things to clean up:
LocalTimeOffset.Lookups to avoid building them over and over again. They are quite expensive to build and would likely cache very well.auto_date_histogram. It could speed it up a ton. (Speed up rounding in auto_date_histogram #56384)date_histogram. It won't really do anything after we get interval rounding. (Drop rewriting in date_histogram #57836)nextRoundingValueto make date histogram on date ranges faster.Maybe:
composite. It might not benefit much from it but then again it might. It is worth checking.The text was updated successfully, but these errors were encountered: