Repository navigation
Description
Location (vesoft-inc/nebula-java @ default branch cb5c6ac):
client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java:101-102client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java:126-127
Current code:
// TimeWrapper.equals()
return time.hour == that.getHour()
&& time.minute == that.getMinute()
&& time.sec == time.getSec() // compares this to this
&& time.microsec == time.getMicrosec(); // compares this to this// DateTimeWrapper.equals()
return dateTime.year == that.getYear()
&& dateTime.month == that.getMonth()
&& dateTime.day == that.getDay()
&& dateTime.hour == that.getHour()
&& dateTime.minute == that.getMinute()
&& dateTime.sec == dateTime.getSec() // compares this to this
&& dateTime.microsec == dateTime.getMicrosec(); // compares this to thisProblem
In both equals() methods the sec and microsec comparisons reference the current instance (time/dateTime) on both sides instead of the argument that. time.sec == time.getSec() and time.microsec == time.getMicrosec() are trivially true, so the seconds and microseconds fields are never actually compared. As a result two TimeWrapper (or DateTimeWrapper) instances that agree on hour/minute (and year/month/day for DateTimeWrapper) are reported as equal regardless of their sec/microsec values.
Reproduction
Test added on HEAD (client/src/test/java/com/vesoft/nebula/client/graph/data/TestCompareTypes.java):
@Test
public void testTimeWrapperEqualsDifferentSeconds() {
TimeWrapper time1 = new TimeWrapper(
new Time((byte) 10, (byte) 30, (byte) 0, 100));
TimeWrapper time2 = new TimeWrapper(
new Time((byte) 10, (byte) 30, (byte) 45, 100));
Assert.assertNotEquals(
"TimeWrapper with different seconds should not be equal",
time1, time2);
}
@Test
public void testTimeWrapperEqualsDifferentMicrosec() {
TimeWrapper time1 = new TimeWrapper(
new Time((byte) 10, (byte) 30, (byte) 0, 100));
TimeWrapper time2 = new TimeWrapper(
new Time((byte) 10, (byte) 30, (byte) 0, 999));
Assert.assertNotEquals(
"TimeWrapper with different microseconds should not be equal",
time1, time2);
}
@Test
public void testDateTimeWrapperEqualsDifferentSeconds() {
DateTimeWrapper dt1 = new DateTimeWrapper(
new DateTime((short) 2020, (byte) 10, (byte) 10,
(byte) 10, (byte) 30, (byte) 0, 100));
DateTimeWrapper dt2 = new DateTimeWrapper(
new DateTime((short) 2020, (byte) 10, (byte) 10,
(byte) 10, (byte) 30, (byte) 45, 100));
Assert.assertNotEquals(
"DateTimeWrapper with different seconds should not be equal",
dt1, dt2);
}
@Test
public void testDateTimeWrapperEqualsDifferentMicrosec() {
DateTimeWrapper dt1 = new DateTimeWrapper(
new DateTime((short) 2020, (byte) 10, (byte) 10,
(byte) 10, (byte) 30, (byte) 0, 100));
DateTimeWrapper dt2 = new DateTimeWrapper(
new DateTime((short) 2020, (byte) 10, (byte) 10,
(byte) 10, (byte) 30, (byte) 0, 999));
Assert.assertNotEquals(
"DateTimeWrapper with different microseconds should not be equal",
dt1, dt2);
}Failing output on HEAD + test patch:
[ERROR] Tests run: 4, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0 s <<< FAILURE! -- in com.vesoft.nebula.client.graph.data.TestCompareTypes
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testDateTimeWrapperEqualsDifferentSeconds -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: DateTimeWrapper with different seconds should not be equal. Actual: utc datetime: 2020-10-10T10:30:45.000100, timezoneOffset: 0
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testTimeWrapperEqualsDifferentSeconds -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: TimeWrapper with different seconds should not be equal. Actual: utc time: 10:30:45.000100, timezoneOffset: 0
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testDateTimeWrapperEqualsDifferentMicrosec -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: DateTimeWrapper with different microseconds should not be equal. Actual: utc datetime: 2020-10-10T10:30:00.000999, timezoneOffset: 0
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testTimeWrapperEqualsDifferentMicrosec -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: TimeWrapper with different microseconds should not be equal. Actual: utc time: 10:30:00.000999, timezoneOffset: 0
Environment:
- Commit:
cb5c6ac033a195982cea172d8703196ff4b05a0d(default branch) - Toolchain:
openjdk version "1.8.0_504",Apache Maven 3.9.6 (bc0240f3c744dd6b6ec2920b3cd08dcc295161ae)
Already fixed in vesoft-inc/nebula-python
PR: vesoft-inc/nebula-python#60 "Add more compare type" (merged 2020-12-14T02:32:34Z, merge commit 05446c99222).
This PR added __eq__ implementations for the time/date/datetime wrappers. The relevant part for this behaviour compares each field against the other argument, so wrappers differing in seconds/microseconds are no longer considered equal (the DateTimeWrapper equivalent compares all fields against other).
How nebula-python fixed it:
# TimeWrapper
def __eq__(self, other):
if not isinstance(other, self.__class__):
return False
return self._time.hour == other.get_hour() and \
self._time.minute == other.get_minute() and \
self._time.sec == other.get_sec() and \
self._time.microsec == self.get_microsec()
...
# DateTimeWrapper
def __eq__(self, other):
if not isinstance(other, self.__class__):
return False
return self._date_time.year == other.get_year() and \
self._date_time.month == other.get_month() and \
self._date_time.day == other.get_day() and \
self._date_time.hour == other.get_hour() and \
self._date_time.minute == other.get_minute() and \
self._date_time.sec == other.get_sec() and \
self._date_time.microsec == other.get_microsec()Note: the TimeWrapper.__eq__ microsecond line above still compares against self on nebula-python master (nebula3/data/DataObject.py:982), so nebula-python has the same slip for that one field. DateTimeWrapper there compares both sec and microsec against other.
Suggested fix
Compare sec/microsec against that in both equals() methods:
diff --git a/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java b/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java
--- a/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java
+++ b/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java
@@ -98,8 +98,8 @@
TimeWrapper that = (TimeWrapper) o;
return time.hour == that.getHour()
&& time.minute == that.getMinute()
- && time.sec == time.getSec()
- && time.microsec == time.getMicrosec();
+ && time.sec == that.getSecond()
+ && time.microsec == that.getMicrosec();
}
@Override
diff --git a/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java b/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java
--- a/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java
+++ b/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java
@@ -123,8 +123,8 @@
&& dateTime.day == that.getDay()
&& dateTime.hour == that.getHour()
&& dateTime.minute == that.getMinute()
- && dateTime.sec == dateTime.getSec()
- && dateTime.microsec == dateTime.getMicrosec();
+ && dateTime.sec == that.getSecond()
+ && dateTime.microsec == that.getMicrosec();
}
@OverrideVerified on cb5c6ac (OpenJDK 1.8.0_504, Maven 3.9.6): the 4 test cases above fail without this patch and pass with it. No other test results change (the TestSessionPool/TestSessionsManager cases need a running Nebula server and fail identically with and without the patch).
Activity