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

Make sure TimeIntervalRounding is monotonic for increasing dates #19020

Merged
merged 2 commits into from Jun 30, 2016

Conversation

Projects
None yet
3 participants
@cbuescher
Member

cbuescher commented Jun 22, 2016

Currently there are cases when using TimeIntervalRounding#round() that if date1 < date2 we get round(date2) < round(date1). These errors can happen when using a non-fixed time zone and the values to be rounded are slightly after a time zone offset change (e.g. DST transition).

Here is an example for the "CET" time zone with a 45 minute rounding interval. The dates to be rounded are on the left (with utc time stamp), the rounded values on the right. The error case is marked:

orig. date rounded
2011-10-30T01:40:00.000+02:00 1319931600000  2011-10-30T01:30:00.000+02:00 1319931000000
2011-10-30T02:02:30.000+02:00 1319932950000  2011-10-30T01:30:00.000+02:00 1319931000000
2011-10-30T02:25:00.000+02:00 1319934300000  2011-10-30T02:15:00.000+02:00 1319933700000
2011-10-30T02:47:30.000+02:00 1319935650000  2011-10-30T02:15:00.000+02:00 1319933700000
2011-10-30T02:10:00.000+01:00 1319937000000  2011-10-30T01:30:00.000+02:00 1319931000000 < previous rounded value
2011-10-30T02:32:30.000+01:00 1319938350000  2011-10-30T02:15:00.000+01:00 1319937300000
2011-10-30T02:55:00.000+01:00 1319939700000  2011-10-30T02:15:00.000+01:00 1319937300000
2011-10-30T03:17:30.000+01:00 1319941050000  2011-10-30T03:00:00.000+01:00 1319940000000

We should correct this by detecting that we are crossing a transition when rounding, and in that case pick the largest valid rounded value before the transition.

This change adds this correction logic to the rounding function and adds this invariant to the randomized TimeIntervalRounding tests. Also adding the example test case from above (with corrected behaviour) for illustrative purposes.

Make sure TimeIntervalRounding is monotonic for increasing dates
Currently there are cases when using TimeIntervalRounding#round() and date1 <
date2 that round(date2) < round(date1). These errors can happen when using a
non-fixed time zone and the values to be rounded are slightly after a time zone
offset change (e.g. DST transition).

Here is an example for the "CET" time zone with a 45 minute rounding interval.
The dates to be rounded are on the left (with utc time stamp), the rounded
values on the right. The error case is marked:

2011-10-30T01:40:00.000+02:00 1319931600000 | 2011-10-30T01:30:00.000+02:00 1319931000000
2011-10-30T02:02:30.000+02:00 1319932950000 | 2011-10-30T01:30:00.000+02:00 1319931000000
2011-10-30T02:25:00.000+02:00 1319934300000 | 2011-10-30T02:15:00.000+02:00 1319933700000
2011-10-30T02:47:30.000+02:00 1319935650000 | 2011-10-30T02:15:00.000+02:00 1319933700000
2011-10-30T02:10:00.000+01:00 1319937000000 | 2011-10-30T01:30:00.000+02:00 1319931000000 *
2011-10-30T02:32:30.000+01:00 1319938350000 | 2011-10-30T02:15:00.000+01:00 1319937300000
2011-10-30T02:55:00.000+01:00 1319939700000 | 2011-10-30T02:15:00.000+01:00 1319937300000
2011-10-30T03:17:30.000+01:00 1319941050000 | 2011-10-30T03:00:00.000+01:00 1319940000000

We should correct this by detecting that we are crossing a transition when
rounding, and in that case pick the largest valid rounded value before the
transition.

This change adds this correction logic to the rounding function and adds this
invariant to the randomized TimeIntervalRounding tests. Also adding the example
test case from above (with corrected behaviour) for illustrative purposes.
@cbuescher

This comment has been minimized.

Show comment
Hide comment
@cbuescher

cbuescher Jun 22, 2016

Member

@jpountz @jimferenczi this is a small spin-off of a larger change I have locally, maybe one of you can take a look.

Member

cbuescher commented Jun 22, 2016

@jpountz @jimferenczi this is a small spin-off of a larger change I have locally, maybe one of you can take a look.

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Jun 30, 2016

Contributor

LGTM

Contributor

jpountz commented Jun 30, 2016

LGTM

@cbuescher

This comment has been minimized.

Show comment
Hide comment
@cbuescher

cbuescher Jun 30, 2016

Member

@jpountz thanks for the review

Member

cbuescher commented Jun 30, 2016

@jpountz thanks for the review

@cbuescher cbuescher merged commit afb5e63 into elastic:master Jun 30, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

cbuescher added a commit that referenced this pull request Jun 30, 2016

Make sure TimeIntervalRounding is monotonic for increasing dates (#19020
)

Currently there are cases when using TimeIntervalRounding#round() and date1 <
date2 that round(date2) < round(date1). These errors can happen when using a
non-fixed time zone and the values to be rounded are slightly after a time zone
offset change (e.g. DST transition).

Here is an example for the "CET" time zone with a 45 minute rounding interval.
The dates to be rounded are on the left (with utc time stamp), the rounded
values on the right. The error case is marked:

2011-10-30T01:40:00.000+02:00 1319931600000 | 2011-10-30T01:30:00.000+02:00 1319931000000
2011-10-30T02:02:30.000+02:00 1319932950000 | 2011-10-30T01:30:00.000+02:00 1319931000000
2011-10-30T02:25:00.000+02:00 1319934300000 | 2011-10-30T02:15:00.000+02:00 1319933700000
2011-10-30T02:47:30.000+02:00 1319935650000 | 2011-10-30T02:15:00.000+02:00 1319933700000
2011-10-30T02:10:00.000+01:00 1319937000000 | 2011-10-30T01:30:00.000+02:00 1319931000000 *
2011-10-30T02:32:30.000+01:00 1319938350000 | 2011-10-30T02:15:00.000+01:00 1319937300000
2011-10-30T02:55:00.000+01:00 1319939700000 | 2011-10-30T02:15:00.000+01:00 1319937300000
2011-10-30T03:17:30.000+01:00 1319941050000 | 2011-10-30T03:00:00.000+01:00 1319940000000

We should correct this by detecting that we are crossing a transition when
rounding, and in that case pick the largest valid rounded value before the
transition.

This change adds this correction logic to the rounding function and adds this
invariant to the randomized TimeIntervalRounding tests. Also adding the example
test case from above (with corrected behaviour) for illustrative purposes.
@cbuescher

This comment has been minimized.

Show comment
Hide comment
@cbuescher

cbuescher Jun 30, 2016

Member

Merged to 2.4 with 31ade93.

Member

cbuescher commented Jun 30, 2016

Merged to 2.4 with 31ade93.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment