Skip to content

Commit

Permalink
Change TimeValue.writeTo and time() to use nanosPerDayLocal
Browse files Browse the repository at this point in the history 
We previously used nanosPerDayUTC, but this seemed less useful
as the value typically is specified in local time. To arrive
at a valid nanosPerDayUTC we might therefore overflow the valid
range as we add the offset to the nanosPerDayLocal, which would
a) require expensive modulus computations and b) mean that the
nanosPerDayUTC would not always compare as needed for TimeValue
comparison semantics. From a valid nanosPerDayLocal we can easily
compute an potentially invalid nanosPerDayUTC using the offset,
that can be used for semantically correct comparison.
  • Loading branch information
@fickludd @Lojjs
fickludd authored and Lojjs committed Mar 16, 2018
1 parent 6e6869d commit b39da24
Show file tree
Hide file tree
Showing 16 changed files with 52 additions and 94 deletions.
2 changes: 1 addition & 1 deletion community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPackV1.java
View file
Expand Up @@ -332,7 +332,7 @@ public void writeLocalTime( long nanoOfDay ) throws IOException
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws IOException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws IOException
{ {
error = new Error( Status.Request.Invalid, error = new Error( Status.Request.Invalid,
"Time is not yet supported as a return type in Bolt" ); "Time is not yet supported as a return type in Bolt" );
Expand Down
4 changes: 2 additions & 2 deletions community/bolt/src/main/java/org/neo4j/bolt/v2/messaging/Neo4jPackV2.java
View file
Expand Up @@ -136,10 +136,10 @@ public void writeLocalTime( long nanoOfDay ) throws IOException
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws IOException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws IOException
{ {
packStructHeader( 2, TIME ); packStructHeader( 2, TIME );
pack( nanosOfDayUTC ); pack( nanosOfDayLocal );
pack( offsetSeconds ); pack( offsetSeconds );
} }


Expand Down
4 changes: 2 additions & 2 deletions ...ity/cypher/cypher/src/main/java/org/neo4j/cypher/internal/codegen/ParameterConverter.java
View file
Expand Up @@ -376,9 +376,9 @@ public void writeLocalTime( long nanoOfDay )
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) public void writeTime( long nanosOfDayLocal, int offsetSeconds )
{ {
writeValue( OffsetTime.of( LocalTime.ofNanoOfDay( nanosOfDayUTC ), ZoneOffset.ofTotalSeconds( offsetSeconds ) ) ); writeValue( OffsetTime.of( LocalTime.ofNanoOfDay( nanosOfDayLocal ), ZoneOffset.ofTotalSeconds( offsetSeconds ) ) );
} }


@Override @Override
Expand Down
23 changes: 19 additions & 4 deletions ...cypher/cypher/src/test/java/org/neo4j/cypher/internal/codegen/ParameterConverterTest.java
View file
Expand Up @@ -271,18 +271,33 @@ public void shouldHandleDate()
} }


@Test @Test
public void shouldHandleTime() public void shouldHandleTimeUTC()
{ {
// Given // Given
TimeValue dvalue = TimeValue.time( 1, 2, 3, 4, "+00:00" ); TimeValue time = TimeValue.time( 1, 2, 3, 4, "+00:00" );


// When // When
dvalue.writeTo( converter ); time.writeTo( converter );


// Then // Then
Object value = converter.value(); Object value = converter.value();
assertThat( value, instanceOf( OffsetTime.class ) ); assertThat( value, instanceOf( OffsetTime.class ) );
assertThat( value, equalTo( dvalue.asObjectCopy() ) ); assertThat( value, equalTo( time.asObjectCopy() ) );
}

@Test
public void shouldHandleTimeWithOffset()
{
// Given
TimeValue time = TimeValue.time( 1, 2, 3, 4, "+01:00" );

// When
time.writeTo( converter );

// Then
Object value = converter.value();
assertThat( value, instanceOf( OffsetTime.class ) );
assertThat( value, equalTo( time.asObjectCopy() ) );
} }


@Test @Test
Expand Down
56 changes: 0 additions & 56 deletions ...d-runtime/src/main/java/org/neo4j/cypher/internal/javacompat/ValueToObjectSerializer.java
View file

This file was deleted.

4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/api/index/ArrayEncoder.java
View file
Expand Up @@ -180,9 +180,9 @@ public void writeLocalTime( long nanoOfDay ) throws RuntimeException
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws RuntimeException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws RuntimeException
{ {
builder.append( TimeValue.time( nanosOfDayUTC, ZoneOffset.ofTotalSeconds( offsetSeconds ) ).prettyPrint() ); builder.append( TimeValue.time( nanosOfDayLocal, ZoneOffset.ofTotalSeconds( offsetSeconds ) ).prettyPrint() );
builder.append( '|' ); builder.append( '|' );
} }


Expand Down
4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ZonedTimeSchemaKey.java
View file
Expand Up @@ -87,9 +87,9 @@ public String toString()
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) public void writeTime( long nanosOfDayLocal, int offsetSeconds )
{ {
this.nanosOfDayUTC = nanosOfDayUTC; this.nanosOfDayUTC = nanosOfDayLocal - offsetSeconds * 1_000_000_000;
this.zoneOffsetSeconds = offsetSeconds; this.zoneOffsetSeconds = offsetSeconds;
} }


Expand Down
4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyStore.java
View file
Expand Up @@ -585,11 +585,11 @@ public void writeLocalTime( long nanoOfDay ) throws IllegalArgumentException
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws IllegalArgumentException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws IllegalArgumentException
{ {
if ( allowStorePointsAndTemporal ) if ( allowStorePointsAndTemporal )
{ {
block.setValueBlocks( TemporalType.encodeTime( keyId, nanosOfDayUTC, offsetSeconds ) ); block.setValueBlocks( TemporalType.encodeTime( keyId, nanosOfDayLocal, offsetSeconds ) );
} }
else else
{ {
Expand Down
4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/store/TemporalType.java
View file
Expand Up @@ -509,7 +509,7 @@ public static long[] encodeDateTime( int keyId, long epochSecond, long nanoOfSec
return data; return data;
} }


public static long[] encodeTime( int keyId, long nanoOfDay, int secondOffset ) public static long[] encodeTime( int keyId, long nanoOfDayLocal, int secondOffset )
{ {
int idBits = StandardFormatSettings.PROPERTY_TOKEN_MAXIMUM_ID_BITS; int idBits = StandardFormatSettings.PROPERTY_TOKEN_MAXIMUM_ID_BITS;
int minuteOffset = secondOffset / 60; int minuteOffset = secondOffset / 60;
Expand All @@ -520,7 +520,7 @@ public static long[] encodeTime( int keyId, long nanoOfDay, int secondOffset )
long[] data = new long[BLOCKS_TIME]; long[] data = new long[BLOCKS_TIME];
// Offset are always in the range +-18:00, so minuteOffset will never require more than 12 bits // Offset are always in the range +-18:00, so minuteOffset will never require more than 12 bits
data[0] = keyAndType | temporalTypeBits | ((long) minuteOffset << 32); data[0] = keyAndType | temporalTypeBits | ((long) minuteOffset << 32);
data[1] = nanoOfDay; data[1] = nanoOfDayLocal;


return data; return data;
} }
Expand Down
4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/util/BaseToObjectValueWriter.java
View file
Expand Up @@ -429,9 +429,9 @@ public void writeLocalTime( long nanoOfDay ) throws RuntimeException
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws RuntimeException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws RuntimeException
{ {
writeValue( OffsetTime.of( LocalTime.ofNanoOfDay( nanosOfDayUTC ), ZoneOffset.ofTotalSeconds( offsetSeconds ) ) ); writeValue( OffsetTime.of( LocalTime.ofNanoOfDay( nanosOfDayLocal ), ZoneOffset.ofTotalSeconds( offsetSeconds ) ) );
} }


@Override @Override
Expand Down
11 changes: 5 additions & 6 deletions community/values/src/main/java/org/neo4j/values/storable/TimeValue.java
View file
Expand Up @@ -72,9 +72,9 @@ public static TimeValue time( int hour, int minute, int second, int nanosOfSecon
OffsetTime.of( LocalTime.of( hour, minute, second, nanosOfSecond ), offset ) ); OffsetTime.of( LocalTime.of( hour, minute, second, nanosOfSecond ), offset ) );
} }


public static TimeValue time( long nanosOfDayUTC, ZoneOffset offset ) public static TimeValue time( long nanosOfDayLocal, ZoneOffset offset )
{ {
return new TimeValue( OffsetTime.ofInstant( Instant.ofEpochSecond( 0, nanosOfDayUTC ), offset ) ); return new TimeValue( OffsetTime.of( LocalTime.ofNanoOfDay( nanosOfDayLocal ), offset ) );
} }


public static TimeValue parse( CharSequence text, Supplier<ZoneId> defaultZone ) public static TimeValue parse( CharSequence text, Supplier<ZoneId> defaultZone )
Expand Down Expand Up @@ -305,11 +305,10 @@ public boolean equals( Value other )
@Override @Override
public <E extends Exception> void writeTo( ValueWriter<E> writer ) throws E public <E extends Exception> void writeTo( ValueWriter<E> writer ) throws E
{ {
int offset = value.getOffset().getTotalSeconds(); int zoneOffsetSeconds = value.getOffset().getTotalSeconds();
long seconds = value.getLong( ChronoField.SECOND_OF_DAY ); long seconds = value.getLong( ChronoField.SECOND_OF_DAY );
seconds = ((-offset % SECONDS_PER_DAY) + seconds + SECONDS_PER_DAY) % SECONDS_PER_DAY; long nanosOfDayLocal = seconds * DurationValue.NANOS_PER_SECOND + value.getNano();
long nano = seconds * DurationValue.NANOS_PER_SECOND + value.getNano(); writer.writeTime( nanosOfDayLocal, zoneOffsetSeconds );
writer.writeTime( nano, offset );
} }


@Override @Override
Expand Down
4 changes: 2 additions & 2 deletions community/values/src/main/java/org/neo4j/values/storable/ValueWriter.java
View file
Expand Up @@ -89,7 +89,7 @@ default void writeUTF8( byte[] bytes, int offset, int length ) throws E


void writeLocalTime( long nanoOfDay ) throws E; void writeLocalTime( long nanoOfDay ) throws E;


void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws E; void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws E;


void writeLocalDateTime( long epochSecond, int nano ) throws E; void writeLocalDateTime( long epochSecond, int nano ) throws E;


Expand Down Expand Up @@ -185,7 +185,7 @@ public void writeLocalTime( long nanoOfDay ) throws E
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws E public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws E
{ // no-op { // no-op
} }


Expand Down
6 changes: 3 additions & 3 deletions community/values/src/main/java/org/neo4j/values/utils/PrettyPrinter.java
View file
Expand Up @@ -195,10 +195,10 @@ public void writeLocalTime( long nanoOfDay ) throws RuntimeException
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws RuntimeException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws RuntimeException
{ {
append( "{time: {nanosOfDay: " ); append( "{time: {nanosOfDayLocal: " );
append( Long.toString( nanosOfDayUTC ) ); append( Long.toString( nanosOfDayLocal ) );
append( ", offsetSeconds: " ); append( ", offsetSeconds: " );
append( Long.toString( offsetSeconds ) ); append( Long.toString( offsetSeconds ) );
append( "}}" ); append( "}}" );
Expand Down
4 changes: 2 additions & 2 deletions community/values/src/test/java/org/neo4j/values/storable/BufferValueWriter.java
View file
Expand Up @@ -196,9 +196,9 @@ public void writeLocalTime( long nanoOfDay ) throws RuntimeException
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws RuntimeException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws RuntimeException
{ {
buffer.add( TimeValue.time( nanosOfDayUTC, ZoneOffset.ofTotalSeconds( offsetSeconds ) ) ); buffer.add( TimeValue.time( nanosOfDayLocal, ZoneOffset.ofTotalSeconds( offsetSeconds ) ) );
} }


@Override @Override
Expand Down
4 changes: 2 additions & 2 deletions community/values/src/test/java/org/neo4j/values/storable/ThrowingValueWriter.java
View file
Expand Up @@ -25,7 +25,7 @@ public abstract class ThrowingValueWriter<E extends Exception> implements ValueW
{ {
protected abstract E exception( String method ); protected abstract E exception( String method );


public static <E extends Exception> ValueWriter<E> throwing( Supplier<E> exception ) static <E extends Exception> ValueWriter<E> throwing( Supplier<E> exception )
{ {
return new ThrowingValueWriter<E>() return new ThrowingValueWriter<E>()
{ {
Expand Down Expand Up @@ -149,7 +149,7 @@ public void writeLocalTime( long nanoOfDay ) throws E
} }


@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws E public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws E
{ {
throw exception( "writeTime" ); throw exception( "writeTime" );
} }
Expand Down
8 changes: 4 additions & 4 deletions community/values/src/test/java/org/neo4j/values/storable/TimeValueTest.java
View file
Expand Up @@ -113,10 +113,10 @@ public void shouldWriteTime()
ValueWriter<RuntimeException> writer = new ThrowingValueWriter.AssertOnly() ValueWriter<RuntimeException> writer = new ThrowingValueWriter.AssertOnly()
{ {
@Override @Override
public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws RuntimeException public void writeTime( long nanosOfDayLocal, int offsetSeconds ) throws RuntimeException
{ {
values.add( time( nanosOfDayUTC, ZoneOffset.ofTotalSeconds( offsetSeconds ) ) ); values.add( time( nanosOfDayLocal, ZoneOffset.ofTotalSeconds( offsetSeconds ) ) );
locals.add( localTime( nanosOfDayUTC ) ); locals.add( localTime( nanosOfDayLocal ) );
} }
}; };


Expand All @@ -125,7 +125,7 @@ public void writeTime( long nanosOfDayUTC, int offsetSeconds ) throws RuntimeExc


// then // then
assertEquals( singletonList( time ), values ); assertEquals( singletonList( time ), values );
assertEquals( singletonList( inUTC( time ) ), locals ); assertEquals( singletonList( localTime( time.getLocalTimePart() ) ), locals );
} }
} }


Expand Down

0 comments on commit b39da24

Please sign in to comment.