ThreeTen / threeten

This project was the home of code used to develop a modern date and time library for JDK8. Development has moved to OpenJDK and a separate backport project, threetenbp.
http://threeten.github.io/
191 stars 37 forks source link

isEqual(), for OffsetTimes that are equal relitive to GMT, when their zoneOffsets span day divides, returns false #334

Closed JeremyBetts closed 10 years ago

JeremyBetts commented 10 years ago

Let's say you have an OffsetTime:

march 24, 23:00:00 offset: -02:00 -> withOffsetSameInstant(+02:00) -> march 25, 03:00:00 offset: +02:00

should isEqual() return true for these two OffsetTimes?

While the localTimes as nanos are no longer the same due to the midnight rollover, should they be equal?

from the java doc: "This is equivalent to converting both times to an instant using the same date and comparing the instants"

Should "using the same date" happen before or after normalizing the offset adjustment to the time?

currently it happens after, causing isEqual to return false, even though in terms of GMT they are the same time.

My expectation would be that if after applying withOffsetSameInstant(ZoneOffset.ofHours(0)) -in other words setting both to GMT- to two OffsetTimes, if isEqual returns true, then it should return true no-matter what the ZoneOffset is set to. in the case that the offset spans a day divide, this is not working.

jodastephen commented 10 years ago

The equals() method matches the toString() method. So, if two OffsetTime instances have the same string form, then they should be equal.

The isEqual() method is based on instants. ie. take the two OffsetTime instances, apply the same date to both and calculated the effective Instant. Then compare the instants. eg.

03:00+02:00   add a date
1970-01-01T03:00+02:00  resolve to instant
1970-01-01T01:00Z

00:30-00:30   add a date
1970-01-01T00:30-00:30  resolve to instant
1970-01-01T01:00Z

Instants are the same

If you still think there is a bug could you post a test case?

JeremyBetts commented 10 years ago

run this unit test, I'm out of date code so, if it's already fixed that's fine. I just didnt look that way inspecting the source, I'm pulling a fresh copy now, but the download speeds are only a few KB/sec :P

@Test public void testWithOffsetSameInstant2() { OffsetTime odt = OffsetTime.now(); odt =odt.withOffsetSameInstant(ZoneOffset.ofHours(-5)).withHour(23); //waitNano(); OffsetTime result = odt.withOffsetSameInstant(ZoneOffset.ofHours(1)); assertTrue( " odt.hours: "

jodastephen commented 10 years ago

OK, I see the confusion. withOffsetSameInstant is working fine and changing the local time to match the requested offset. But in doing so it crosses the date boundary. That date boundary crossing information is not retained, it is is lost. Then you compare the two offset times and expect the lost information to be used. Instead, the comparison sees two times that are 18 hours apart (ignoring the offset different)

    System.out.println(odt);
    System.out.println(result);
    System.out.println(odt.equals(result));
    System.out.println(odt.isEqual(result));
    System.out.println(odt.atDate(LocalDate.now()));
    System.out.println(result.atDate(LocalDate.now()));
    System.out.println(odt.atDate(LocalDate.now()).isEqual(result.atDate(LocalDate.now())));
23:03:40.288-05:00
05:03:40.288+01:00
false
false
2013-09-12T23:03:40.288-05:00
2013-09-12T05:03:40.288+01:00
false

If you change the first line from 23:00 to say 13:00 it will work, because withOffsetSameInstant does not then cross the midnight boundary.

JeremyBetts commented 10 years ago

yes, So in summary i'm wondering if checking isEquals should be checking if the time is the same after converting to GMT first.

jodastephen commented 10 years ago

No, I don't think it should.

Looked at in isolation, the local time of 23:00 is later than 05:00. Resolving the 23:00 to 04:00 would take it into the next day, 24 hours different to the original 05:00. Its only because you started from the same point that it seems like it should work the way you want.

In general, OffsetTime does not really have enough information to make this unambiguous. But the only logical way to look at two OffsetTime instances is to treat them as though they are both missing the same date. This also provides a sensible ordering in compareTo().

JeremyBetts commented 10 years ago

23:00 is later than 05:00, but we're not dealing with a just a time object we're dealing with a time object with an offset. while 23:00 is later than 05:00, is 23:00:-02 != 03:00:+02? normalize their offsets and they're both (23+2 mod 24)=1 and (3-2 mod 24)= 1.

if we think about it applying a date like you say WHEN do you apply the date? Example 1 apply date before normalizing: March 24, 23:00:-2 and March 24, 03:00:+2 normalize March 25, 01:00 and March 24, 01:00 They are not the same.

Example 2 apply after normalizing: 23:00:-2 and 03:00:+2 normalize 01:00 and 01:00 apply date march 24, 01:00 and march 24 01:00 they are the same.

You are using the method in example 1, however this is making assumptions for equality based on information we do not have(it's march 24 or march 25) looking at these offset times, all we can say is that both normalize to 01:00 GMT, we have no idea what date it is and therefore cannot make assumptions that there is a 24 hour gap between them.

JackRich commented 10 years ago

Are these supposed to model real-world concepts? The only meaning for a time and offset without a date is "every day", "any day", or "day doesn't matter". I get up at 06:00 -2:00 every day. Does Joe get up at the same moment? He gets up at 10:00 + 2, so yes, he does. I got to bed at 23:00 - 2:00 every night. Does Joe go to bed at the same moment? Joe goes to bed at 3:00 + 2:00 every night. No, he doesn't???!!!! That's a useless modeling concept whose real-world meaning is unclear and is based on how it is implemented, not what it means. The same time is the same time, not a different time because they are on different dates, when there is no date. Undated times cannot be 24 hours or more apart.

RogerRiggs commented 10 years ago

Each offset puts the respective local time on the timeline (relative to zero). The comparison is done on the result. The date does not come into the computation of OffsetTime.isEquals. Offsets are -18hr .. +18hrs and there is no normalization. Of course, around the globe at any instant, the date is not the same everywhere. In the descriptions above, it is not enough to insert the 'same' date with two OffsetTime instances, each needs to paired with the date of the localtime at the offset to refer to the same instant. Is there a use case and enough utility to define a new kind of equals/before/after for OffsetTime that work on the normalized times?

RogerRiggs commented 10 years ago

LocalTime is the right type for any day/every day. Why is the offset needed in your use case?

jodastephen commented 10 years ago

@JackRich OffsetTime will be used in some cases, notably around network transfer. It won't be used much in object modelling, although it can be used effectively (a non-null LocalDate and a nullable OffsetTime make a useful pairing to establish a flexible date/time/instant).

@RogerRiggs I wouldn't want to add another equals form here. The OffsetTime isEquals is based on "imagining in" a date. Your mental model (of local time extended by the offset) also works.

RogerRiggs commented 10 years ago

A normalization method that adjust the offset to be in the range -11:59.. +12:00 might be useful. The localtimes remain the same but the offset values would be consistent with respect to the GMT reference.

jodastephen commented 10 years ago

I think such a method would be very weird. I cna't imagine why you'd want to mangle the data you have in that way.

jodastephen commented 10 years ago

We considered this today. The only way to compare two OffsetTime objects is using the data they contain, which is a LocalTime and a ZoneOffset. It is perfectly possible to have two OffsetTime objects representing time on two islands in the middle of the Pacific Ocean where each is on a different side of the international date line. One might have an offset of +13:00 and the other -11:00. These two locations are in reality 24 hours apart. The current OffsetTime equals/compare definition correctly adjusts to handle this.

As such, we decided to make no change.