From 8ac4b8466a7db96d9941e27cecf7aaf64c85bfaf Mon Sep 17 00:00:00 2001 From: Haozhun Jin Date: Fri, 9 Nov 2018 18:05:30 -0800 Subject: [PATCH] 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 | 6 ++ presto-accumulo/pom.xml | 6 ++ presto-base-jdbc/pom.xml | 6 ++ presto-cassandra/pom.xml | 6 ++ .../src/main/sphinx/develop/connectors.rst | 11 ++++ presto-hive/pom.xml | 6 ++ presto-kafka/pom.xml | 6 ++ presto-kudu/pom.xml | 6 ++ presto-main/pom.xml | 8 +++ .../facebook/presto/server/PrestoServer.java | 2 + .../presto/util/TestTimeZoneUtils.java | 36 ++++++++---- presto-mongodb/pom.xml | 6 ++ presto-orc/pom.xml | 6 ++ presto-raptor/pom.xml | 6 ++ presto-rcfile/pom.xml | 6 ++ presto-record-decoder/pom.xml | 6 ++ presto-redis/pom.xml | 6 ++ .../presto/spi/type/zone-index.properties | 56 +------------------ .../presto/spi/type/TestTimeZoneKey.java | 11 +++- presto-teradata-functions/pom.xml | 6 ++ presto-tpcds/pom.xml | 6 ++ 21 files changed, 149 insertions(+), 65 deletions(-) diff --git a/pom.xml b/pom.xml index fb194ef063040..d154ebb7080d9 100644 --- a/pom.xml +++ b/pom.xml @@ -592,6 +592,12 @@ 2.1.5.1 + + io.airlift + joda-to-java-time-bridge + 1 + + io.airlift.drift drift-api diff --git a/presto-accumulo/pom.xml b/presto-accumulo/pom.xml index ef095bbd1f717..23d8cfaa8d98a 100644 --- a/presto-accumulo/pom.xml +++ b/presto-accumulo/pom.xml @@ -245,6 +245,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + org.apache.zookeeper zookeeper diff --git a/presto-base-jdbc/pom.xml b/presto-base-jdbc/pom.xml index 80ff10d3c4178..4f3bd39efae92 100644 --- a/presto-base-jdbc/pom.xml +++ b/presto-base-jdbc/pom.xml @@ -68,6 +68,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + javax.inject javax.inject diff --git a/presto-cassandra/pom.xml b/presto-cassandra/pom.xml index a8d149a2ba41e..1baa35de2d36d 100644 --- a/presto-cassandra/pom.xml +++ b/presto-cassandra/pom.xml @@ -77,6 +77,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + javax.inject javax.inject diff --git a/presto-docs/src/main/sphinx/develop/connectors.rst b/presto-docs/src/main/sphinx/develop/connectors.rst index 6d2493bb1f738..68a83da72f596 100644 --- a/presto-docs/src/main/sphinx/develop/connectors.rst +++ b/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 `_. + 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. diff --git a/presto-hive/pom.xml b/presto-hive/pom.xml index 6a87cb872d001..72741b0632d00 100644 --- a/presto-hive/pom.xml +++ b/presto-hive/pom.xml @@ -144,6 +144,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + com.amazonaws aws-java-sdk-core diff --git a/presto-kafka/pom.xml b/presto-kafka/pom.xml index e978caf6470d0..8f9f569c29861 100644 --- a/presto-kafka/pom.xml +++ b/presto-kafka/pom.xml @@ -67,6 +67,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + javax.annotation javax.annotation-api diff --git a/presto-kudu/pom.xml b/presto-kudu/pom.xml index 1c336581ab1c8..198567b838b14 100644 --- a/presto-kudu/pom.xml +++ b/presto-kudu/pom.xml @@ -53,6 +53,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + io.airlift configuration diff --git a/presto-main/pom.xml b/presto-main/pom.xml index a74a13b2705c9..fdccb75a092d8 100644 --- a/presto-main/pom.xml +++ b/presto-main/pom.xml @@ -186,6 +186,11 @@ joni + + io.airlift + joda-to-java-time-bridge + + com.teradata re2j-td @@ -401,6 +406,9 @@ **/TestLocalExecutionPlanner.java + + io.airlift.jodabridge.JdkBasedZoneInfoProvider + diff --git a/presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java b/presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java index 4664d7e0488eb..86bcbac4358c4 100644 --- a/presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java +++ b/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); diff --git a/presto-main/src/test/java/com/facebook/presto/util/TestTimeZoneUtils.java b/presto-main/src/test/java/com/facebook/presto/util/TestTimeZoneUtils.java index 2d4d9ce8dd4c9..3d95f47be7256 100644 --- a/presto-main/src/test/java/com/facebook/presto/util/TestTimeZoneUtils.java +++ b/presto-main/src/test/java/com/facebook/presto/util/TestTimeZoneUtils.java @@ -13,15 +13,14 @@ */ 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; @@ -29,26 +28,41 @@ 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 jodaZones = new TreeSet<>(DateTimeZone.getAvailableIDs()); - TreeSet jdkZones = new TreeSet<>(Arrays.asList(TimeZone.getAvailableIDs())); + TreeSet 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; } diff --git a/presto-mongodb/pom.xml b/presto-mongodb/pom.xml index d04001b0a5740..332ae3d53cc99 100644 --- a/presto-mongodb/pom.xml +++ b/presto-mongodb/pom.xml @@ -30,6 +30,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + javax.validation validation-api diff --git a/presto-orc/pom.xml b/presto-orc/pom.xml index 476776f6ae8cf..ad3a85d20c8ef 100644 --- a/presto-orc/pom.xml +++ b/presto-orc/pom.xml @@ -71,6 +71,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + com.google.guava guava diff --git a/presto-raptor/pom.xml b/presto-raptor/pom.xml index 2959080ea0da2..24bbef8a19944 100644 --- a/presto-raptor/pom.xml +++ b/presto-raptor/pom.xml @@ -102,6 +102,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + javax.validation validation-api diff --git a/presto-rcfile/pom.xml b/presto-rcfile/pom.xml index cfec021b16c5a..2f5f081b7af9b 100644 --- a/presto-rcfile/pom.xml +++ b/presto-rcfile/pom.xml @@ -32,6 +32,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + com.google.guava guava diff --git a/presto-record-decoder/pom.xml b/presto-record-decoder/pom.xml index 771f6911c9bf7..9e827191ca9f2 100644 --- a/presto-record-decoder/pom.xml +++ b/presto-record-decoder/pom.xml @@ -51,6 +51,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + org.apache.avro avro diff --git a/presto-redis/pom.xml b/presto-redis/pom.xml index 8d7c3fee745bb..524389f05fa2f 100644 --- a/presto-redis/pom.xml +++ b/presto-redis/pom.xml @@ -67,6 +67,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + javax.annotation javax.annotation-api diff --git a/presto-spi/src/main/resources/com/facebook/presto/spi/type/zone-index.properties b/presto-spi/src/main/resources/com/facebook/presto/spi/type/zone-index.properties index 163391699f643..6b3b40afbf463 100644 --- a/presto-spi/src/main/resources/com/facebook/presto/spi/type/zone-index.properties +++ b/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 diff --git a/presto-spi/src/test/java/com/facebook/presto/spi/type/TestTimeZoneKey.java b/presto-spi/src/test/java/com/facebook/presto/spi/type/TestTimeZoneKey.java index 023a70c6b8e83..58242426eb613 100644 --- a/presto-spi/src/test/java/com/facebook/presto/spi/type/TestTimeZoneKey.java +++ b/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) diff --git a/presto-teradata-functions/pom.xml b/presto-teradata-functions/pom.xml index 1cfb70fc33ead..4e55e53b42db5 100644 --- a/presto-teradata-functions/pom.xml +++ b/presto-teradata-functions/pom.xml @@ -31,6 +31,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + org.antlr antlr4-runtime diff --git a/presto-tpcds/pom.xml b/presto-tpcds/pom.xml index c4321888815b4..6b7acdb0a1878 100644 --- a/presto-tpcds/pom.xml +++ b/presto-tpcds/pom.xml @@ -31,6 +31,12 @@ joda-time + + io.airlift + joda-to-java-time-bridge + runtime + + com.fasterxml.jackson.core jackson-databind