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 LocalDate.MAX, special case for postgresql time column. Use 24:00… #992

Merged
merged 4 commits into from Oct 24, 2017

Conversation

Projects
None yet
6 participants
@davecramer
Member

davecramer commented Oct 19, 2017

…:00 as marker for MAX

sbuf.setLength(0);
if (localTime == LocalTime.MAX) {

This comment has been minimized.

@dpoineau

dpoineau Oct 19, 2017

@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.

This comment has been minimized.

@davecramer

davecramer Oct 19, 2017

Member

good point, thx

@codecov-io

This comment has been minimized.

codecov-io commented Oct 19, 2017

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
LocalTime localTime = (LocalTime)rs.getObject(1,LocalTime.class);
Assert.assertTrue( localTime == LocalTime.MAX);

This comment has been minimized.

@vlsi

vlsi Oct 20, 2017

Member

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

@@ -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());

This comment has been minimized.

@jorsol

jorsol Oct 23, 2017

Contributor

The assertTrue here was fine.

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

This comment has been minimized.

@jorsol

jorsol Oct 23, 2017

Contributor

The assertTrue here was fine.

@vlsi vlsi added this to the 42.1.5 milestone Oct 24, 2017

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

2 checks passed

codecov/project 65.94% (+0.01%) compared to f187645
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

dope9967 added a commit to dope9967/pgjdbc that referenced this pull request Dec 28, 2017

fix: use 00:00:00 and 24:00:00 for LocalTime.MIN/MAX (pgjdbc#992)
'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

This comment has been minimized.

Member

bokken commented Dec 29, 2017

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

This comment has been minimized.

dpoineau commented Dec 29, 2017

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

@bokken

This comment has been minimized.

Member

bokken commented Dec 29, 2017

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

This comment has been minimized.

Member

davecramer commented Dec 30, 2017

@bokken

This comment has been minimized.

Member

bokken commented Dec 30, 2017

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

This comment has been minimized.

Member

davecramer commented Dec 30, 2017

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 30, 2017

@bokken

This comment has been minimized.

Member

bokken commented Dec 30, 2017

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

This comment has been minimized.

Member

bokken commented Dec 30, 2017

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

This comment has been minimized.

Member

bokken commented Dec 30, 2017

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

This comment has been minimized.

Member

bokken commented Dec 30, 2017

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

This comment has been minimized.

Member

vlsi commented Jan 8, 2018

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.

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

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: use 00:00:00 and 24:00:00 for LocalTime.MIN/MAX (pgjdbc#992)
'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

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: use 00:00:00 and 24:00:00 for LocalTime.MIN/MAX (pgjdbc#992)
'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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment