Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use JVM time zone rules in Joda #11891

Merged
merged 1 commit into from
Dec 17, 2018
Merged

Use JVM time zone rules in Joda #11891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-accumulo/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-base-jdbc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-cassandra/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
11 changes: 11 additions & 0 deletions presto-docs/src/main/sphinx/develop/connectors.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
6 changes: 6 additions & 0 deletions presto-hive/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-kafka/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-kudu/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
8 changes: 8 additions & 0 deletions presto-main/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
2 changes: 2 additions & 0 deletions presto-main/src/main/java/com/facebook/presto/server/PrestoServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -84,6 +85,7 @@ public PrestoServer(SqlParserOptions sqlParserOptions)
public void run()
{
verifyJvmRequirements();
JdkBasedZoneInfoProvider.registerAsJodaZoneInfoProvider();
verifySystemTimeIsReasonable();

Logger log = Logger.get(PrestoServer.class);
Expand Down
36 changes: 25 additions & 11 deletions presto-main/src/test/java/com/facebook/presto/util/TestTimeZoneUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
6 changes: 6 additions & 0 deletions presto-mongodb/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-orc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-raptor/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-rcfile/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-record-decoder/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 6 additions & 0 deletions presto-redis/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
56 changes: 3 additions & 53 deletions presto-spi/src/main/resources/com/facebook/presto/spi/type/zone-index.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2186,13 +2186,13 @@
2177 CST6CDT
2178 Cuba
2179 EET
2180 EST
# 2180 EST # not in java.time
Copy link
Contributor

@hellium01 hellium01 Nov 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should removed it probably from a while back. Currently, when we run following query in presto-cli:

SELECT CAST('2018-10-28 14:44:59.000 EST' AS TIMESTAMP WITH TIME ZONE)

we got error:

Error fetching next at https://[2401:db00:1010:811a:face:0:1:0]:7778/v1/statement/20181110_024907_49716_ysdca/2 returned an invalid response: JsonResponse
java.time.zone.ZoneRulesException: Unknown time-zone ID: EST

How do you find out which timezone to be removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "2040 Canada/East-Saskatchewan" also got removed in 2017
http://mm.icann.org/pipermail/tz-announce/2017-October/000047.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume TestTimeZoneUtils won't be very useful anymore since joda/jdk timezones are unified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test for zone-index contents?

Copy link
Contributor Author

@haozhun haozhun Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test for zone-index contents?

We already have one in TestTimeZoneUtils. I updated it.

think "2040 Canada/East-Saskatchewan" also got removed in 2017

Updated in the preceding commit.

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
Expand All @@ -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
Expand Down Expand Up @@ -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
11 changes: 10 additions & 1 deletion presto-spi/src/test/java/com/facebook/presto/spi/type/TestTimeZoneKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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)
Expand Down
Loading