Skip to content
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

LocalTime Rounding #1385

Open
kdubb opened this issue Jan 11, 2019 · 11 comments

Comments

Projects
None yet
2 participants
@kdubb
Copy link
Contributor

commented Jan 11, 2019

assertEquals(Time.valueOf("24:00:00"), actual);

Are we sure this test is correct? Time.valueOf(24:00:00) is non-existent in the PostgreSQL world as the value wraps around to zero. A LocalTime (aka a wall clock time) doesn't have this value either as it also wraps around to zero. The test creates a value at what it reports internally as "1970-01-02 00:00:00". In other words it doesn't wrap around to zero but advances to the next day.

Although visually the Time class displays the same value (00:00:00), they are 86400000 different.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Asking because in our implementation of the driver we send and receive binary zero from the server and the test has this ironic failure message:

java.lang.AssertionError: expected: java.sql.Time<00:00:00> but was: java.sql.Time<00:00:00>
@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

This test has the same inequality:

assertEquals(Time.valueOf("24:00:00"), actual);

@vlsi

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

https://rextester.com/l/postgresql_online_compiler

select version() as postgresql_version;

create table pgjdbc1385(x time);

insert into pgjdbc1385(x) values(time '00:00:00'), (time '24:00:00');

select * from pgjdbc1385;

select time '24:00:00' > time '00:00:00';

2019-01-20 23 32 53

I don't know how much sense TIME '24:00:00' makes, however PostgreSQL 9.6.2 consumes it and it flips to the subsequent day.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

This is unfortunate and I'd be interested in their reasoning on accepting this. I was going by the documentation when I raised this issue..

time [ (p) ] [ without time zone ] 8 bytes time of day (no date) 00:00:00 24:00:00 1 microsecond / 14 digits

I assumed that meant less than 24:00:00. I am still under the belief that it's should wrap to zero at 24:00:00 because, as you eluded to, it makes no real sense.

Also, PostgreSQL won't accept anything larger, as this fails...

insert into pgjdbc1385(x) values(time '00:00:00'), (time '24:00:00.000001');

Seems like somebody used a <= when it should be <.

Another point to think about is that java.sql.Time is supposed to be normalized (see #1389) for whatever timezone you believe it to be in. Normalizing the time will only allow up to 23:59:59.99999.

So, I'm still thinking it should be wrapping to zero. What's your thought?

@vlsi

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

Frankly speaking, I'm not quite fond of having 24:00:00, however it seems 24:00:00 is the exact text representation that PostgreSQL is using.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

Yes, PostgreSQL allows it, but java.sql.Time does not. Since this is specifically related to both of them I'd say it shouldn't be there.

@vlsi

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

Rounding 24:00:00 to 00:00:00 does not seem quite right. 23:59:59.999 (or whatever max representable value for the relevant Java type is) should be better

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

I guess there is definitely an argument to be made both ways.

I'm going by the fact that every 24 hour clock I'v seen (on earth lol) wraps to zero. This means 24:00:00 == 00:00:00 is generally a valid statement. While 23:59:59.99999 == 24:00:00 is not.

@vlsi

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@kdubb, you are right LocalTime is unable to represent 24:00:00 (which might be a good idea on its own).
On the other hand, database might happen to have 24:00:00 data.

I think it might be a good idea to throw an exception when 24:00:00 is accessed as LocalTime.
In other words, getString should probably result in 24:00:00, and getObject(1, LocalTime.class) should just fail.

java.sql.Time is a bit moot. Technically speaking, Time.valueOf("24:00:00") seems to work (it produces 86_400_000-like result), however it is not clear if that behavior is specified (or forbidden) somewhere.

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

From the JavaDoc description for java.sql.Time class...

The date components should be set to the "zero epoch" value of January 1, 1970 and should not be accessed.

This relates to #1389 because that test creates and expects all kind of invalid times (and dates).

@kdubb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

86_400_000 == 1970-1-2 00:00:00 and that's what java.sql.Time shows in its internal fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.