Clean up time zone options for `date_histogram` #9637

cbuescher · 2015-02-10T18:51:19Z

Remove the current time zone options pre_zone and post_zone in favor of a simplified time_zone option.
The former were hard to understand and made it possible to return dates that are not in UTC. The time_zone parameter can be used to compute the buckets in the specified time zone and the buckets are returned in UTC.
Also the pre_zone_adjust_large_interval parameter is removed because now buckets are always returned in UTC.

Closes #9062

…neAdjustLargeInterval

cbuescher · 2015-02-10T20:47:37Z

Two thing about TimeZoneRounding that I still don't find intuitive to understand and are a potential source of missunderstandings and errors:

The roundKey() and valueForKey() methods in the Roundings classes both look like they take time dates (in millis) as arguments, but only the former does. The valueForKey() method should only be called with arguments that are the result of calling the roundKey() method with a date. Resons for this are internal optimizations mentioned here (Aggs: clean up date_histogram #9062 (comment)). One possibility to make this less error prone would be to pull all the time zone calculations in roundKey() and have all the internal keys be valid UTC dates instead of just ids like right now, but this would mean that the additional calculations that now are in valueForKey() would be called internally for every value during the collect phase in the aggregation.
The nextRoundingValue() should also only be used with certain input values, otherwise the semantics in terms of the underlying Rounding is not clear. At the moment however this is not enforced, and so calling that method e.g. for interval-based Roundings with input values that are not rounded returns values that are not rounded according to the underlying Rounding, (that is round(nextRoundingValue(v)) =< nextRoundingValue(v)). This can lead to bugs like the one discussed in Strange bucket for date_histogram with negative post_zone and zero min_doc_count #7673. One solution would be to take the rounding into account by (1) round the input value (2) add interval or time unit (3) return the rounded result. However this comes at the cost of extra computations that are avoided now.

cbuescher · 2015-02-10T20:48:51Z

Also, after the refactoring the six TimeZoneRounding implementations are mostly identical. I think we can try to reduce them to just two (one for TimeUnit, the other for intervals).

rjernst · 2015-02-11T00:26:03Z

docs/reference/migration/migrate_2_0.asciidoc

@@ -125,6 +125,12 @@ The `histogram` and the `date_histogram` aggregation now support a simplified `o
 `post_offset` rounding options. Instead of having to specify two separate offset shifts of the underlying buckets, the `offset` option
 moves the bucket boundaries in positive or negative direction depending on its argument.

+The `date_histogram` options for `pre_zone` and `post_zone` are replaced by the `time_zone` option. So far the `time_zone` option was


I think this paragraph can be simplified to something like:

The `date_histogram` options for `pre_zone` and `post_zone` have been replaced by the `time_zone` option. The behavior of `time_zone` is equivalent to the former `pre_zone` option.

I think the migration guide should be to the point about what options were available before, and are available now.

I agree, time_zone basically replaces the old pre_zone and the explanation is too verbose here. However I would stress here that the rounding values returned by round(utcTime) which should be equivalent to keyForValue(roundKey(utcTime)) (and thus be used as the bucket keys) are always UTC now. Before you could shift them to any other time zone using post_zone which to avoid was one of the points of #9062. At least thats what I thought it was, but I could also be mistaken.

rjernst · 2015-02-11T00:31:19Z

@cbuescher I'm sorry I haven't been following the discussion about this closely, but I did notice right away here that the logic seems wrong to me. I would expect a single time_zone setting to be the equivalent of passing both pre and post zone before. I thought we always stored dates as UTC, is that not true? If so, setting time zone should tell us what timezone to parse incoming dates, and what time zone to output dates in string form (so that they can be displayed directly, without having to do translation on the client side). It is setting just the pre or post zone alone that I think is bogus.

Also, after the refactoring the six TimeZoneRounding implementations are mostly identical. I think we can try to reduce them to just two (one for TimeUnit, the other for intervals).

Big +1!

cbuescher · 2015-02-11T11:28:03Z

@rjernst I might have missed something here too.

I would expect a single time_zone setting to be the equivalent of passing both pre and post zone before.

I thought that post_zone before was applied after rounding, so that ES could return dates that are not UTC,

I thought we always stored dates as UTC, is that not true?

I think yes, that should be true. Also in InternalDateHistogram.Bucket.getKey() the internal long keys are treated as if they are in UTC. Thats why all values returned by rounding should also be in UTC

setting time zone should tell us what timezone to parse incoming dates, and what time zone to output dates in string form (so that they can be displayed directly, without having to do translation on the client side)

Agree with the first part, but not sure about the second. I thought that after clean up dates and bucket keys (and key_as_string) are always UTC, so client might need to convert that for displaying it.

@jpountz would you agree with the above statements or am I still confusing something here?

jpountz · 2015-02-11T16:54:04Z

What @cbuescher says is correct:

pre_zone was what the new time_zone is
post_zone was a way to convert buckets from UTC to a different time zone

Also, after the refactoring the six TimeZoneRounding implementations are mostly identical. I think we can try to reduce them to just two (one for TimeUnit, the other for intervals).

Huge +1 to that

Two thing about TimeZoneRounding that I still don't find intuitive to understand and are a potential source of missunderstandings and errors

For the record, I'm perfectly fine with simplifying things for now (like putting all the rounding in roundKey) if it helps us be correct. We can still look at optimizing that stuff later...

cbuescher · 2015-02-11T17:00:52Z

I will add the DST tests @orenash mentioned in #9491. They all pass with the cleanup refactorings here. Then I will add another commit with a clean up of the six rounding classes.

jpountz · 2015-02-11T17:17:01Z

src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java

@@ -303,6 +283,8 @@ public void writeTo(StreamOutput out) throws IOException {
        }

        UTCIntervalTimeZoneRounding(long interval) {
+            if (interval < 1)
+                throw new ElasticsearchIllegalArgumentException("Negative time interval not supported");


thanks for adding this!

jpountz · 2015-02-11T17:18:51Z

For the record, the change looks good to me so far.

I will add the DST tests @orenash mentioned in #9491. They all pass with the cleanup refactorings here. Then I will add another commit with a clean up of the six rounding classes.

Sounds great!

cbuescher · 2015-02-13T15:38:28Z

src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java

+            }
+            long rounded = field.roundFloor(timeLocal);
+            if (!isUtc) {
+                return timeZone.convertLocalToUTC(rounded, true, utcMillis);


@jpountz Doing the conversion back from local time to UTC here makes more sense to me, but I'm still wondering about performance maybe beeing better if this would be in valueForKey(). On the other hand that would implicitly create a dependency about how the two methods should be used. Should I make performance tests here?

@cbuescher Making performance tests to see whether we would benefit from moving some computations to valueForKey would be awesome, but I'm already happy that we are making the time zone handling more reasonable, so I would be totally fine with investigating performance in a different issue/PR.

cbuescher · 2015-02-13T15:45:48Z

I've removed four of the six TimeZoneRoundings, this will probably result in some test cases not be needed anymore. Will go through the unit and integration tests to see what is redundant there if you're generally okay with this simplification.

jpountz · 2015-02-13T15:47:38Z

src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java

+            this.field = unit.field();
+            this.durationField = field.getDurationField();
+            this.timeZone = timeZone;
+            this.isUtc = timeZone.equals(DateTimeZone.UTC);


Not sure if we need this boolean to guard against some timezone conversions. When the tz is UTC, it looks like the conversion is going to be very cheap anyway (just substracting a 0 if I'm not mistaken). It would also make the impl simpler?

I agree, only reason I added it was potential performance gains. But as you said above, simplify it here and do performance tests to check if it makes any change at all is the right way I think.

jpountz · 2015-02-13T15:58:15Z

I've removed four of the six TimeZoneRoundings, this will probably result in some test cases not be needed anymore. Will go through the unit and integration tests to see what is redundant there if you're generally okay with this simplification.

Absolutely, this is great!

…ting changes in tests

cbuescher · 2015-02-13T17:09:53Z

Removed the special utc flag in the roundings and reverted some of the auto formatting which should make the diff more readable. There's not that many redundant tests as I thought, better to leave most of them in than miss a case that we test now. I only deleted the tests for removed options (like the "pre_zone_adjust...")

jpountz · 2015-02-13T17:11:40Z

LGTM

cbuescher · 2015-02-16T14:35:28Z

I trief comparing the performance of having the local-to-utc backward conversion either in roundKey() or valueForKey() and could not measure much difference calling it on 10e7 random values, once with a time conversion in roundKey() for every call, the other times just after each 1000/10000/100000 docs in valueForKey(). I dont know if that are usual ratios for buckets, but the measured difference was very small (maybe 1%), so I'll leave it more readable like it is now.

@jpountz since this pr is a little bit larger, do you suggest someone else looking at it or should I just merge this with master?

jpountz · 2015-02-16T14:38:03Z

@cbuescher Please merge!

the other times just after each 1000/10000/100000 docs in valueForKey()

Not sure I understand that part, are these numbers the numbers of buckets that were generated in your test?

cbuescher · 2015-02-16T14:41:25Z

I tested calling roundKey/valueForKey() with the implemenation where the local-to-utc conversion is only done once every bucket in valueForKey(). The time saving in just doing the back conversion depends on the number of values that fall into each bucket, so I varied these (those are the numbers above) but could not see any significant speed up, so I leave it without this optimization which makes the code harder to read.

jpountz · 2015-02-16T14:45:52Z

OK, I understand now. This means that we could simplify further by removing roundKey and valueForKey and just use round() (in another PR).

cbuescher · 2015-02-16T14:59:18Z

Except that these methods are defined in the the abstract Rounding class and it might be different in other implementations than the TimeZoneRoundings.

Christoph Büscher added 4 commits February 9, 2015 16:38

Remove pre/post Timezone in favour of timeZone in Date Histogram

065f6fb

removing internal use of pre/post Timezone in Rounding classes, preZo…

3c0b51e

…neAdjustLargeInterval

Added check for negative interval to rounding classes

a0dfa1a

Update documentation and notes in migration guide

e7ff984

cbuescher mentioned this pull request Feb 10, 2015

Aggs: clean up date_histogram #9062

Closed

cbuescher added review v2.0.0-beta1 >breaking >enhancement labels Feb 10, 2015

cbuescher assigned jpountz Feb 10, 2015

rjernst reviewed Feb 11, 2015
View reviewed changes

orenash mentioned this pull request Feb 11, 2015

date_histogram issue when using "pre_zone_adjust_large_interval" and a timezone with DST #9491

Closed

Add tests for DST boundary cases mentioned in issue 9491

0228701

jpountz reviewed Feb 11, 2015
View reviewed changes

Reduce the number of TimeZoneRounding implementations to two

cea314a

cbuescher reviewed Feb 13, 2015
View reviewed changes

jpountz reviewed Feb 13, 2015
View reviewed changes

Removed extra utc flag in time rounding classes, reverted some format…

da5dc1e

…ting changes in tests

cbuescher closed this in 30fd70f Feb 16, 2015

cbuescher mentioned this pull request Feb 25, 2015

Aggs: Fix rounding issues when using date_histogram and time zones #9790

Closed

clintongormley removed the review label Mar 19, 2015

clintongormley added the :Analytics/Aggregations Aggregations label Jun 6, 2015

clintongormley changed the title ~~Aggregations: Clean up date_histogram~~ Clean up time zone options for date_histogram Jun 6, 2015

cbuescher deleted the fix/9062 branch March 31, 2016 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up time zone options for `date_histogram` #9637

Clean up time zone options for `date_histogram` #9637

cbuescher commented Feb 10, 2015

cbuescher commented Feb 10, 2015

cbuescher commented Feb 10, 2015

rjernst Feb 11, 2015

cbuescher Feb 11, 2015

rjernst commented Feb 11, 2015

cbuescher commented Feb 11, 2015

jpountz commented Feb 11, 2015

cbuescher commented Feb 11, 2015

jpountz Feb 11, 2015

jpountz commented Feb 11, 2015

cbuescher Feb 13, 2015

jpountz Feb 13, 2015

cbuescher commented Feb 13, 2015

jpountz Feb 13, 2015

cbuescher Feb 13, 2015

jpountz commented Feb 13, 2015

cbuescher commented Feb 13, 2015

jpountz commented Feb 13, 2015

cbuescher commented Feb 16, 2015

jpountz commented Feb 16, 2015

cbuescher commented Feb 16, 2015

jpountz commented Feb 16, 2015

cbuescher commented Feb 16, 2015

Clean up time zone options for date_histogram #9637

Clean up time zone options for date_histogram #9637

Conversation

cbuescher commented Feb 10, 2015

cbuescher commented Feb 10, 2015

cbuescher commented Feb 10, 2015

rjernst Feb 11, 2015

Choose a reason for hiding this comment

cbuescher Feb 11, 2015

Choose a reason for hiding this comment

rjernst commented Feb 11, 2015

cbuescher commented Feb 11, 2015

jpountz commented Feb 11, 2015

cbuescher commented Feb 11, 2015

jpountz Feb 11, 2015

Choose a reason for hiding this comment

jpountz commented Feb 11, 2015

cbuescher Feb 13, 2015

Choose a reason for hiding this comment

jpountz Feb 13, 2015

Choose a reason for hiding this comment

cbuescher commented Feb 13, 2015

jpountz Feb 13, 2015

Choose a reason for hiding this comment

cbuescher Feb 13, 2015

Choose a reason for hiding this comment

jpountz commented Feb 13, 2015

cbuescher commented Feb 13, 2015

jpountz commented Feb 13, 2015

cbuescher commented Feb 16, 2015

jpountz commented Feb 16, 2015

cbuescher commented Feb 16, 2015

jpountz commented Feb 16, 2015

cbuescher commented Feb 16, 2015

Clean up time zone options for `date_histogram` #9637

Clean up time zone options for `date_histogram` #9637