Fix DateTimeValue equals and compareTo; move TimeZones to values module

DateTimeValue now correctly marks renamed time zones as being equal.
neo4j · Mar 16, 2018 · 4a34056 · 4a34056
1 parent 9ea4bff
commit 4a34056
Show file tree

Hide file tree

Showing 13 changed files with 85 additions and 17 deletions.
diff --git a/community/bolt/src/test/java/org/neo4j/bolt/v2/messaging/Neo4jPackV2Test.java b/community/bolt/src/test/java/org/neo4j/bolt/v2/messaging/Neo4jPackV2Test.java
@@ -35,7 +35,7 @@
 import org.neo4j.bolt.v1.messaging.Neo4jPack;
 import org.neo4j.bolt.v1.packstream.PackedInputArray;
 import org.neo4j.bolt.v1.packstream.PackedOutputArray;
-import org.neo4j.kernel.impl.store.TimeZones;
+import org.neo4j.values.storable.TimeZones;
 import org.neo4j.values.AnyValue;
 import org.neo4j.values.storable.CoordinateReferenceSystem;
 import org.neo4j.values.storable.DateTimeValue;

diff --git a/community/kernel/pom.xml b/community/kernel/pom.xml
@@ -267,11 +267,5 @@ the relevant Commercial Agreement.
       <type>test-jar</type>
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>org.apache.commons</groupId>
-      <artifactId>commons-compress</artifactId>
-      <version>1.16.1</version>
-      <scope>test</scope>
-    </dependency>
   </dependencies>
 </project>
diff --git a/...unity/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ZonedDateTimeSchemaKey.java b/...unity/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ZonedDateTimeSchemaKey.java
@@ -22,7 +22,7 @@
 import java.time.ZoneId;
 import java.time.ZoneOffset;
 
-import org.neo4j.kernel.impl.store.TimeZones;
+import org.neo4j.values.storable.TimeZones;
 import org.neo4j.values.storable.DateTimeValue;
 import org.neo4j.values.storable.Value;
 import org.neo4j.values.storable.Values;

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ZonedTimeSchemaKey.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ZonedTimeSchemaKey.java
@@ -21,7 +21,7 @@
 
 import java.time.ZoneOffset;
 
-import org.neo4j.kernel.impl.store.TimeZones;
+import org.neo4j.values.storable.TimeZones;
 import org.neo4j.values.storable.TimeValue;
 import org.neo4j.values.storable.Value;
 

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/TemporalType.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/TemporalType.java
@@ -43,6 +43,7 @@
 import org.neo4j.values.storable.LocalTimeValue;
 import org.neo4j.values.storable.LongArray;
 import org.neo4j.values.storable.TimeValue;
+import org.neo4j.values.storable.TimeZones;
 import org.neo4j.values.storable.Value;
 import org.neo4j.values.storable.Values;
 

diff --git a/.../kernel/src/test/java/org/neo4j/kernel/api/index/IndexProviderCompatibilityTestSuite.java b/.../kernel/src/test/java/org/neo4j/kernel/api/index/IndexProviderCompatibilityTestSuite.java
@@ -115,6 +115,7 @@ public Compatibility( IndexProviderCompatibilityTestSuite testSuite, SchemaIndex
                             DateTimeValue.datetime( 2014, 3, 25, 12, 46, 13, 7474, "+05:00" ),
                             DateTimeValue.datetime( 2014, 3, 25, 12, 45, 14, 7474, "+05:00" ),
                             DateTimeValue.datetime( 2014, 3, 25, 12, 45, 14, 7475, "+05:00" ),
+                            DateTimeValue.datetime( 2001, 1, 25, 11, 11, 30, 0, "Canada/East-Saskatchewan" ),
                             DateTimeValue.datetime( 10000, 100, ZoneOffset.ofTotalSeconds( 3 ) ),
                             DateTimeValue.datetime( 10000, 101, ZoneOffset.ofTotalSeconds( -3 ) ),
                             DurationValue.duration( 10, 20, 30, 40 ),

diff --git a/community/values/pom.xml b/community/values/pom.xml
@@ -71,5 +71,11 @@
       <groupId>org.mockito</groupId>
       <artifactId>mockito-core</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-compress</artifactId>
+      <version>1.16.1</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 </project>
diff --git a/community/values/src/main/java/org/neo4j/values/storable/DateTimeValue.java b/community/values/src/main/java/org/neo4j/values/storable/DateTimeValue.java
@@ -407,7 +407,15 @@ boolean hasTime()
     @Override
     public boolean equals( Value other )
     {
-        return other instanceof DateTimeValue && value.equals( ((DateTimeValue) other).value );
+        if ( other instanceof DateTimeValue )
+        {
+            ZonedDateTime that = ((DateTimeValue) other).value;
+            return value.toLocalDateTime().equals( that.toLocalDateTime() ) &&
+                   value.getOffset().equals( that.getOffset() ) &&
+                   !areDifferentZones( value.getZone(), that.getZone() );
+
+        }
+        return false;
     }
 
     @Override
@@ -427,10 +435,39 @@ public <E extends Exception> void writeTo( ValueWriter<E> writer ) throws E
     }
 
     @Override
-    int unsafeCompareTo( Value otherValue )
+    int unsafeCompareTo( Value other )
+    {
+        ZonedDateTime that = ((DateTimeValue) other).value;
+        int cmp = Long.compare( value.toEpochSecond(), that.toEpochSecond() );
+        if ( cmp == 0 )
+        {
+            cmp = value.toLocalTime().getNano() - that.toLocalTime().getNano();
+            if ( cmp == 0 )
+            {
+                cmp = value.toLocalDateTime().compareTo( that.toLocalDateTime() );
+                if ( cmp == 0 )
+                {
+                    ZoneId thisZone = value.getZone();
+                    ZoneId thatZone = that.getZone();
+                    if ( areDifferentZones( thisZone, thatZone ) )
+                    {
+                        cmp = thisZone.getId().compareTo( thatZone.getId() );
+                    }
+                    if ( cmp == 0 )
+                    {
+                        cmp = value.getChronology().compareTo( that.getChronology() );
+                    }
+                }
+            }
+        }
+        return cmp;
+    }
+
+    private boolean areDifferentZones( ZoneId thisZone, ZoneId thatZone )
     {
-        DateTimeValue other = (DateTimeValue) otherValue;
-        return value.compareTo( other.value );
+        return !(thisZone instanceof ZoneOffset) &&
+               !(thatZone instanceof ZoneOffset) &&
+                TimeZones.map( thisZone.getId() ) != TimeZones.map( thatZone.getId() );
     }
 
     @Override

diff --git a/...rg/neo4j/kernel/impl/store/TimeZones.java → .../org/neo4j/values/storable/TimeZones.java b/...rg/neo4j/kernel/impl/store/TimeZones.java → .../org/neo4j/values/storable/TimeZones.java
@@ -17,7 +17,7 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
-package org.neo4j.kernel.impl.store;
+package org.neo4j.values.storable;
 
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -85,6 +85,8 @@ public static Set<String> supportedTimeZones()
     {
         String latestVersion = "";
         Pattern version = Pattern.compile( "# tzdata([0-9]{4}[a-z])" );
+        Map<String,Short> tempStringToShort = new HashMap<>( 1024 );
+
         try ( BufferedReader reader = new BufferedReader( new InputStreamReader( TimeZones.class.getResourceAsStream( "/TZIDS" ) ) ) )
         {
             for ( String line; (line = reader.readLine()) != null; )
@@ -108,12 +110,12 @@ else if ( line.startsWith( "#" ) )
                     String oldName = line.substring( 0, sep );
                     String newName = line.substring( sep + 1 );
                     TIME_ZONE_SHORT_TO_STRING.add( newName );
-                    TIME_ZONE_STRING_TO_SHORT.put( oldName, (short) (TIME_ZONE_SHORT_TO_STRING.size() - 1) );
+                    tempStringToShort.put( oldName, (short) (TIME_ZONE_SHORT_TO_STRING.size() - 1) );
                 }
                 else
                 {
                     TIME_ZONE_SHORT_TO_STRING.add( line );
-                    TIME_ZONE_STRING_TO_SHORT.put( line, (short) (TIME_ZONE_SHORT_TO_STRING.size() - 1) );
+                    tempStringToShort.put( line, (short) (TIME_ZONE_SHORT_TO_STRING.size() - 1) );
                 }
             }
             LATEST_SUPPORTED_IANA_VERSION = latestVersion;
@@ -122,5 +124,13 @@ else if ( line.startsWith( "#" ) )
         {
             throw new RuntimeException( "Failed to read time zone id file." );
         }
+
+        for ( Map.Entry<String,Short> entry : tempStringToShort.entrySet() )
+        {
+            String oldName = entry.getKey();
+            String newName = TIME_ZONE_SHORT_TO_STRING.get( entry.getValue() );
+            Short newNameId = tempStringToShort.get( newName );
+            TIME_ZONE_STRING_TO_SHORT.put( oldName, newNameId );
+        }
     }
 }
diff --git a/community/kernel/src/main/resources/TZIDS → community/values/src/main/resources/TZIDS b/community/kernel/src/main/resources/TZIDS → community/values/src/main/resources/TZIDS
diff --git a/community/values/src/test/java/org/neo4j/values/storable/DateTimeValueTest.java b/community/values/src/test/java/org/neo4j/values/storable/DateTimeValueTest.java
@@ -454,6 +454,13 @@ public void shouldEqualItself()
         assertEqual( datetime( 10000, 100, UTC ), datetime( 10000, 100, UTC ) );
     }
 
+    @Test
+    public void shouldEqualRenamedTimeZone()
+    {
+        assertEqual( datetime( 10000, 100, ZoneId.of( "Canada/Saskatchewan" ) ),
+                     datetime( 10000, 100, ZoneId.of( "Canada/East-Saskatchewan" ) ) );
+    }
+
     @Test
     public void shouldNotEqualSameInstantButDifferentTimezone()
     {

diff --git a/...eo4j/kernel/impl/store/TimeZonesTest.java → .../neo4j/values/storable/TimeZonesTest.java b/...eo4j/kernel/impl/store/TimeZonesTest.java → .../neo4j/values/storable/TimeZonesTest.java
@@ -17,7 +17,7 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
-package org.neo4j.kernel.impl.store;
+package org.neo4j.values.storable;
 
 import org.apache.commons.compress.archivers.ArchiveEntry;
 import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;

diff --git a/community/values/src/test/java/org/neo4j/values/storable/ValueComparisonTest.java b/community/values/src/test/java/org/neo4j/values/storable/ValueComparisonTest.java
@@ -27,13 +27,15 @@
 import java.time.LocalDateTime;
 import java.time.LocalTime;
 import java.time.OffsetTime;
+import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.List;
 import java.util.stream.Collectors;
 
 import static java.lang.String.format;
+import static org.junit.Assert.assertEquals;
 import static org.neo4j.values.storable.CoordinateReferenceSystem.Cartesian;
 import static org.neo4j.values.storable.CoordinateReferenceSystem.WGS84;
 import static org.neo4j.values.storable.DateTimeValue.datetime;
@@ -103,6 +105,8 @@ public class ValueComparisonTest
             datetime(2018, 2, 2, 0, 0, 0, 0, "+00:00"),
             datetime(2018, 2, 2, 1, 30, 0, 0, "+01:00"),
             datetime(2018, 2, 2, 1, 0, 0, 0, "+00:00"),
+            datetime(2018, 3, 2, 1, 0, 0, 0, "Europe/Berlin"),
+            datetime(2018, 3, 2, 1, 0, 0, 0, "Europe/Stockholm"), // same offset as Europe/Berlin, so compared by zone name
             localDateTime(2018, 2, 2, 0, 0, 0, 0),
             localDateTime(2018, 2, 2, 0, 0, 0, 1),
             localDateTime(2018, 2, 2, 0, 0, 1, 0),
@@ -259,6 +263,14 @@ public void shouldOrderValuesCorrectly()
         }
     }
 
+    @Test
+    public void shouldCompareRenamedTimeZonesByZoneNumber()
+    {
+        int cmp = Values.COMPARATOR.compare( datetime( 10000, 100, ZoneId.of( "Canada/Saskatchewan" ) ),
+                                             datetime( 10000, 100, ZoneId.of( "Canada/East-Saskatchewan" ) ) );
+        assertEquals( "East-Saskatchewan and Saskatchewan are the same place", 0, cmp );
+    }
+
     private <T> int compare( Comparator<T> comparator, T left, T right )
     {
         int cmp1 = comparator.compare( left, right );