Fix handling of dates and timestamps before the 20th century #4563

aalbu · 2020-07-24T10:43:18Z

No description provided.

losipiuk · 2020-07-24T11:25:13Z

nit: commit message summary to long

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java

findepi · 2020-07-27T07:46:58Z

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java

+    // the default date when the Gregorian calendar was instituted (October 15, 1582)
+    private static final long GREGORIAN_CALENDAR_INTRODUCTION = new GregorianCalendar().getGregorianChange().getTime();


What do you mean by "the default" here?

The cutoff date defaults to 1582-10-15, but you are not using a constant.
Instead youre asking for locale/env specific date. Maybe

// The date when the Gregorian calendar was instituted, environment specific. Defaults to October 15, 1582.

?

It is a constant. The actual change happened on different dates, in different countries, but GregorianCalendar doesn't have information for specific locales. Instead, it provides a method the client can call to specify the cutover date (setGregorianChange()). So unless that method is called, getGregorianChange() will always return the same value.

Anyway, after adding some more tests, it turns out that Joda millisecond values are not consistent with java.sql.Date even for more recent dates (as late as the 1800's), so I decided to use Joda only starting with the 20th century.

Anyway, after adding some more tests, it turns out that Joda millisecond values are not consistent with java.sql.Date even for more recent dates (as late as the 1800's), so I decided to use Joda only starting with the 20th century.

I forgot about that, but indeed. I think i remember finding a zone where java time and java.util.Calendar differed > 1900 (but before 1950).
java time and java.util.Calendar use the same tz data file, but they seem to parse it differently.

... I can only find as recent as 1890s in multiple zones (eg Europe/Warsaw or Asia/Aden), so maybe my memory is wrong. Using 1900 seems to be safe.

java time and joda seem to behave the same as long as they have the same tz data. But tz data shouldn't change for the past date/times.

This is some variation of the test code i am using: https://github.com/findepi/urandom-bits/blob/master/src/main/java/findepi/time/JodaJdkZoneDrift.java

Nice, that's good info.

findepi · 2020-07-27T07:49:47Z

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java

+            if (millis >= GREGORIAN_CALENDAR_INTRODUCTION) {
+                return new Date(millis);
+            }
+            // the chronology used by default by Joda is not appropriate for dates preceding the introduction of the Gregorian calendar,


it is "appropriate" in some way. I think it would be better to indicate the actual problem

Suggested change

// the chronology used by default by Joda is not appropriate for dates preceding the introduction of the Gregorian calendar,

// The chronology used by Joda is not consistent with java.sql.Date

for dates preceding the introduction of the Gregorian calendar.

// Same millisecond value represents a different year/month/day.

The documentation says:

it is not historically accurate before 1583

I like the "historically accurate" term

of course it's not historically accurate for >1583 for some (many) countries as well

findepi · 2020-07-27T07:50:45Z

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java

+            }
+            // the chronology used by default by Joda is not appropriate for dates preceding the introduction of the Gregorian calendar,
+            // so for such cases we are falling back to the more expensive GregorianCalendar; note that Joda also has a chronology that
+            // works for older dates, but it uses a slightly different algorithm, so we are sticking with the standard library


What are the end user visible symptoms of these differences? are they test covered?

The assertions I have added for dates before the introduction of the Gregorian calendar were failing using Joda's chronology.

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java

electrum · 2020-07-29T01:22:14Z

Please update the commit description to be a single line, without an asterisk:

Fixes handling of dates and timestamps before the 20th century.

The body should be wrapped at 72 characters and provide explanatory text. It's fine to use a bulleted list if that's the best way to explain something, but using a single bullet with nothing else looks strange. See https://chris.beams.io/posts/git-commit/

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java

electrum · 2020-07-29T01:31:41Z

presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcResultSet.java

@@ -193,6 +225,45 @@ public void testObjectTypes()
            assertEquals(rs.getTimestamp(column), Timestamp.valueOf(LocalDateTime.of(2018, 2, 13, 13, 14, 15, 555_555_556)));
        });

+        // distant past, but apparently not an uncommon value in practice
+        checkRepresentation("TIMESTAMP '0001-01-01 00:00:00'", Types.TIMESTAMP, (rs, column) -> {
+            assertEquals(rs.getObject(column), Timestamp.valueOf(LocalDateTime.of(1, 1, 1, 0, 0, 0)));


I realize the others do this, but it'd be better to factor this out

Timestamp expected = Timestamp.valueOf(LocalDateTime.of(1, 1, 1, 0, 0, 0);

That way it's clear to the reader that these are the same value.

Others do it to avoid sharing a mutable objected as the expected state.

findepi · 2020-07-30T06:58:33Z

Merged, thanks!

cla-bot bot added the cla-signed label Jul 24, 2020

aalbu requested a review from findepi July 24, 2020 10:43

losipiuk reviewed Jul 24, 2020

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Show resolved Hide resolved

electrum reviewed Jul 24, 2020

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Outdated Show resolved Hide resolved

aalbu force-pushed the jdbc-old-dates branch from c242bbb to 1fe2dfb Compare July 27, 2020 02:37

findepi approved these changes Jul 27, 2020

View reviewed changes

aalbu force-pushed the jdbc-old-dates branch from 1fe2dfb to efefad5 Compare July 28, 2020 03:11

findepi reviewed Jul 28, 2020

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Outdated Show resolved Hide resolved

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Outdated Show resolved Hide resolved

aalbu force-pushed the jdbc-old-dates branch 2 times, most recently from 235660c to e9fcffe Compare July 28, 2020 15:58

electrum changed the title ~~Handle dates/timestamps preceding the introduction of the Gregorian c…~~ Handle dates preceding the introduction of the Gregorian calendar calendar Jul 29, 2020

electrum reviewed Jul 29, 2020

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Outdated Show resolved Hide resolved

electrum reviewed Jul 29, 2020

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Outdated Show resolved Hide resolved

electrum reviewed Jul 29, 2020

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Outdated Show resolved Hide resolved

presto-jdbc/src/main/java/io/prestosql/jdbc/AbstractPrestoResultSet.java Outdated Show resolved Hide resolved

electrum reviewed Jul 29, 2020

View reviewed changes

aalbu force-pushed the jdbc-old-dates branch from e9fcffe to b2909b6 Compare July 29, 2020 18:45

Fix handling of dates and timestamps before the 20th century

70c3998

aalbu force-pushed the jdbc-old-dates branch from b2909b6 to 70c3998 Compare July 30, 2020 02:08

findepi merged commit 4efe924 into trinodb:master Jul 30, 2020

findepi changed the title ~~Handle dates preceding the introduction of the Gregorian calendar calendar~~ Fix handling of dates and timestamps before the 20th century Jul 30, 2020

findepi mentioned this pull request Jul 30, 2020

Release notes for 340 #4527

Closed

8 tasks

findepi added this to the 340 milestone Jul 30, 2020

findepi mentioned this pull request Mar 12, 2021

Invalid Date, Timestamp returned from JDBC for date times before 1582 #4018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of dates and timestamps before the 20th century #4563

Fix handling of dates and timestamps before the 20th century #4563

aalbu commented Jul 24, 2020 •

edited by electrum

Loading

losipiuk commented Jul 24, 2020

findepi Jul 27, 2020

aalbu Jul 28, 2020

findepi Jul 28, 2020

aalbu Jul 28, 2020

findepi Jul 27, 2020

aalbu Jul 28, 2020

findepi Jul 28, 2020

findepi Jul 28, 2020

findepi Jul 27, 2020

aalbu Jul 28, 2020

electrum commented Jul 29, 2020

electrum Jul 29, 2020

findepi Jul 29, 2020

findepi commented Jul 30, 2020

		// the default date when the Gregorian calendar was instituted (October 15, 1582)
		private static final long GREGORIAN_CALENDAR_INTRODUCTION = new GregorianCalendar().getGregorianChange().getTime();

-            // the chronology used by default by Joda is not appropriate for dates preceding the introduction of the Gregorian calendar,
+            // The chronology used by Joda is not consistent with java.sql.Date
+             for dates preceding the introduction of the Gregorian calendar.
+            // Same millisecond value represents a different year/month/day.

Fix handling of dates and timestamps before the 20th century #4563

Fix handling of dates and timestamps before the 20th century #4563

Conversation

aalbu commented Jul 24, 2020 • edited by electrum Loading

losipiuk commented Jul 24, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Jul 30, 2020

aalbu commented Jul 24, 2020 •

edited by electrum

Loading