Use JVM time zone rules in Joda

Mixed use of joda and java.time results in two different versions of
tzdata being used at the same time for a query. This has lead to
inconsistencies that had resulted in incorrect query results, and
query execution failure.

This commit makes joda and java.time use the same tzdata by making
joda delegate to java.time for all time zone rules.

pom.xml

@@ -592,6 +592,12 @@
                 <version>2.1.5.1</version>
             </dependency>
 
+            <dependency>
+                <groupId>io.airlift</groupId>
+                <artifactId>joda-to-java-time-bridge</artifactId>
+                <version>1</version>
+            </dependency>
+
             <dependency>
                 <groupId>io.airlift.drift</groupId>
                 <artifactId>drift-api</artifactId>

presto-accumulo/pom.xml

@@ -245,6 +245,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>org.apache.zookeeper</groupId>
             <artifactId>zookeeper</artifactId>

presto-base-jdbc/pom.xml

@@ -68,6 +68,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>javax.inject</groupId>
             <artifactId>javax.inject</artifactId>

presto-cassandra/pom.xml

@@ -77,6 +77,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>javax.inject</groupId>
             <artifactId>javax.inject</artifactId>

presto-docs/src/main/sphinx/develop/connectors.rst

@@ -56,3 +56,14 @@ Given a split and a list of columns, the record set provider is
 responsible for delivering data to the Presto execution engine.
 It creates a ``RecordSet``, which in turn creates a ``RecordCursor``
 that is used by Presto to read the column values for each row.
+
+.. note::
+
+    For any connector that uses Joda-Time, it must declare a ``runtime``
+    dependency on `joda-to-java-time-bridge <https://github.com/airlift/joda-to-java-time-bridge/blob/master/README.md>`_.
+    Presto internally uses ``java.time`` for certain date/time
+    computations. Thus, to avoid incorrect results caused by inconsistent
+    time zone rules, Presto registers a custom Joda-Time zone provider
+    at startup time, which makes Joda-Time delegate to the JVM's built-in
+    zone rules. The zone provider can be found in the above package.
+    If the dependency is not declared, a class loading error will occur.

presto-hive/pom.xml

@@ -144,6 +144,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>com.amazonaws</groupId>
             <artifactId>aws-java-sdk-core</artifactId>

presto-kafka/pom.xml

@@ -67,6 +67,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>javax.annotation</groupId>
             <artifactId>javax.annotation-api</artifactId>

presto-kudu/pom.xml

@@ -53,6 +53,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>io.airlift</groupId>
             <artifactId>configuration</artifactId>

presto-main/pom.xml

@@ -186,6 +186,11 @@
             <artifactId>joni</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+        </dependency>
+
         <dependency>
             <groupId>com.teradata</groupId>
             <artifactId>re2j-td</artifactId>
@@ -401,6 +406,9 @@
                     <excludes>
                         <exclude>**/TestLocalExecutionPlanner.java</exclude>
                     </excludes>
+                    <systemPropertyVariables>
+                        <org.joda.time.DateTimeZone.Provider>io.airlift.jodabridge.JdkBasedZoneInfoProvider</org.joda.time.DateTimeZone.Provider>
+                    </systemPropertyVariables>
                 </configuration>
             </plugin>
         </plugins>

presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java

@@ -41,6 +41,7 @@
 import io.airlift.jaxrs.JaxrsModule;
 import io.airlift.jmx.JmxHttpModule;
 import io.airlift.jmx.JmxModule;
+import io.airlift.jodabridge.JdkBasedZoneInfoProvider;
 import io.airlift.json.JsonModule;
 import io.airlift.log.LogJmxModule;
 import io.airlift.log.Logger;
@@ -84,6 +85,7 @@ public PrestoServer(SqlParserOptions sqlParserOptions)
     public void run()
     {
         verifyJvmRequirements();
+        JdkBasedZoneInfoProvider.registerAsJodaZoneInfoProvider();
         verifySystemTimeIsReasonable();
 
         Logger log = Logger.get(PrestoServer.class);

presto-main/src/test/java/com/facebook/presto/util/TestTimeZoneUtils.java

@@ -13,42 +13,56 @@
  */
 package com.facebook.presto.util;
 
-import com.facebook.presto.server.JavaVersion;
 import com.facebook.presto.spi.type.TimeZoneKey;
-import com.google.common.collect.Sets;
+import io.airlift.jodabridge.JdkBasedZoneInfoProvider;
 import org.joda.time.DateTime;
 import org.joda.time.DateTimeZone;
+import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
-import java.util.Arrays;
-import java.util.TimeZone;
+import java.time.ZoneId;
 import java.util.TreeSet;
 
 import static com.facebook.presto.spi.type.DateTimeEncoding.packDateTimeWithZone;
 import static com.facebook.presto.spi.type.TimeZoneKey.isUtcZoneId;
 import static com.facebook.presto.util.DateTimeZoneIndex.getDateTimeZone;
 import static com.facebook.presto.util.DateTimeZoneIndex.packDateTimeWithZone;
 import static com.facebook.presto.util.DateTimeZoneIndex.unpackDateTimeZone;
-import static java.util.Locale.ENGLISH;
 import static org.testng.Assert.assertEquals;
 
 public class TestTimeZoneUtils
 {
+    @BeforeClass
+    protected void validateJodaZoneInfoProvider()
+    {
+        try {
+            JdkBasedZoneInfoProvider.registerAsJodaZoneInfoProvider();
+        }
+        catch (RuntimeException e) {
+            throw new RuntimeException("Set the following system property to JVM running the test: -Dorg.joda.time.DateTimeZone.Provider=com.facebook.presto.tz.JdkBasedZoneInfoProvider");
+        }
+    }
+
     @Test
     public void test()
     {
         TimeZoneKey.getTimeZoneKey("GMT-13:00");
 
         TreeSet<String> jodaZones = new TreeSet<>(DateTimeZone.getAvailableIDs());
-        TreeSet<String> jdkZones = new TreeSet<>(Arrays.asList(TimeZone.getAvailableIDs()));
+        TreeSet<String> jdkZones = new TreeSet<>(ZoneId.getAvailableZoneIds());
+        // We use JdkBasedZoneInfoProvider for joda
+        assertEquals(jodaZones, jdkZones);
 
-        for (String zoneId : new TreeSet<>(Sets.intersection(jodaZones, jdkZones))) {
-            if (zoneId.toLowerCase(ENGLISH).startsWith("etc/") || zoneId.toLowerCase(ENGLISH).startsWith("gmt")) {
+        for (String zoneId : new TreeSet<>(jdkZones)) {
+            if (zoneId.startsWith("Etc/") || zoneId.startsWith("GMT") || zoneId.startsWith("SystemV/")) {
                 continue;
             }
-            // Known bug in Joda(https://github.com/JodaOrg/joda-time/issues/427)
-            // We will skip this timezone in test
-            if (JavaVersion.current().getMajor() == 8 && JavaVersion.current().getUpdate().orElse(0) < 121 && zoneId.equals("Asia/Rangoon")) {
+
+            if (zoneId.equals("Canada/East-Saskatchewan")) {
+                // TODO: remove once minimum Java version is increased to 8u161 and 9.0.4, see PrestoSystemRequirement.
+                // Removed from tzdata since 2017c.
+                // Java updated to 2017c since 8u161, 9.0.4.
+                // All Java 10+ are on later versions
                 continue;
             }
 

presto-mongodb/pom.xml

@@ -30,6 +30,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>javax.validation</groupId>
             <artifactId>validation-api</artifactId>

presto-orc/pom.xml

@@ -71,6 +71,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>

presto-raptor/pom.xml

@@ -102,6 +102,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>javax.validation</groupId>
             <artifactId>validation-api</artifactId>

presto-rcfile/pom.xml

@@ -32,6 +32,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>

presto-record-decoder/pom.xml

@@ -51,6 +51,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>org.apache.avro</groupId>
             <artifactId>avro</artifactId>

presto-redis/pom.xml

@@ -67,6 +67,12 @@
             <artifactId>joda-time</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>joda-to-java-time-bridge</artifactId>
+            <scope>runtime</scope>
+        </dependency>
+
         <dependency>
             <groupId>javax.annotation</groupId>
             <artifactId>javax.annotation-api</artifactId>

presto-spi/src/main/resources/com/facebook/presto/spi/type/zone-index.properties

@@ -2186,13 +2186,13 @@
 2177 CST6CDT
 2178 Cuba
 2179 EET
-2180 EST
+# 2180 EST  # not in java.time
 2181 EST5EDT
 2182 Egypt
 2183 Eire
 2184 GB
 2185 GB-Eire
-2186 HST
+# 2186 HST  # not in java.time
 2187 Hongkong
 2188 Iceland
 2189 Iran
@@ -2202,7 +2202,7 @@
 2193 Kwajalein
 2194 Libya
 2195 MET
-2196 MST
+# 2196 MST  # not in java.time
 2197 MST7MDT
 2198 NZ
 2199 NZ-CHAT
@@ -2235,53 +2235,3 @@
 2226 Asia/Atyrau
 2227 Asia/Famagusta
 2228 Europe/Saratov
-
-# Zones not supported in Java
-# ROC
-
-# Zones not supported in Joda
-# ACT
-# AET
-# AGT
-# ART
-# AST
-# Asia/Khandyga
-# Asia/Riyadh87
-# Asia/Riyadh88
-# Asia/Riyadh89
-# BET
-# BST
-# CAT
-# CNT
-# CST
-# CTT
-# EAT
-# ECT
-# IET
-# IST
-# JST
-# MIT
-# Mideast/Riyadh87
-# Mideast/Riyadh88
-# Mideast/Riyadh89
-# NET
-# NST
-# PLT
-# PNT
-# PRT
-# PST
-# SST
-# SystemV/AST4
-# SystemV/AST4ADT
-# SystemV/CST6
-# SystemV/CST6CDT
-# SystemV/EST5
-# SystemV/EST5EDT
-# SystemV/HST10
-# SystemV/MST7
-# SystemV/MST7MDT
-# SystemV/PST8
-# SystemV/PST8PDT
-# SystemV/YST9
-# SystemV/YST9YDT
-# VST

presto-spi/src/test/java/com/facebook/presto/spi/type/TestTimeZoneKey.java

@@ -179,6 +179,15 @@ public void testZoneKeyIdRange()
         // previous spot for Canada/East-Saskatchewan
         assertFalse(hasValue[2040]);
         hasValue[2040] = true;
+        // previous spot for EST
+        assertFalse(hasValue[2180]);
+        hasValue[2180] = true;
+        // previous spot for HST
+        assertFalse(hasValue[2186]);
+        hasValue[2186] = true;
+        // previous spot for MST
+        assertFalse(hasValue[2196]);
+        hasValue[2196] = true;
 
         for (int i = 0; i < hasValue.length; i++) {
             assertTrue(hasValue[i], "There is no time zone with key " + i);
@@ -204,7 +213,7 @@ public int compare(TimeZoneKey left, TimeZoneKey right)
             hasher.putString(timeZoneKey.getId(), StandardCharsets.UTF_8);
         }
         // Zone file should not (normally) be changed, so let's make this more difficult
-        assertEquals(hasher.hash().asLong(), 4694133082746267068L, "zone-index.properties file contents changed!");
+        assertEquals(hasher.hash().asLong(), -4582158485614614451L, "zone-index.properties file contents changed!");
     }
 
     public void assertTimeZoneNotSupported(String zoneId)