Change TimeValue back to nanosPerDayUTC #11330
Conversation
| { | ||
| packStructHeader( 2, TIME ); | ||
| pack( nanosOfDayLocal ); | ||
| pack( TimeConstants.asValidLocalTime( nanosOfDayUTC ) ); |
There was a problem hiding this comment.
I believe this will change the serialized format to be the UTC local time, correct?
Is that really what we want for the BOLT format? I have a feeling that it might be easier for clients if we sent the localtime within the particular offset.
There was a problem hiding this comment.
Yes, and when I changed it from UTC to local the first time I broke it too. Was trying to revert that brake, and have communicated with the driver team about it.
In Drivers they're not sure what it the best format to get, as temporal support in a the driver languages is very different. I think they need to find out what works best, and then change this to what they really want. In short, they're on top on it.
But to cause the least chaos, I changed this PR to not change the serialization. It's probably easier if they do everything from now, and I keep my hand off :).
|
There is a test failure that looks related. In: |
|
The |
| @SuppressWarnings( "WeakerAccess" ) | ||
| public class TimeConstants | ||
| { | ||
| public static final long NANOS_PER_SECOND = 1_000_000_000L; |
There was a problem hiding this comment.
Why are the ones that do fit in an int still defined as a long, requiring casting elsewhere?
There was a problem hiding this comment.
Leaving NANOS_PER_SECOND as an int gives some interesting overflow problems if you multiply it with another int, e.g. offsetSeconds. I find these problems much harder to detect, than it is annoying to down-cast to int in the 1 place where that is required. Thus this.
| { | ||
| } | ||
|
|
||
| public static long asValidLocalTime( long nanosPerDayUTC ) |
There was a problem hiding this comment.
I find this method confusing. It sounds like it converts UTC to localtime, but it only makes sure to convert a long representing any time from [-18,+42] to [0,24] instead.
Also, the parameter nanosPerDayUTC should maybe just be nanos ?
| @@ -246,7 +247,7 @@ private TimeValue unpackTime() throws IOException | |||
| { | |||
| long nanosOfDayUTC = unpackLong(); | |||
| int offsetSeconds = unpackInteger(); | |||
| return time( nanosOfDayUTC, ZoneOffset.ofTotalSeconds( offsetSeconds ) ); | |||
| return time( nanosOfDayUTC - offsetSeconds * NANOS_PER_SECOND, ZoneOffset.ofTotalSeconds( offsetSeconds ) ); | |||
There was a problem hiding this comment.
Why is conversion needed here?
I think bolt server should both send and receive local time with offset. Thus conversion is probably only needed in #writeTime() because database stores UTC.
Update: please nvm, I misunderstood the code.
| { | ||
| packStructHeader( 2, TIME ); | ||
| pack( nanosOfDayLocal ); | ||
| pack( nanosOfDayUTC + offsetSeconds * NANOS_PER_SECOND ); |
There was a problem hiding this comment.
Would be nice to extract this conversion in a helper method because it's repeated in couple places
| import static java.time.temporal.ChronoUnit.DAYS; | ||
|
|
||
| @SuppressWarnings( "WeakerAccess" ) | ||
| public class TimeConstants |
There was a problem hiding this comment.
This class also contains a method, so Constants suffix feels strange. Maybe TimeUtil?
This is different from the first implementation though, in that it stores nanosPerDayUTC in the range -18h to +42h instead of wrapping around to the next day. This makes the on-disc format directly semantically comparable, which will be useful if we do predicate push-down in the future.
This is different from the first implementation though, in that it stores
nanosPerDayUTC in the range -18h to +42h instead of wrapping around to the
next day. This makes the on-disc format directly semantically comparable,
which will be useful if we do predicate push-down in the future.