Skip to content

Commit

Permalink
Fix some review comments
Browse files Browse the repository at this point in the history 
- Extract some shared code into AbstractSingleAnyValueExtractor
- Test that the default time zone is used
- Make the Temporal property verifier in the integration test
less implementation specific
- Test more variations of explicit/implicit time zone
  • Loading branch information
@henriknyman
henriknyman committed Mar 7, 2018
1 parent fe61cae commit 67cf690
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 68 deletions.
91 changes: 29 additions & 62 deletions community/csv/src/main/java/org/neo4j/csv/reader/Extractors.java
View file
Expand Up @@ -369,6 +369,28 @@ protected boolean nullValue( int length, boolean hadQuotes )
protected abstract boolean extract0( char[] data, int offset, int length );
}

private abstract static class AbstractSingleAnyValueExtractor extends AbstractSingleValueExtractor<AnyValue>
{
protected AnyValue value;

AbstractSingleAnyValueExtractor( String toString )
{
super( toString );
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
public AnyValue value()
{
return value;
}
}

public static class StringExtractor extends AbstractSingleValueExtractor<String>
{
private String value;
Expand Down Expand Up @@ -979,21 +1001,13 @@ protected void extract0( char[] data, int offset, int length )
}
}

public static class PointExtractor extends AbstractSingleValueExtractor<AnyValue>
public static class PointExtractor extends AbstractSingleAnyValueExtractor
{
private AnyValue value;

PointExtractor()
{
super( "Point" );
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
protected boolean extract0( char[] data, int offset, int length )
{
Expand All @@ -1008,21 +1022,12 @@ public AnyValue value()
}
}

public static class DateExtractor extends AbstractSingleValueExtractor<AnyValue>
public static class DateExtractor extends AbstractSingleAnyValueExtractor
{
private AnyValue value;

DateExtractor()
{
super( "Date" );
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
protected boolean extract0( char[] data, int offset, int length )
{
Expand All @@ -1037,9 +1042,8 @@ public AnyValue value()
}
}

public static class TimeExtractor extends AbstractSingleValueExtractor<AnyValue>
public static class TimeExtractor extends AbstractSingleAnyValueExtractor
{
private AnyValue value;
private Supplier<ZoneId> defaultTimeZone;

TimeExtractor( Supplier<ZoneId> defaultTimeZone )
Expand All @@ -1048,12 +1052,6 @@ public static class TimeExtractor extends AbstractSingleValueExtractor<AnyValue>
this.defaultTimeZone = defaultTimeZone;
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
protected boolean extract0( char[] data, int offset, int length )
{
Expand All @@ -1068,9 +1066,8 @@ public AnyValue value()
}
}

public static class DateTimeExtractor extends AbstractSingleValueExtractor<AnyValue>
public static class DateTimeExtractor extends AbstractSingleAnyValueExtractor
{
private AnyValue value;
private Supplier<ZoneId> defaultTimeZone;

DateTimeExtractor( Supplier<ZoneId> defaultTimeZone )
Expand All @@ -1079,12 +1076,6 @@ public static class DateTimeExtractor extends AbstractSingleValueExtractor<AnyVa
this.defaultTimeZone = defaultTimeZone;
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
protected boolean extract0( char[] data, int offset, int length )
{
Expand All @@ -1099,21 +1090,13 @@ public AnyValue value()
}
}

public static class LocalTimeExtractor extends AbstractSingleValueExtractor<AnyValue>
public static class LocalTimeExtractor extends AbstractSingleAnyValueExtractor
{
private AnyValue value;

LocalTimeExtractor()
{
super( "LocalTime" );
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
protected boolean extract0( char[] data, int offset, int length )
{
Expand All @@ -1128,21 +1111,13 @@ public AnyValue value()
}
}

public static class LocalDateTimeExtractor extends AbstractSingleValueExtractor<AnyValue>
public static class LocalDateTimeExtractor extends AbstractSingleAnyValueExtractor
{
private AnyValue value;

LocalDateTimeExtractor()
{
super( "LocalDateTime" );
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
protected boolean extract0( char[] data, int offset, int length )
{
Expand All @@ -1157,21 +1132,13 @@ public AnyValue value()
}
}

public static class DurationExtractor extends AbstractSingleValueExtractor<AnyValue>
public static class DurationExtractor extends AbstractSingleAnyValueExtractor
{
private AnyValue value;

DurationExtractor()
{
super( "Duration" );
}

@Override
protected void clear()
{
value = Values.NO_VALUE;
}

@Override
protected boolean extract0( char[] data, int offset, int length )
{
Expand Down
48 changes: 44 additions & 4 deletions ...rnel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputBatchImportIT.java
View file
Expand Up @@ -34,7 +34,9 @@
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.time.temporal.Temporal;
import java.time.temporal.TemporalAmount;
import java.time.temporal.TemporalQueries;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
Expand All @@ -49,6 +51,7 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Function;
import java.util.function.Consumer;
import java.util.function.Supplier;

import org.neo4j.graphdb.GraphDatabaseService;
import org.neo4j.graphdb.Label;
Expand All @@ -65,6 +68,7 @@
import org.neo4j.kernel.impl.store.format.RecordFormatSelector;
import org.neo4j.kernel.impl.util.AutoCreatingHashMap;
import org.neo4j.kernel.internal.GraphDatabaseAPI;
import org.neo4j.logging.LogTimeZone;
import org.neo4j.storageengine.api.Token;
import org.neo4j.test.TestGraphDatabaseFactory;
import org.neo4j.test.rule.TestDirectory;
Expand All @@ -87,6 +91,7 @@
import static java.lang.System.currentTimeMillis;
import static java.nio.charset.Charset.defaultCharset;

import static org.neo4j.graphdb.factory.GraphDatabaseSettings.db_timezone;
import static org.neo4j.helpers.collection.Iterators.asSet;
import static org.neo4j.kernel.impl.util.AutoCreatingHashMap.nested;
import static org.neo4j.kernel.impl.util.AutoCreatingHashMap.values;
Expand Down Expand Up @@ -121,11 +126,13 @@ private int indexOf( InputEntity node )
private final long seed = currentTimeMillis();
private final Random random = new Random( seed );

private static final Supplier<ZoneId> testDefaultTimeZone = () -> ZoneId.of( "Asia/Shanghai" );

@Test
public void shouldImportDataComingFromCsvFiles() throws Exception
{
// GIVEN
Config dbConfig = Config.defaults();
Config dbConfig = Config.builder().withSetting( db_timezone, LogTimeZone.SYSTEM.name() ).build();
BatchImporter importer = new ParallelBatchImporter( directory.graphDbDir(), fileSystemRule.get(), null,
smallBatchSizeConfig(), NullLogService.getInstance(), invisible(), AdditionalInitialIds.EMPTY, dbConfig,
RecordFormatSelector.defaultFormat(), NO_MONITOR );
Expand Down Expand Up @@ -155,9 +162,12 @@ public static Input csv( File nodes, File relationships, IdType idType,
org.neo4j.unsafe.impl.batchimport.input.csv.Configuration configuration, Collector badCollector )
{
return new CsvInput(
datas( data( NO_DECORATOR, defaultCharset(), nodes ) ), defaultFormatNodeFileHeader(),
datas( data( NO_DECORATOR, defaultCharset(), nodes ) ),
defaultFormatNodeFileHeader( testDefaultTimeZone ),
datas( data( NO_DECORATOR, defaultCharset(), relationships ) ),
defaultFormatRelationshipFileHeader(), idType, configuration,
defaultFormatRelationshipFileHeader( testDefaultTimeZone ),
idType,
configuration,
badCollector );
}

Expand Down Expand Up @@ -191,6 +201,8 @@ private List<InputEntity> randomNodeData()
node.property( "time", OffsetTime.of( 1, i % 60, 0, 0, ZoneOffset.ofHours( 9 ) ) );
node.property( "dateTime",
ZonedDateTime.of( 2011, 9, 11, 8, i % 60, 0, 0, ZoneId.of( "Europe/Stockholm" ) ) );
node.property( "dateTime2",
LocalDateTime.of( 2011, 9, 11, 8, i % 60, 0, 0 ) ); // No zone specified
node.property( "localTime", LocalTime.of( 1, i % 60, 0 ) );
node.property( "localDateTime", LocalDateTime.of( 2011, 9, 11, 8, i % 60 ) );
node.property( "duration", Period.of( 2, -3, i % 30 ) );
Expand Down Expand Up @@ -251,7 +263,7 @@ private File nodeDataAsFile( List<InputEntity> nodeData ) throws IOException
try ( Writer writer = fileSystemRule.get().openAsWriter( file, StandardCharsets.UTF_8, false ) )
{
// Header
println( writer, "id:ID,name,point:Point,date:Date,time:Time,dateTime:DateTime,localTime:LocalTime," +
println( writer, "id:ID,name,point:Point,date:Date,time:Time,dateTime:DateTime,dateTime2:DateTime,localTime:LocalTime," +
"localDateTime:LocalDateTime,duration:Duration,some-labels:LABEL" );

// Data
Expand Down Expand Up @@ -517,6 +529,34 @@ private void buildUpExpectedData(
assertEquals( expected, actual );
};
}
else if ( expectedValue instanceof Temporal )
{
final LocalDate expectedDate = ((Temporal) expectedValue).query( TemporalQueries.localDate() );
final LocalTime expectedTime = ((Temporal) expectedValue).query( TemporalQueries.localTime() );
final ZoneId expectedZoneId = ((Temporal) expectedValue).query( TemporalQueries.zone() );

verify = actualValue ->
{
LocalDate actualDate = ((Temporal) actualValue).query( TemporalQueries.localDate() );
LocalTime actualTime = ((Temporal) actualValue).query( TemporalQueries.localTime() );
ZoneId actualZoneId = ((Temporal) actualValue).query( TemporalQueries.zone() );

assertEquals( expectedDate, actualDate );
assertEquals( expectedTime, actualTime );
if ( expectedZoneId == null )
{
if ( actualZoneId != null )
{
// If the actual value is zoned it should have the default zone
assertEquals( testDefaultTimeZone.get(), actualZoneId );
}
}
else
{
assertEquals( expectedZoneId, actualZoneId );
}
};
}
else
{
verify = actualValue ->
Expand Down
11 changes: 9 additions & 2 deletions community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputTest.java
View file
Expand Up @@ -520,7 +520,8 @@ public void shouldParseTimePropertyValues() throws Exception
DataFactory data = data(
":ID,name,time:Time\n" +
"0,Mattias,13:37\n" +
"1,Johan,\"16:20:01\"\n" );
"1,Johan,\"16:20:01\"\n" +
"2,Bob,07:30-05:00\n" );
Iterable<DataFactory> dataIterable = dataIterable( data );
Input input = new CsvInput( dataIterable, defaultFormatNodeFileHeader(), datas(),
defaultFormatRelationshipFileHeader(),
Expand All @@ -534,6 +535,8 @@ public void shouldParseTimePropertyValues() throws Exception
TimeValue.time( 13, 37, 0, 0, "+00:00" )}, labels() );
assertNextNode( nodes, 1L, new Object[]{"name", "Johan", "time",
TimeValue.time( 16, 20, 1, 0, "+00:00" )}, labels() );
assertNextNode( nodes, 2L, new Object[]{"name", "Bob", "time",
TimeValue.time( 7, 30, 0, 0, "-05:00" )}, labels() );
assertFalse( readNext( nodes ) );
}
}
Expand All @@ -545,7 +548,9 @@ public void shouldParseDateTimePropertyValues() throws Exception
DataFactory data = data(
":ID,name,time:DateTime\n" +
"0,Mattias,2018-02-27T13:37\n" +
"1,Johan,\"2018-03-01T16:20:01\"\n" );
"1,Johan,\"2018-03-01T16:20:01\"\n" +
"2,Bob,1981-05-11T07:30-05:00\n" );

Iterable<DataFactory> dataIterable = dataIterable( data );
Input input = new CsvInput( dataIterable, defaultFormatNodeFileHeader(), datas(),
defaultFormatRelationshipFileHeader(),
Expand All @@ -559,6 +564,8 @@ public void shouldParseDateTimePropertyValues() throws Exception
DateTimeValue.datetime( 2018, 2, 27, 13, 37, 0, 0, "+00:00" )}, labels() );
assertNextNode( nodes, 1L, new Object[]{"name", "Johan", "time",
DateTimeValue.datetime( 2018, 3, 1, 16, 20, 1, 0, "+00:00" )}, labels() );
assertNextNode( nodes, 2L, new Object[]{"name", "Bob", "time",
DateTimeValue.datetime( 1981, 5, 11, 7, 30, 0, 0, "-05:00" )}, labels() );
assertFalse( readNext( nodes ) );
}
}
Expand Down

0 comments on commit 67cf690

Please sign in to comment.