Обсуждение: Make time/timestamp tests fair for binary transfers

Hi,

The main purpose of the following patch is to make TimezoneTest fair
for binary transfers. The modified test cases now work both on text and
binary protocol (I'll post my new version of it soonish after I have
resolved all remaining unit test case failures).

The patch also alters what kind of java.sql.Time values are returned.
Before the patch the millisecond value of a ResultSet.getTime could be
between -24h<x<24h. The patch changes the returned value to be always
between 0h<=x<24h. If someone thinks this change is not acceptable I'll
go back and revert it re-fix the test cases to match.

The TimeTest was only altered because of the Time millisecond value
adjustments.

The TimezoneTest has a larger alteration. Previously the code did not
mostly test the parsing of time/date/timestamp values but instead
compared the values returned by ResultSet.getString, which are
formatted by the server.
The patched version does now two checks.
1) adds a new verify round that ensures that the insert was correct by
doing an explicit query with UTC timezone and casting all values to
varchar so that the server will format them.
2) Changes all other tests to use the getTime/getDate/getTimestamp
methods when comparing if the queried data is correct.

This basically means that the getString of a time type column is now
undefined and can change. But I think it's not a problem because
different server versions have had different string representation of
time already.

Mikko Tiihonen wrote:

> The patch also alters what kind of java.sql.Time values are returned.
> Before the patch the millisecond value of a ResultSet.getTime could be
> between -24h<x<24h. The patch changes the returned value to be always
> between 0h<=x<24h. If someone thinks this change is not acceptable I'll
> go back and revert it re-fix the test cases to match.

I'm not sure if this is right. In theory Time values are meant to have a
day/month/year of Jan 1 1970. Time (via Date) uses the local timezone to
compute the day/month/year. But to represent all time values in all
local timezones that means you need negative millisecond values (to
handle timezones ahead of UTC). For example, Jan 1 1970 02:00 +1200 is
actually Dec 31 1969 14:00 UTC, which is a negative milliseconds value.

There's an additional complication that if you're dealing with time
values in different timezones, it'd imply that 2am +1200 is identical to
2pm UTC, which is not entirely true.. they're on different days.
Currently they'd return Time values that are exactly one day apart, with
this change they'd return identical Time values.

As usual, the javadoc is fairly hopeless at telling you how Time is
actually supposed to behave!

(I wonder what java.sql.Time.valueOf() returns in timezones ahead of
UTC? Might be worth testing..)

-O

Oliver Jowett wrote:

> (I wonder what java.sql.Time.valueOf() returns in timezones ahead of
> UTC? Might be worth testing..)

Time.valueOf("00:00:00").getTime() returns -43200000 in a +1200
timezone, so I'd be happier keeping negative values to be consistent
with that.

-O

On Tue, 2007-07-24 at 10:30 +1200, Oliver Jowett wrote:
> Mikko Tiihonen wrote:
>
> > The patch also alters what kind of java.sql.Time values are returned.
> > Before the patch the millisecond value of a ResultSet.getTime could be
> > between -24h<x<24h. The patch changes the returned value to be always
> > between 0h<=x<24h. If someone thinks this change is not acceptable I'll
> > go back and revert it re-fix the test cases to match.
>
> I'm not sure if this is right. In theory Time values are meant to have a
> day/month/year of Jan 1 1970. Time (via Date) uses the local timezone to
> compute the day/month/year. But to represent all time values in all
> local timezones that means you need negative millisecond values (to
> handle timezones ahead of UTC). For example, Jan 1 1970 02:00 +1200 is
> actually Dec 31 1969 14:00 UTC, which is a negative milliseconds value.

Yes. But the Time part is 14:00, and if you store the above to postgres
column with TIME type it will not print out nor return any negative
values.
The negative time is purely a Java problem.

> There's an additional complication that if you're dealing with time
> values in different timezones, it'd imply that 2am +1200 is identical to
> 2pm UTC, which is not entirely true.. they're on different days.
> Currently they'd return Time values that are exactly one day apart, with
> this change they'd return identical Time values.

Yes, but one would want to know both the day and the time of an event
then
a timestamp would have been a better choice.

> As usual, the javadoc is fairly hopeless at telling you how Time is
> actually supposed to behave!
> (I wonder what java.sql.Time.valueOf() returns in timezones ahead of
> UTC? Might be worth testing..)
... next post
> Time.valueOf("00:00:00").getTime() returns -43200000 in a +1200
> timezone, so I'd be happier keeping negative values to be consistent
> with that.

OK, lets keep using old way. It is most likely that the current
functionality is the one to which application writers have pushed all
JDBC driver implementations to converge to thus creating an undocumented
standard. Although everywhere I have seen the design guidelines for
portable JDBC code, the guideline is to store times to database either
in UTC milliseconds or as fully formatted strings with time zone. Thus I
believe that most applications have not exercised the corner cases
caused by varying time zones.

I'll try again if I can get that to work with the binary transfers and
resend the patch.

-Mikko

On Tue, 2007-07-24 at 10:55 +1200, Oliver Jowett wrote:
> Oliver Jowett wrote:
>
> > (I wonder what java.sql.Time.valueOf() returns in timezones ahead of
> > UTC? Might be worth testing..)
>
> Time.valueOf("00:00:00").getTime() returns -43200000 in a +1200
> timezone, so I'd be happier keeping negative values to be consistent
> with that.

OK. I managed to get everything working with binary transfer and the
time
zone dependant normalisation. Just to clarify: the values returned by
ResultSet.getTime are normalised as follows:
GMT-13:00 (+46800000..+133200000)
GMT+00:00 (+00000000..+086400000)
GMT+13:00 (-46800000..+039600000)

The original code already did it this way. Maybe it should be
documented somewhere explicitly so that it is a known decision
instead of just an implementation detail.

Patch description:
The main purpose of the following patch is to make TimezoneTest fair
for binary transfers. The modified test cases now work both on text and
binary protocol (I'll post my new version of it soonish after I have
resolved all remaining unit test case failures).

Previously the code mostly did not test the parsing of
time/date/timestamp values but instead compared the values returned by
ResultSet.getString, which are formatted by the server.
The patched version does now two checks:
1) adds a new verify round that ensures that the insert was correct by
doing an explicit query with UTC timezone and casting all values to
varchar so that the server will format them.
2) Changes all other tests to use the getTime/getDate/getTimestamp
methods when comparing if the queried data is correct.

This basically means that the getString of a time type column is now
undefined and can change. But I think it's not a problem because
different server versions have had different string representation of
time already.

-Mikko