-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Support time_zone on composite's date_histogram #51172
Conversation
We've been parsing the `time_zone` parameter on `date_hitogram` for a while but it hasn't *done* anything. This wires it up. Closes elastic#45199 Inspired by elastic#45200
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
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.
Changes look good, but I'm a bit confused why the timezone tests in CompositeAggregatorTests
pass without modification?
E.g. since DateHistogramValuesSourceBuilder would set it's own timezone before, so the shard rounding math was correct (e.g. in DateHistogramValuesSourceBuilder#innderBuild()
) but the VS resolution would use null and thus the formatter would be in UTC. But with this fix the correct formatter should be used, so I'm a little surprised the tests are still passing.
I'll see what I can find out! |
@polyfractal it looks like we just didn't have any unit tests using time zones. I added some. |
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, thanks @nik9000
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.
LGTM2, thanks for adding the test :)
(The test I was looking at before was testWithDateHistogramAndTimeZone()
, but I'm probably misreading what it's supposed to do, only skimmed over it.)
Wow, I can't believe I missed that test! I spent a while staring at it and I think it didn't catch the problem or the change because there weren't any times that changed which interval they are part of with the time zone. |
We've been parsing the `time_zone` parameter on `date_hitogram` for a while but it hasn't *done* anything. This wires it up. Closes elastic#45199 Inspired by elastic#45200
Thanks @polyfractal and @jimczi ! |
We've been parsing the `time_zone` parameter on `date_hitogram` for a while but it hasn't *done* anything. This wires it up. Closes elastic#45199 Inspired by elastic#45200
Now that elastic#51172 is fully backported we can fix the `skip` clause in the bwc tests for it.
Now that elastic#51172 is fully backported we can fix the `skip` clause in the bwc tests for it.
Now that elastic#51172 is fully backported we can fix the `skip` clause in the bwc tests for it.
Now that #51172 is fully backported we can fix the `skip` clause in the bwc tests for it.
Now that #51172 is fully backported we can fix the `skip` clause in the bwc tests for it.
Now that #51172 is fully backported we can fix the `skip` clause in the bwc tests for it.
We've been parsing the
time_zone
parameter ondate_histogram
for awhile but it hasn't done anything. This wires it up.
Closes #45199
Inspired by #45200