shunping commented on PR #30758:
URL: https://github.com/apache/beam/pull/30758#issuecomment-2150345644
> Do they equal when you invoke Object.equal method? If machine doesn't
think its equal then there is a risk to change this.
>
> In general if there is no functionality issue of current status I usually
hesitate to fix things as we've seen many seemingly harmless fix had unexpected
breaking changes
That sounds reasonable, Yi. I created a simple test, the lines commented
below are the failed assertions. We can see that dt1 and dt2 are not considered
equal when calling either `Objects.equals()` or `dt1.equals()`.
```
@Test
public void testEquality() {
Instant ist = Instant.now();
DateTime dt1 = new DateTime(ist.getMillis());
DateTime dt2 = new DateTime(ist);
DateTime dt3 = ist.toDateTime();
System.out.println(dt1);
System.out.println(dt2);
System.out.println(dt3);
// assertEquals(dt1, dt2);
assertEquals(dt1, dt3);
// assertTrue(Objects.equals(dt1, dt2));
assertTrue(Objects.equals(dt1, dt3));
// assertTrue(dt1.equals(dt2));
assertTrue(dt1.equals(dt3));
// assertSame(dt1, dt2);
// assertSame(dt1, dt3);
assertTrue(dt1.isEqual(dt2));
assertTrue(dt1.isEqual(dt3));
}
```
In order to make it consistent, we have to explicitly set timezone when
calling the constructor with a `long` value. The following test passed.
```
@Test
public void testEquality2() {
Instant ist = Instant.now();
DateTime dt1New = new DateTime(ist.getMillis(), ist.getZone());
DateTime dt2 = new DateTime(ist);
System.out.println(dt1New);
System.out.println(dt2);
assertEquals(dt1New, dt2);
assertTrue(dt1New.equals(dt2));
assertTrue(Objects.equals(dt1New, dt2));
assertTrue(dt1New.isEqual(dt2));
}
```
In other words, `DateTime(Instant)` should be equivalent to
`DateTime(Instant.getMills(), Instant.getZone())`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
