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.

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

fix: use 'time with time zone' and 'timestamp with time zone' values as is and avoid computation with user-provided/default Calendars #1053

Merged
merged 2 commits into from Jan 8, 2018

Conversation

Projects
None yet
4 participants
@vlsi
Member

vlsi commented Jan 3, 2018

Previous behavior:
getTime(int), getTime(int, Calendar) kind of methods ensured that the resulting java.sql.Time
would render as 1970-01-01 ... in the given time zone (e.g. given Calendar and/or default TimeZone)
Note: the resulting Time object

E.g. if server was sending 1970-01-01 15:00:00 +0300 and getTime(int, GMT+13) was used by the client, then pgjdbc subtracted a day since otherwise the timestamp is equal to 1970-01-02 01:00:00 +1300 (note the date is 2).

New behavior:
for 'time with time zone' and 'timestamp with time zone' the value from the server is assumed to be
"instant" (i.e. absolute point in time), and no further adjustments is made to make the date part to be 1970-01-01.
For instance, 1970-01-01 15:00:00 +0300 is parsed as is (no matter which Calendar is provided to getTime(...)).

'time' and 'timestamp' work as earlier except "00:00:00" and "24:00:00" times in text format.
Previously, text times "00:00:00" and "24:00:00" were mapped to Time(0) and Time.valueOf("24:00:00"):

  1. Time(0) is 1970-01-01 00:00:00 UTC (it does not account neither user-provided nor default Calendar)
  2. Time.valueOf("24:00:00") uses system-provided calendar, and it does not account user-provided one
@davecramer

This comment has been minimized.

Member

davecramer commented Jan 3, 2018

What are we fixing here ?

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 3, 2018

See changes to TimezoneTest.java.
For instance, lines 287..302

// timestamptz: 2005-01-01 15:00:00+03
t = rs.getTime(1);
// 2005-01-01 13:00:00 +0100 -> 1970-01-01 13:00:00 +0100
assertEquals(43200000L, t.getTime());
t = rs.getTime(1, cUTC);
// 2005-01-01 12:00:00 +0000 -> 1970-01-01 12:00:00 +0000
assertEquals(43200000L, t.getTime());
t = rs.getTime(1, cGMT03);
// 2005-01-01 15:00:00 +0300 -> 1970-01-01 15:00:00 +0300
assertEquals(43200000L, t.getTime());
t = rs.getTime(1, cGMT05);
// 2005-01-01 07:00:00 -0500 -> 1970-01-01 07:00:00 -0500
assertEquals(43200000L, t.getTime());
t = rs.getTime(1, cGMT13);
// 2005-01-02 01:00:00 +1300 -> 1970-01-01 01:00:00 +1300
assertEquals(-43200000L, t.getTime());

Note how the resulting Time flips as the passed Calendar is equal to cGMT13 (the 'expected' value flips to a negative one)

I do not think that flip was a good thing, even though it has been there since 2005:

assertEquals(-43200000L, t.getTime()); // 2005-01-02 01:00:00 +1300 -> 1970-01-01 01:00:00 +1300

The reasoning is "backend sends absolute time", so there is a reason to trust it for timetz.
Note: this is a time-only change. timestamps, dates are not considered here.

@davecramer

This comment has been minimized.

Member

davecramer commented Jan 3, 2018

hmmm ok, I see your point...

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 3, 2018

Technically speaking, this PR should not alter rendering of the java.sql.Time object (as only hour, minute, second) components from Time make sense.

The change is to avoid 1 day flipping for certain time zones and time values when dealing with timetz/timestamptz.

@bokken

This comment has been minimized.

Member

bokken commented Jan 4, 2018

So you are suggesting for a timestamptz column, getTime should return a java.sql.Time object that effectively takes the hours, minutes, seconds of the point in time in UTC and converts that to Jan 1, 1970 (still UTC) regardless of what Calendar is provided?

@bokken

This comment has been minimized.

Member

bokken commented Jan 4, 2018

It might be worth adding the changes from https://github.com/pgjdbc/pgjdbc/pull/1049/files#diff-0daf95dda12fb66362e30c45a225c582
(likely leaving out the test2400Time test).
This provides coverage with different default time zones in both string and binary format, which is where I had found some of the issues.

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 4, 2018

So you are suggesting for a timestamptz column, getTime should return a java.sql.Time object that effectively takes the hours, minutes, seconds of the point in time in UTC and converts that to Jan 1, 1970 (still UTC) regardless of what Calendar is provided?

Time is suppose to be 1970-01-01 afaik.

@bokken

This comment has been minimized.

Member

bokken commented Jan 4, 2018

Time is suppose to be 1970-01-01 afaik.

I agree. But when going from absolute point in time to a "time", some concept of time zone has to be used. What you have proposed will always use UTC for a timestamptz column, regardless of provided Calendar. Since timestamptz are absolute points in time, my expectation was that calling getTime would result in the "time" being in either the provided calendar (or default calendar if none provided).

For example, lets start with timestamptz 2011-07-08 12:00:00 -06
I would expect the flow would be to get an absolute point in time from this:

    final TimeZone timeZone = TimeZone.getTimeZone("GMT-6:00");
    Calendar cal = Calendar.getInstance(timeZone);
    cal.set(Calendar.YEAR, 2011);
    cal.set(Calendar.MONTH, 6);
    cal.set(Calendar.DAY_OF_MONTH, 8);
    cal.set(Calendar.HOUR_OF_DAY, 12);
    cal.set(Calendar.MINUTE, 0);
    cal.set(Calendar.SECOND, 0);
    cal.set(Calendar.MILLISECOND, 0);
    Timestamp timestamp = new Timestamp(cal.getTimeInMillis());

If getTime is called with Calendar/TimeZone GMT-0600 I would expect the following time back:

    final TimeZone timeZone = TimeZone.getTimeZone("GMT-6:00");
    Calendar cal = Calendar.getInstance(timeZone);
    cal.setTimeInMillis(timestamp.getTime());
    cal.set(Calendar.YEAR, 1970);
    cal.set(Calendar.MONTH, 0);
    cal.set(Calendar.DAY_OF_MONTH, 1);
    Time time = new Time(cal.getTimeInMillis());

If getTime is called with Calendar/TimeZone of GMT+0600 I would expect the following back:

    final TimeZone timeZone = TimeZone.getTimeZone("GMT+6:00");
    Calendar cal = Calendar.getInstance(timeZone);
    cal.setTimeInMillis(timestamp.getTime());
    cal.set(Calendar.YEAR, 1970);
    cal.set(Calendar.MONTH, 0);
    cal.set(Calendar.DAY_OF_MONTH, 1);
    Time time = new Time(cal.getTimeInMillis());

If getTime is called with no Calendar/TimeZone, I would expect:

    Calendar cal = Calendar.getInstance();
    cal.setTimeInMillis(timestamp.getTime());
    cal.set(Calendar.YEAR, 1970);
    cal.set(Calendar.MONTH, 0);
    cal.set(Calendar.DAY_OF_MONTH, 1);
    Time time = new Time(cal.getTimeInMillis());

If called with any of the above Calendar/TimeZones, I would not expect this:

    final TimeZone timeZone = TimeZone.getTimeZone("UTC");
    Calendar cal = Calendar.getInstance();
    cal.setTimeInMillis(timestamp.getTime());
    cal.set(Calendar.YEAR, 1970);
    cal.set(Calendar.MONTH, 0);
    cal.set(Calendar.DAY_OF_MONTH, 1);
    Time time = new Time(cal.getTimeInMillis());

If I read what you are suggesting, this is what would be the result.

java.sql.Time really sucks as an API, and I am not stating with certainty that my expectations are correct. And maybe the moral of the story here is to just not use getTime on timestamptz columns. Call getTimestamp and then massage it on your own.

@codecov-io

This comment has been minimized.

codecov-io commented Jan 8, 2018

Codecov Report

Merging #1053 into master will decrease coverage by 0.46%.
The diff coverage is 58.33%.

@@             Coverage Diff              @@
##             master    #1053      +/-   ##
============================================
- Coverage     67.03%   66.57%   -0.47%     
+ Complexity     3647     3617      -30     
============================================
  Files           170      170              
  Lines         15616    15613       -3     
  Branches       2525     2519       -6     
============================================
- Hits          10468    10394      -74     
- Misses         3952     4009      +57     
- Partials       1196     1210      +14
@bokken

This comment has been minimized.

Member

bokken commented Jan 8, 2018

For the sake of getting the 42.2 release out, I say move forward with this PR as is (maybe add the parameterized test to cover more timezones). The change I am proposing on use of Calendars on timestamptz columns is not only different than historical behavior, it still would not match how java 8 defines LocalTime -> java.sql.Time (#1050).

fix: use 'time with time zone' and 'timestamp with time zone' values …
…as is and avoid computation with user-provided/default Calendars

Previous behavior:
getTime(int), getTime(int, Calendar) kind of methods ensured that the resulting java.sql.Time
would render as 1970-01-01 ... in the given time zone (e.g. given Calendar and/or default TimeZone)
Note: the resulting Time object

New behavior:
for 'time with time zone' and 'timestamp with time zone' the value from the server is assumed to be
"instant" (i.e. absolute point in time), and no further adjustments is made to make the date part to be 1970-01-01

'time' and 'timestamp' work as earlier except "00:00:00" and "24:00:00" times in text format.
Previously, text times "00:00:00" and "24:00:00" were mapped to Time(0) and Time.valueOf("24:00:00"):
1) Time(0) is 1970-01-01 00:00:00 UTC (it does not account neither user-provided nor default Calendar)
2) Time.valueOf("24:00:00") uses system-provided calendar, and it does not account user-provided one
appendEra(sbuf, localDate);
return sbuf.toString();
// LocalDateTime is always passed with

This comment has been minimized.

@vlsi

vlsi Jan 8, 2018

Member

ah, "with time zone so backend can decide between timestamp and timestamptz"

@vlsi vlsi merged commit be06946 into pgjdbc:master Jan 8, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@vlsi vlsi deleted the vlsi:timetz branch Jan 8, 2018

throw new PSQLException(
GT.tr("Infinite value found for timestamp/date. This cannot be represented as time."),
PSQLState.DATETIME_OVERFLOW);
ParsedTimestamp ts = parseBackendTimestamp(s);

This comment has been minimized.

@bokken

bokken Jan 8, 2018

Member

this seems to be missing the handling of infinity and -infinity as unsupported by getTime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment