fix LocalDate.MAX, special case for postgresql time column. Use 24:00… #992

davecramer · 2017-10-19T20:07:45Z

…:00 as marker for MAX

dpoineau · 2017-10-19T20:16:48Z

pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java


 sbuf.setLength(0);

+ if (localTime == LocalTime.MAX) {


@davecramer
Would it be better to do LocalTime.MAX.equals(localTime) here, instead of using the reference equality?

Otherwise there will be a difference in write behavior between a LocalTime that has a value of 23:59:59.999999999 and when using LocalTime.MAX, even though they are value-equal.

good point, thx

codecov-io · 2017-10-19T21:22:28Z

Codecov Report

Merging #992 into master will increase coverage by 0.01%.
The diff coverage is 77.77%.

@@             Coverage Diff              @@
##             master     #992      +/-   ##
============================================
+ Coverage     65.92%   65.94%   +0.01%     
- Complexity     3560     3568       +8     
============================================
  Files           166      167       +1     
  Lines         15244    15251       +7     
  Branches       2465     2469       +4     
============================================
+ Hits          10050    10057       +7     
- Misses         4023     4025       +2     
+ Partials       1171     1169       -2

vlsi · 2017-10-20T07:31:28Z

pgjdbc/src/test/java/org/postgresql/test/jdbc42/PreparedStatementTest.java

+ LocalTime localTime = (LocalTime)rs.getObject(1,LocalTime.class);
+
+
+ Assert.assertTrue( localTime == LocalTime.MAX);


Please use assertEquals(a, b) (avoid assertTrue(expression))

jorsol · 2017-10-23T14:00:58Z

pgjdbc/src/test/java/org/postgresql/test/jdbc42/PreparedStatementTest.java

@@ -77,20 +77,19 @@ public void testLocalTimeMax() throws SQLException {
 pstmt.executeUpdate();

 ResultSet rs = con.createStatement().executeQuery("select tt from timetable order by id asc");
- Assert.assertTrue(rs.next());
+ Assert.assertEquals(true, rs.next());


The assertTrue here was fine.

jorsol · 2017-10-23T14:01:09Z

pgjdbc/src/test/java/org/postgresql/test/jdbc42/PreparedStatementTest.java


- Assert.assertTrue(rs.next());
+ Assert.assertEquals(true, rs.next());


The assertTrue here was fine.

'infinity'::time does not work, so 00:00:00 and 24:00:00 are used. # select '24:00:01'::time; ERROR: date/time field value out of range: "24:00:01" LINE 1: select '24:00:01'::time; fixes pgjdbc#991 (cherry picked from commit f2d8ec5)

bokken · 2017-12-29T22:26:00Z

What is the need to represent LocalTime.MAX as 24:00:00 rather than how it is documented in java as 23:59:59.999999999?
This special handling of LocalTime seems to be inconsistent with handling of java.sql.Time.

dpoineau · 2017-12-29T22:30:13Z

@bokken My understanding was that this was the unfortunate reason:
#991 (comment)

bokken · 2017-12-29T22:47:40Z

What is the "time" of 24:00:00 in postgresql supposed to represent? Is this to handle leap seconds? Or is this truly supposed to represent the "next" midnight?

There has always been possible differences in precision between java and postgresql. For example, java.sql.Time has millisecond precision. So it has always been possible to try and store 23:59:59.999, which postgresql would truncate to 23:59:59. The fact that there is a constant for LocalTime.MAX that cannot be exactly represented in postgresql does not seem to mean a "sentinel" with different hours, minutes, and seconds should be substituted.

davecramer · 2017-12-30T02:14:17Z

I think it is the other way around. We need to deal with postgresql's MAX time of 24:00:00 Dave Cramer

…

On 29 December 2017 at 17:47, Brett Okken ***@***.***> wrote: What is the "time" of 24:00:00 in postgresql supposed to represent? Is this to handle leap seconds? Or is this truly supposed to represent the "next" midnight? There has always been possible differences in precision between java and postgresql. For example, java.sql.Time has millisecond precision. So it has always been possible to try and store 23:59:59.999, which postgresql would truncate to 23:59:59. The fact that there is a constant for LocalTime.MAX that cannot be exactly represented in postgresql does not seem to mean a "sentinel" with different hours, minutes, and seconds should be substituted. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#992 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYz9hq79DI8lV-mUhkS9vjJOW_f7KoNks5tFWwNgaJpZM4P_yeF> .

bokken · 2017-12-30T13:19:04Z

Do you know what 24:00:00 is supposed to represent?

Assuming it is the "next" midnight, then my proposal is that LocalTime should represent that as 00:00:00 (LocalTime.MIDNIGHT) and java.sql.Time should represent it as 1970-01-02 00:00:00.

This certainly is not perfect, and LocalTime is not symmetrical (i.e. no way to set 24:00:00 and cannot distinguish between 00:00:00 and 24:00:00 on read). However, this seems perhaps the least surprising behavior.

I asked previously about a leap second, but I seem to recall that is usually represented as 23:59:60.

davecramer · 2017-12-30T14:01:03Z

No, I don't know what it is supposed to represent. Do you have a reason for changing the semantics of it ? Dave Cramer

…

On 30 December 2017 at 08:19, Brett Okken ***@***.***> wrote: Do you know what 24:00:00 is supposed to represent? Assuming it is the "next" midnight, then my proposal is that LocalTime should represent that as 00:00:00 (LocalTime.MIDNIGHT <https://docs.oracle.com/javase/8/docs/api/java/time/LocalTime.html#MIDNIGHT>) and java.sql.Time should represent it as 1970-01-02 00:00:00. This certainly is not perfect, and LocalTime is not symmetrical (i.e. no way to set 24:00:00 and cannot distinguish between 00:00:00 and 24:00:00 on read). However, this seems perhaps the least surprising behavior. I asked previously about a leap second, but I seem to recall that is usually represented as 23:59:60. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#992 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYz9rkuI4sEU2bXVFw1o38-0FcPKH33ks5tFjhJgaJpZM4P_yeF> .

davecramer · 2017-12-30T14:07:53Z

It appears looking at the code that it represents the number of seconds in a day Dave Cramer

…

On 30 December 2017 at 09:00, Dave Cramer ***@***.***> wrote: No, I don't know what it is supposed to represent. Do you have a reason for changing the semantics of it ? Dave Cramer On 30 December 2017 at 08:19, Brett Okken ***@***.***> wrote: > Do you know what 24:00:00 is supposed to represent? > > Assuming it is the "next" midnight, then my proposal is that LocalTime > should represent that as 00:00:00 (LocalTime.MIDNIGHT > <https://docs.oracle.com/javase/8/docs/api/java/time/LocalTime.html#MIDNIGHT>) > and java.sql.Time should represent it as 1970-01-02 00:00:00. > > This certainly is not perfect, and LocalTime is not symmetrical (i.e. no > way to set 24:00:00 and cannot distinguish between 00:00:00 and 24:00:00 on > read). However, this seems perhaps the least surprising behavior. > > I asked previously about a leap second, but I seem to recall that is > usually represented as 23:59:60. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#992 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAYz9rkuI4sEU2bXVFw1o38-0FcPKH33ks5tFjhJgaJpZM4P_yeF> > . >

bokken · 2017-12-30T14:56:49Z

I do not (necessarily) want to change the semantics, I want to clarify/understand the semantics.

So it sounds like the time column represents 0-86,400,000,000 microseconds, and LocalTime represents 0-85,399,999,999,999 nano seconds.
That does seem like 24:00:00 is the "next" midnight.
Since there is a mismatch, it needs to be handled on both inserts/updates and reads.

On inserts, I think it would be best to leave LocalTime.MAX as 23:59:99.999999. Currently, on inserts of java.sql.Time, there is also no way to set 24:00:00. That does not seem problematic, and I don't think that introducing support for LocalTime needs to change that.

On reads, if 24:00:00 truly represents the "next" midnight as it seems, then the closest semantic match would seem to be LocalTime.MIDNIGHT. This would line up nicely with how this has been handled with java.sql.Time, where it has been 1970-01-02 00:00:00. The important piece there being that the time elements are midnight. Using the java utilities to go from a java.util.Date to a LocalTime would result in LocalTime.MIDNIGHT.
I can see the argument for massaging 24:00:00 to LocalTime.MAX. Because 24:00:00 is the MAX value for postgres. However, the defined meanings appear to be different (postgres definition is midnight, LocalTime definition is just before midnight) and it then introduces an inconsistency with java.sql.Time. The inconsistency with java.sql.Time could be addressed by massaging 24:00:00 to 23:59:59.999. Indeed I suggested that earlier. After more thought, I am not certain what it actually makes any better and does break with historical behavior.
The problem is that postrgres allows a time column to have 2 midnights, but LocalTime does not. Representing both as LocalTime.MIDNIGHT seems to perhaps be the least surprising behavior of several bad options.

bokken · 2017-12-30T15:37:36Z

I need to correct a mis-statement. I stated that 24:00:00 has historically been handled with java.sql.Time as 1970-01-02 00:00:00. That is not accurate. It has been handled as 1970-01-01 00:00:00. Which is to say, no distinction between 00:00:00 and 24:00:00.
It does parse into 1970-01-02, but the convertToTime method trims it back to 1970-01-01.

bokken · 2017-12-30T15:54:26Z

I've updated the tests to reflect historical behavior and to also include tests of the binary representation.

Given the historical behavior of getTime treating 00:00:00 and 24:00:00 the same, I think it makes most sense for getObject(LocalTime.class) to do the same.

bokken · 2017-12-30T22:04:40Z

I have now also included proposed code changes to address test failures. #1049

I did identify 3 assertions in in tests in TimezoneTest which appeared to be wrong. All 3 were testing time with timezone or timestamp with timezone columns and were asserting that a different value would result if the ResultSet were called with a specific different Calendar. Indeed it asserted that it was the same value when called with a number of different Calendars.
As I read the javadoc for getTime, getTimestamp, etc., the Calendar should only be used if the database does not have timezone information.
So these tests appeared to be asserting that the api was behaving incorrectly in a specific case.

vlsi · 2018-01-08T21:05:23Z

Just an update: this change causes regression (as @bokken noted).
Luckily the PR has not been included into a public release yet.

The fix regarding 00:00:00 is in #1053
24:00:00 and MIN/MAX are likely to require more changes/tests, so I would reopen the issue just in case.

fix LocalDate.MAX, special case for postgresql time column. Use 24:00…

e02db2a

…:00 as marker for MAX

davecramer mentioned this pull request Oct 19, 2017

Possible error with handling of LocalTime.MIN/MAX and "time" column type #991

Closed

dpoineau reviewed Oct 19, 2017

View reviewed changes

use equals for check with LocalTime.MAX

10925e3

vlsi requested changes Oct 20, 2017

View reviewed changes

change assertTrue to assertEquals

f79efc5

jorsol requested changes Oct 23, 2017

View reviewed changes

fix checkstyle and revert assertEquals where appropriate

d098f56

vlsi added this to the 42.1.5 milestone Oct 24, 2017

vlsi merged commit f2d8ec5 into pgjdbc:master Oct 24, 2017

bokken mentioned this pull request Dec 31, 2017

TimeTest unit test appears to only pass in UTC #1048

Open

vlsi modified the milestones: 42.1.5, 42.2.0 Jan 8, 2018

bokken mentioned this pull request Feb 7, 2019

fix: time '24:00:00' should not wrap to '00:00:00' when received in binary mode #1394

Open

davecramer deleted the fixtimeminmax branch December 16, 2019 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix LocalDate.MAX, special case for postgresql time column. Use 24:00… #992

fix LocalDate.MAX, special case for postgresql time column. Use 24:00… #992

davecramer commented Oct 19, 2017

dpoineau Oct 19, 2017

davecramer Oct 19, 2017

codecov-io commented Oct 19, 2017 •

edited

Loading

vlsi Oct 20, 2017

jorsol Oct 23, 2017

jorsol Oct 23, 2017

bokken commented Dec 29, 2017

dpoineau commented Dec 29, 2017

bokken commented Dec 29, 2017

davecramer commented Dec 30, 2017 via email

bokken commented Dec 30, 2017

davecramer commented Dec 30, 2017 via email

davecramer commented Dec 30, 2017 via email

bokken commented Dec 30, 2017

bokken commented Dec 30, 2017

bokken commented Dec 30, 2017

bokken commented Dec 30, 2017

vlsi commented Jan 8, 2018 •

edited

Loading

		LocalTime localTime = (LocalTime)rs.getObject(1,LocalTime.class);


		Assert.assertTrue( localTime == LocalTime.MAX);


		Assert.assertTrue(rs.next());
		Assert.assertEquals(true, rs.next());

fix LocalDate.MAX, special case for postgresql time column. Use 24:00… #992

fix LocalDate.MAX, special case for postgresql time column. Use 24:00… #992

Conversation

davecramer commented Oct 19, 2017

dpoineau Oct 19, 2017

Choose a reason for hiding this comment

davecramer Oct 19, 2017

Choose a reason for hiding this comment

codecov-io commented Oct 19, 2017 • edited Loading

Codecov Report

vlsi Oct 20, 2017

Choose a reason for hiding this comment

jorsol Oct 23, 2017

Choose a reason for hiding this comment

jorsol Oct 23, 2017

Choose a reason for hiding this comment

bokken commented Dec 29, 2017

dpoineau commented Dec 29, 2017

bokken commented Dec 29, 2017

davecramer commented Dec 30, 2017 via email

bokken commented Dec 30, 2017

davecramer commented Dec 30, 2017 via email

davecramer commented Dec 30, 2017 via email

bokken commented Dec 30, 2017

bokken commented Dec 30, 2017

bokken commented Dec 30, 2017

bokken commented Dec 30, 2017

vlsi commented Jan 8, 2018 • edited Loading

codecov-io commented Oct 19, 2017 •

edited

Loading

vlsi commented Jan 8, 2018 •

edited

Loading