Use JVM time zone rules in Joda #11891

haozhun · 2018-11-10T02:11:23Z

Fixes #8233, fixes #11855 depends on #11890

haozhun · 2018-11-10T02:25:48Z

Posted JodaOrg/joda-time#491 to request opinion from joda author

hellium01

I haven't really go into the conversion logics yet (needed to read a little about the implementation on joda/jdk code), I added some comments for other parts.

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedZoneInfoProvider.java

presto-main/src/test/java/com/facebook/presto/tz/TestJdkBasedZoneInfoProvider.java

hellium01 · 2018-11-10T02:50:30Z

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


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?

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

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

Can we have a test for zone-index contents?

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.

hellium01 · 2018-11-10T02:58:57Z

presto-main/src/test/java/com/facebook/presto/tz/TestJdkBasedZoneInfoProvider.java

+    // * static initializer of DateTimeZoneIndex
+
+    @BeforeClass
+    protected void registerFunctions()


See comment above, we probably don't need to rely on the system property. Joda allows you to override the default provider.

Sorry about the bad name.

This is not really setting it. It's more like validating it (as evidenced by the error message in catch). However, when you run this in IntelliJ, it does provide the convenience of setting it for you (only when doing so is safe).

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

findepi · 2018-11-10T09:56:10Z

presto-main/pom.xml

@@ -407,6 +407,9 @@
                    <excludes>
                        <exclude>**/TestLocalExecutionPlanner.java</exclude>
                    </excludes>
+                    <systemPropertyVariables>
+                        <org.joda.time.DateTimeZone.Provider>com.facebook.presto.tz.JdkBasedZoneInfoProvider</org.joda.time.DateTimeZone.Provider>


Does joda support service loader? Or any other way to make this be loaded more reliably?

https://github.com/JodaOrg/joda-time/blob/master/src/main/java/org/joda/time/DateTimeZone.java#L507-L548

findepi · 2018-11-10T09:57:58Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedDateTimeZone.java

+    @Override
+    public int hashCode()
+    {
+        return Objects.hash(super.hashCode(), zoneRules);


is hashcode compatible with equals? hashcode consults super's fields and equals does not (it would be correct for equals to do this)

There is no field in the super class. Making things even more interesting, super does have an implementation for hashCode, but not equals. This is very uncommon.

The equals and hashCode implementation here are generated by IntelliJ.

I think the implementation is correct as is. However, it indeed looks very suspicious.

Any concrete suggestions? @findepi @dain

Looking at the classes it appears that subclasses are not equals to each other, and instead you need an exact type match. Providing a default implementation for hashCode is a bit wonky and some of the subclasses override that also. I suggest we simply implement both equals and hashCode and we base them solely on getID().

Naive question -- do we need hashCode/equals at all?

The super class declared equals as abstract so it must be implemented.

Lets make this just return getID().hashCode();

presto-main/src/test/java/com/facebook/presto/tz/TestJdkBasedZoneInfoProvider.java

findepi · 2018-11-10T09:59:33Z

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


Can we have a test for zone-index contents?

dain · 2018-11-12T20:22:16Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedZoneInfoProvider.java

+        // Otherwise, the zone-internal cache won't be useful.
+        // Joda's default Provider implementation, ZoneInfoProvider, caches with SoftReference.
+        // This implementation didn't use SoftReference for simplicity.
+        zoneCache = CacheBuilder.newBuilder().build(new CacheLoader<String, DateTimeZone>()


Does it take a long time to load every time zone? If not, why not just load everything ahead of time?

Ran a quick test. On my mac from cold boot, this takes 250 ms, so I would just load everything up front:

@Test public void test() { JdkBasedZoneInfoProvider jdkBasedZoneInfoProvider = new JdkBasedZoneInfoProvider(); Set<String> availableIDs = jdkBasedZoneInfoProvider.getAvailableIDs(); for (String availableID : availableIDs) { jdkBasedZoneInfoProvider.getZone(availableID); } }

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

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedDateTimeZone.java

presto-main/src/test/java/com/facebook/presto/tz/TestJdkBasedZoneInfoProvider.java

dain · 2018-11-12T21:34:13Z

One more thing. The DateTimeZone class is Serializable, so I would either make that fail or test that it works as expected.

haozhun · 2018-11-13T01:42:04Z

One more thing. The DateTimeZone class is Serializable, so I would either make that fail or test that it works as expected.

Can you tell me how to do this right?

dain · 2018-11-13T06:28:25Z

One more thing. The DateTimeZone class is Serializable, so I would either make that fail or test that it works as expected.

Can you tell me how to do this right?

I just read through the existing serialization code, and it appears to be doing the correct thing. When a DateTimeZone is serialized it is replaced with a class Stub, which contains the String id of the time zone, and when Stub is deserialized it replaces itself with DateTimeZone.forID(iId). So we are good.

dain

There are some follow up comments, but this looking good to me.

dain · 2018-11-13T19:15:58Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedDateTimeZone.java

+    @Override
+    public int hashCode()
+    {
+        return Objects.hash(super.hashCode(), zoneRules);


The super class declared equals as abstract so it must be implemented.

dain · 2018-11-13T19:18:57Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedZoneInfoProvider.java

+public class JdkBasedZoneInfoProvider
+        implements Provider
+{
+    private final Map<String, DateTimeZone> zones;


Is there any reason to not make this static?

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedZoneInfoProvider.java

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

haozhun

Comments addressed, @dain

haozhun · 2018-11-14T22:59:21Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedDateTimeZone.java

+            // this is before the first transition
+            return instant;
+        }
+        return previousTransition.toEpochSecond() * 1000 - 1;


In addition to addressing your comments, I added + 1 and - 1 here. I also added a bunch of tests. Please take a look.

Please add a comment here.

dain

Some minor comments. There are still build failures.

dain · 2018-11-16T19:04:27Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedDateTimeZone.java

+            // this is before the first transition
+            return instant;
+        }
+        return previousTransition.toEpochSecond() * 1000 - 1;


Please add a comment here.

dain · 2018-11-16T19:05:12Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedDateTimeZone.java

+            return false;
+        }
+        JdkBasedDateTimeZone that = (JdkBasedDateTimeZone) o;
+        return Objects.equals(zoneRules, that.zoneRules);


Lets make this just return getID().equals(that.getId());

dain · 2018-11-16T19:05:52Z

presto-main/src/main/java/com/facebook/presto/tz/JdkBasedDateTimeZone.java

+    @Override
+    public int hashCode()
+    {
+        return Objects.hash(super.hashCode(), zoneRules);


Lets make this just return getID().hashCode();

haozhun · 2018-11-16T20:53:10Z

Not ready to merge due to issues related to spi and class loader.

haozhun · 2018-11-19T19:57:24Z

I talked to @dain and @electrum separately offline about this. To summarize,

Proposed Goals:

Presto will continue to support usage of both joda and java.time in connectors. (We recommend joda, but we don't want to force everyone to make the switch.)
Presto will continue to support usage of any reasonable joda version in connectors.
Incorrect results due to inconsistency of tzdata between joda and java.time needs to be eliminated.

Proposed Approach:

The supporting classes in this PR will be put in a library in airlift.
- The library will depend on joda as a "provided" dependency.
- The library will presumably work with any reasonable version of joda.
All connectors that uses joda must depend on this library as a "runtime" dependency.
- Otherwise, connector will see a different tzdata, potentially violating goal 3.
- Presto-main will depend on the library and set the JVM system property. As a result, if a connector doesn't depend on it, it will fail loudly (albeit with a rather unhelpful error message about class loading failure).

Question:

What should the library be named?

dain · 2018-11-19T20:39:12Z

What should the library be named?

JodaToJavaTimeBridge?

findepi · 2018-11-19T21:57:01Z

The supporting classes in this PR will be put in a library in airlift.

What is the benefit of putting this in airlift-joda-javatime-bridge vs presto-joda-javatime-bridge?

haozhun · 2018-11-19T23:15:18Z

@dain airlift/airlift#698

@findepi I'm ok either way. @electrum may want to chime in.

haozhun · 2018-12-13T02:06:12Z

@dain I made significant change to this PR. Please take another look. cc @findepi

dain

Looks good to me.

@electrum do you have any comments on the documentation change?

dain · 2018-12-15T21:13:25Z

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

+    registers a custom Joda-Time zone provider at startup time, which makes
+    Joda-Time delegate to 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.


cc @electrum ^^^

electrum

Documentation looks good

electrum · 2018-12-17T18:11:02Z

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

+
+.. note::
+
+    For any connector that uses Joda-Time, it must declare a runtime


Use code quotes for "runtime"

electrum · 2018-12-17T18:11:14Z

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

+    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. As a result, to avoid incorrect results, Presto


As a result -> Thus

(avoid using "result" twice in slightly different ways)

electrum · 2018-12-17T18:11:53Z

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

+    Presto internally uses ``java.time`` for certain date/time
+    computations. As a result, to avoid incorrect results, Presto
+    registers a custom Joda-Time zone provider at startup time, which makes
+    Joda-Time delegate to JVM's built-in zone rules. The zone provider


to JVM -> to the JVM

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.

haozhun assigned electrum, dain and hellium01 Nov 10, 2018

haozhun requested review from electrum, dain and hellium01 November 10, 2018 02:11

facebook-github-bot added the CLA Signed label Nov 10, 2018

haozhun assigned martint Nov 10, 2018

haozhun requested a review from martint November 10, 2018 02:28

hellium01 reviewed Nov 10, 2018

View reviewed changes

findepi reviewed Nov 10, 2018

View reviewed changes

dain suggested changes Nov 12, 2018

View reviewed changes

haozhun force-pushed the joda2 branch from cc4e7b9 to 1b3d27c Compare November 13, 2018 01:46

dain suggested changes Nov 13, 2018

View reviewed changes

haozhun force-pushed the joda2 branch 3 times, most recently from ca2eb59 to 1856039 Compare November 14, 2018 22:57

haozhun commented Nov 14, 2018

View reviewed changes

dain approved these changes Nov 16, 2018

View reviewed changes

haozhun force-pushed the joda2 branch 2 times, most recently from 95b4757 to 4de19a2 Compare November 16, 2018 20:52

haozhun unassigned electrum Nov 19, 2018

haozhun force-pushed the joda2 branch from 4de19a2 to 25a10b4 Compare December 13, 2018 02:05

haozhun force-pushed the joda2 branch from 25a10b4 to 7bdc54e Compare December 13, 2018 21:25

dain approved these changes Dec 15, 2018

View reviewed changes

electrum reviewed Dec 17, 2018

View reviewed changes

haozhun force-pushed the joda2 branch from 7bdc54e to ce5bc94 Compare December 17, 2018 22:55

haozhun force-pushed the joda2 branch from ce5bc94 to 8ac4b84 Compare December 17, 2018 22:58

haozhun merged commit 8ac4b84 into prestodb:master Dec 17, 2018

haozhun mentioned this pull request Dec 17, 2018

Release notes for 0.216 #12081

Closed

14 tasks


		.. note::

		For any connector that uses Joda-Time, it must declare a runtime

Use JVM time zone rules in Joda #11891

Use JVM time zone rules in Joda #11891

Conversation

haozhun commented Nov 10, 2018 • edited by findepi Loading

haozhun commented Nov 10, 2018

hellium01 left a comment

Choose a reason for hiding this comment

hellium01 Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haozhun Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

hellium01 Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haozhun Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dain commented Nov 12, 2018

haozhun commented Nov 13, 2018

dain commented Nov 13, 2018

dain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haozhun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haozhun commented Nov 16, 2018

haozhun commented Nov 19, 2018 • edited Loading

dain commented Nov 19, 2018

findepi commented Nov 19, 2018

haozhun commented Nov 19, 2018 • edited Loading

haozhun commented Dec 13, 2018

dain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haozhun commented Nov 10, 2018 •

edited by findepi

Loading

hellium01 Nov 10, 2018 •

edited

Loading

haozhun Nov 13, 2018 •

edited

Loading

hellium01 Nov 10, 2018 •

edited

Loading

haozhun Nov 13, 2018 •

edited

Loading

haozhun commented Nov 19, 2018 •

edited

Loading

haozhun commented Nov 19, 2018 •

edited

Loading