From dd2877350e776e9e760255f5f610811645d5a8de Mon Sep 17 00:00:00 2001 From: Henrik Nyman Date: Thu, 1 Mar 2018 21:48:17 +0100 Subject: [PATCH] Add support for spatial point values to the batch importer - Add simple map-parser for PointValue. - Move the logic for creating a PointValue with different default coordinate reference systems depending on the given fields, so that it can be shared with the batch importer without having to instantiate an actual Map object. Instead it can build an array with the given fields directly from the parser. - Let the csv module depend on the values module so it can produce values directly. --- community/csv/pom.xml | 5 + .../java/org/neo4j/csv/reader/Extractors.java | 41 +++ .../commands/expressions/PointFunction.scala | 5 +- .../neo4j/kernel/impl/util/ValueUtils.java | 78 +----- .../internal/BatchInserterImpl.java | 6 +- .../impl/batchimport/EntityImporter.java | 4 +- .../unsafe/impl/batchimport/input/Inputs.java | 4 +- .../input/csv/CsvInputBatchImportIT.java | 30 ++- .../batchimport/input/csv/CsvInputTest.java | 26 ++ .../org/neo4j/values/storable/PointValue.java | 242 ++++++++++++++++++ .../org/neo4j/values/storable/PointTest.java | 87 +++++++ .../neo4j/values/utils/AnyValueTestUtil.java | 20 +- 12 files changed, 462 insertions(+), 86 deletions(-) diff --git a/community/csv/pom.xml b/community/csv/pom.xml index 6d94afc7e7710..730f34772cf58 100644 --- a/community/csv/pom.xml +++ b/community/csv/pom.xml @@ -64,6 +64,11 @@ the relevant Commercial Agreement. neo4j-collections ${project.version} + + org.neo4j + neo4j-values + ${project.version} + diff --git a/community/csv/src/main/java/org/neo4j/csv/reader/Extractors.java b/community/csv/src/main/java/org/neo4j/csv/reader/Extractors.java index 754c1a148e44c..e9e237ec3da09 100644 --- a/community/csv/src/main/java/org/neo4j/csv/reader/Extractors.java +++ b/community/csv/src/main/java/org/neo4j/csv/reader/Extractors.java @@ -20,9 +20,14 @@ package org.neo4j.csv.reader; import java.lang.reflect.Field; +import java.nio.CharBuffer; import java.util.HashMap; import java.util.Map; +import org.neo4j.values.AnyValue; +import org.neo4j.values.storable.PointValue; +import org.neo4j.values.storable.Values; + import static java.lang.Character.isWhitespace; import static java.lang.reflect.Modifier.isStatic; import static org.neo4j.collection.primitive.PrimitiveLongCollections.EMPTY_LONG_ARRAY; @@ -85,6 +90,7 @@ public class Extractors private final Extractor longArray; private final Extractor floatArray; private final Extractor doubleArray; + private final PointExtractor point_; public Extractors( char arrayDelimiter ) { @@ -135,6 +141,7 @@ public Extractors( char arrayDelimiter, boolean emptyStringsAsNull, boolean trim add( longArray = new LongArrayExtractor( arrayDelimiter ) ); add( floatArray = new FloatArrayExtractor( arrayDelimiter ) ); add( doubleArray = new DoubleArrayExtractor( arrayDelimiter ) ); + add( point_ = new PointExtractor() ); } catch ( IllegalAccessException e ) { @@ -242,6 +249,11 @@ public Extractor doubleArray() return doubleArray; } + public PointExtractor point_() + { + return point_; + } + private abstract static class AbstractExtractor implements Extractor { private final String name; @@ -911,6 +923,35 @@ protected void extract0( char[] data, int offset, int length ) } } + public static class PointExtractor extends AbstractSingleValueExtractor + { + private AnyValue value; + + PointExtractor() + { + super( "Point" ); + } + + @Override + protected void clear() + { + value = Values.NO_VALUE; + } + + @Override + protected boolean extract0( char[] data, int offset, int length ) + { + value = PointValue.parse( CharBuffer.wrap( data, offset, length ) ); + return true; + } + + @Override + public AnyValue value() + { + return value; + } + } + private static long extractLong( char[] data, int originalOffset, int fullLength ) { long result = 0; diff --git a/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/expressions/PointFunction.scala b/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/expressions/PointFunction.scala index 62c949eb61865..47de434b4999b 100644 --- a/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/expressions/PointFunction.scala +++ b/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/expressions/PointFunction.scala @@ -22,12 +22,11 @@ package org.neo4j.cypher.internal.runtime.interpreted.commands.expressions import java.util.function.BiConsumer import org.neo4j.cypher.internal.util.v3_4.CypherTypeException -import org.neo4j.kernel.impl.util.ValueUtils import org.neo4j.cypher.internal.runtime.interpreted.ExecutionContext import org.neo4j.cypher.internal.runtime.interpreted.IsMap import org.neo4j.cypher.internal.runtime.interpreted.pipes.QueryState import org.neo4j.values.AnyValue -import org.neo4j.values.storable.Values +import org.neo4j.values.storable.{PointValue, Values} import org.neo4j.values.virtual.MapValue case class PointFunction(data: Expression) extends NullInNullOutExpression(data) { @@ -35,7 +34,7 @@ case class PointFunction(data: Expression) extends NullInNullOutExpression(data) case IsMap(mapCreator) => val map = mapCreator(state.query) if (containsNull(map)) Values.NO_VALUE - else ValueUtils.pointFromMap(map) + else PointValue.fromMap(map) case x => throw new CypherTypeException(s"Expected a map but got $x") } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java index ab2b11f9ad42d..b09a42de5c75c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/util/ValueUtils.java @@ -254,70 +254,6 @@ public static MapValue asMapValue( Map map ) return map( mapValues( map ) ); } - public static PointValue pointFromMap( MapValue map ) - { - CoordinateReferenceSystem crs; - double[] coordinates; - if ( map.containsKey( "crs" ) ) - { - TextValue crsName = (TextValue) map.get( "crs" ); - crs = CoordinateReferenceSystem.byName( crsName.stringValue() ); - if ( crs == null ) - { - throw new IllegalArgumentException( "Unknown coordinate reference system: " + crsName.stringValue() ); - } - } - else - { - crs = null; - } - if ( map.containsKey( "x" ) && map.containsKey( "y" ) ) - { - double x = ((NumberValue) map.get( "x" )).doubleValue(); - double y = ((NumberValue) map.get( "y" )).doubleValue(); - coordinates = map.containsKey( "z" ) ? new double[]{x, y, ((NumberValue) map.get( "z" )).doubleValue()} : new double[]{x, y}; - if ( crs == null ) - { - crs = coordinates.length == 3 ? CoordinateReferenceSystem.Cartesian_3D : CoordinateReferenceSystem.Cartesian; - } - } - else if ( map.containsKey( "latitude" ) && map.containsKey( "longitude" ) ) - { - double x = ((NumberValue) map.get( "longitude" )).doubleValue(); - double y = ((NumberValue) map.get( "latitude" )).doubleValue(); - // TODO Consider supporting key 'height' - if ( map.containsKey( "z" ) ) - { - coordinates = new double[]{x, y, ((NumberValue) map.get( "z" )).doubleValue()}; - } - else if ( map.containsKey( "height" ) ) - { - coordinates = new double[]{x, y, ((NumberValue) map.get( "height" )).doubleValue()}; - } - else - { - coordinates = new double[]{x, y}; - } - if ( crs == null ) - { - crs = coordinates.length == 3 ? CoordinateReferenceSystem.WGS84_3D : CoordinateReferenceSystem.WGS84; - } - if ( !crs.isGeographic() ) - { - throw new IllegalArgumentException( "Geographic points does not support coordinate reference system: " + crs ); - } - } - else - { - throw new IllegalArgumentException( "A point must contain either 'x' and 'y' or 'latitude' and 'longitude'" ); - } - if ( crs.getDimension() != coordinates.length ) - { - throw new IllegalArgumentException( "Cannot create " + crs.getDimension() + "D point with " + coordinates.length + " coordinates" ); - } - return Values.pointValue( crs, coordinates ); - } - private static Map mapValues( Map map ) { HashMap newMap = new HashMap<>( map.size() ); @@ -338,4 +274,18 @@ public static RelationshipValue fromRelationshipProxy( Relationship relationship { return new RelationshipProxyWrappingValue( relationship ); } + + /** + * Creates a {@link Value} from the given object, or if it is already a Value it is returned as it is. + *

+ * This is different from {@link Values#of} which explicitly fails if given a Value. + */ + public static Value asValue( Object value ) + { + if ( value instanceof Value ) + { + return (Value) value; + } + return Values.of( value ); + } } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java index d46a84f48b422..93dabb93c5ccd 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java @@ -148,6 +148,7 @@ import org.neo4j.kernel.impl.transaction.state.RelationshipGroupGetter; import org.neo4j.kernel.impl.transaction.state.storeview.NeoStoreIndexStoreView; import org.neo4j.kernel.impl.util.Dependencies; +import org.neo4j.kernel.impl.util.ValueUtils; import org.neo4j.kernel.internal.EmbeddedGraphDatabase; import org.neo4j.kernel.internal.locker.GlobalStoreLocker; import org.neo4j.kernel.internal.locker.StoreLocker; @@ -161,7 +162,6 @@ import org.neo4j.unsafe.batchinsert.BatchInserter; import org.neo4j.unsafe.batchinsert.BatchRelationship; import org.neo4j.values.storable.Value; -import org.neo4j.values.storable.Values; import static java.lang.Boolean.parseBoolean; import static java.util.Collections.emptyIterator; @@ -408,7 +408,7 @@ private void setPrimitiveProperty( RecordProxy p int propertyKey = getOrCreatePropertyKeyId( propertyName ); RecordAccess propertyRecords = recordAccess.getPropertyRecords(); - propertyCreator.primitiveSetProperty( primitiveRecord, propertyKey, Values.of( propertyValue ), propertyRecords ); + propertyCreator.primitiveSetProperty( primitiveRecord, propertyKey, ValueUtils.asValue( propertyValue ), propertyRecords ); } private void validateIndexCanBeCreated( int labelId, int[] propertyKeyIds ) @@ -773,7 +773,7 @@ private Iterator propertiesIterator( Map properti protected PropertyBlock underlyingObjectToObject( Entry property ) { return propertyCreator.encodePropertyValue( - getOrCreatePropertyKeyId( property.getKey() ), Values.of( property.getValue() ) ); + getOrCreatePropertyKeyId( property.getKey() ), ValueUtils.asValue( property.getValue() ) ); } }; } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/EntityImporter.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/EntityImporter.java index 233d0b337e7d6..63608088dd712 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/EntityImporter.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/EntityImporter.java @@ -29,11 +29,11 @@ import org.neo4j.kernel.impl.store.record.PropertyBlock; import org.neo4j.kernel.impl.store.record.PropertyRecord; import org.neo4j.kernel.impl.store.record.Record; +import org.neo4j.kernel.impl.util.ValueUtils; import org.neo4j.unsafe.impl.batchimport.DataImporter.Monitor; import org.neo4j.unsafe.impl.batchimport.input.InputEntityVisitor; import org.neo4j.unsafe.impl.batchimport.store.BatchingNeoStores; import org.neo4j.unsafe.impl.batchimport.store.BatchingTokenRepository.BatchingPropertyKeyTokenRepository; -import org.neo4j.values.storable.Values; /** * Abstract class containing logic for importing properties for an entity (node/relationship). @@ -119,7 +119,7 @@ private PropertyBlock nextPropertyBlock() private void encodeProperty( PropertyBlock block, int key, Object value ) { - PropertyStore.encodeValue( block, key, Values.of( value ), dynamicStringRecordAllocator, dynamicArrayRecordAllocator, + PropertyStore.encodeValue( block, key, ValueUtils.asValue( value ), dynamicStringRecordAllocator, dynamicArrayRecordAllocator, propertyStore.allowStorePointsAndTemporal() ); } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/input/Inputs.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/input/Inputs.java index 923bda0fd814a..d408f22ae09a4 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/input/Inputs.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/input/Inputs.java @@ -21,12 +21,12 @@ import java.util.function.ToIntFunction; +import org.neo4j.kernel.impl.util.ValueUtils; import org.neo4j.unsafe.impl.batchimport.InputIterable; import org.neo4j.unsafe.impl.batchimport.cache.NumberArrayFactory; import org.neo4j.unsafe.impl.batchimport.cache.idmapping.IdMapper; import org.neo4j.unsafe.impl.batchimport.input.Input.Estimates; import org.neo4j.values.storable.Value; -import org.neo4j.values.storable.Values; public class Inputs { @@ -133,7 +133,7 @@ public static int calculatePropertySize( InputEntity entity, ToIntFunction randomNodeData() InputEntity node = new InputEntity(); node.id( UUID.randomUUID().toString(), Group.GLOBAL ); node.property( "name", "Node " + i ); + node.property( "point", "\" { x : 3, y : " + i + ", crs: WGS-84 } \""); node.labels( randomLabels( random ) ); nodes.add( node ); } @@ -227,14 +230,17 @@ private File nodeDataAsFile( List nodeData ) throws IOException try ( Writer writer = fileSystemRule.get().openAsWriter( file, StandardCharsets.UTF_8, false ) ) { // Header - println( writer, "id:ID,name,some-labels:LABEL" ); + println( writer, "id:ID,name,point:Point,some-labels:LABEL" ); // Data for ( InputEntity node : nodeData ) { String csvLabels = csvLabels( node.labels() ); - println( writer, node.id() + "," + node.properties()[1] + "," + + println( writer, node.id() + "," + node.properties()[1] + "," + node.properties()[3] + "," + (csvLabels != null && csvLabels.length() > 0 ? csvLabels : "") ); + // TODO: Remove me + //System.out.println( node.id() + "," + node.properties()[1] + "," + node.properties()[3] + "," + + // (csvLabels != null && csvLabels.length() > 0 ? csvLabels : "") ); } } return file; @@ -283,12 +289,13 @@ private void verifyImportedData( List nodeData, // Build up expected data for the verification below Map expectedNodes = new HashMap<>(); Map expectedNodeNames = new HashMap<>(); + Map> expectedNodeProperties = new HashMap<>(); Map>> expectedRelationships = new AutoCreatingHashMap<>( nested( String.class, nested( String.class, values( AtomicInteger.class ) ) ) ); Map expectedNodeCounts = new AutoCreatingHashMap<>( values( AtomicLong.class ) ); Map>> expectedRelationshipCounts = new AutoCreatingHashMap<>( nested( String.class, nested( String.class, values( AtomicLong.class ) ) ) ); - buildUpExpectedData( nodeData, relationshipData, expectedNodes, expectedNodeNames, expectedRelationships, + buildUpExpectedData( nodeData, relationshipData, expectedNodes, expectedNodeNames, expectedNodeProperties, expectedRelationships, expectedNodeCounts, expectedRelationshipCounts ); // Do the verification @@ -301,6 +308,13 @@ private void verifyImportedData( List nodeData, String name = (String) node.getProperty( "name" ); String[] labels = expectedNodeNames.remove( name ); assertEquals( asSet( labels ), names( node.getLabels() ) ); + Map expectedProperties = expectedNodeProperties.remove( name ); + Map actualProperties = node.getAllProperties(); + actualProperties.remove( "id" ); + // TODO: Fix the expectations for the points + PointValue actualPoint = (PointValue) actualProperties.remove( "point" ); + String expectedPoint = (String) expectedProperties.remove( "point" ); + assertEquals( expectedProperties, actualProperties ); } assertEquals( 0, expectedNodeNames.size() ); @@ -444,6 +458,7 @@ private void buildUpExpectedData( List relationshipData, Map expectedNodes, Map expectedNodeNames, + Map> expectedNodeProperties, Map>> expectedRelationships, Map nodeCounts, Map>> relationshipCounts ) @@ -452,6 +467,15 @@ private void buildUpExpectedData( { expectedNodes.put( (String) node.id(), node ); expectedNodeNames.put( nameOf( node ), node.labels() ); + + assert node.hasIntPropertyKeyIds == false; + Map properties = new TreeMap<>(); + for ( int i = 0; i < node.propertyCount(); i++ ) + { + properties.put( (String) node.propertyKey( i ), node.propertyValue( i ) ); + } + expectedNodeProperties.put( nameOf( node ), properties ); + countNodeLabels( nodeCounts, node.labels() ); } for ( InputEntity relationship : relationshipData ) diff --git a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputTest.java b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputTest.java index bde13f5c53376..676437b1d483c 100644 --- a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputTest.java +++ b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputTest.java @@ -52,6 +52,8 @@ import org.neo4j.unsafe.impl.batchimport.input.InputEntityDecorators; import org.neo4j.unsafe.impl.batchimport.input.InputEntityVisitor; import org.neo4j.unsafe.impl.batchimport.input.InputException; +import org.neo4j.values.storable.CoordinateReferenceSystem; +import org.neo4j.values.storable.Values; import static java.util.Arrays.asList; import static org.junit.Assert.assertArrayEquals; @@ -456,6 +458,30 @@ public void shouldIgnoreEmptyIntPropertyValues() throws Exception } } + @Test + public void shouldParsePointPropertyValues() throws Exception + { + // GIVEN + DataFactory data = data( + ":ID,name,point:Point\n" + + "0,Mattias,\"{x: 2.7, y:3.2 }\"\n" + + "1,Johan,\" { height :0.01 ,longitude:5, latitude : -4.2 } \"\n" ); + Iterable dataIterable = dataIterable( data ); + Input input = new CsvInput( dataIterable, defaultFormatNodeFileHeader(), datas(), defaultFormatRelationshipFileHeader(), + IdType.ACTUAL, config( COMMAS ), silentBadCollector( 0 ) ); + + // WHEN + try ( InputIterator nodes = input.nodes().iterator() ) + { + // THEN + assertNextNode( nodes, 0L, new Object[]{"name", "Mattias", "point", + Values.pointValue( CoordinateReferenceSystem.Cartesian, 2.7, 3.2) }, labels() ); + assertNextNode( nodes, 1L, new Object[]{"name", "Johan", "point", + Values.pointValue( CoordinateReferenceSystem.WGS84_3D, 5, -4.2, 0.01)}, labels() ); + assertFalse( readNext( nodes ) ); + } + } + @Test public void shouldFailOnArrayDelimiterBeingSameAsDelimiter() { diff --git a/community/values/src/main/java/org/neo4j/values/storable/PointValue.java b/community/values/src/main/java/org/neo4j/values/storable/PointValue.java index a181ae56a0b6d..6c969c0916f02 100644 --- a/community/values/src/main/java/org/neo4j/values/storable/PointValue.java +++ b/community/values/src/main/java/org/neo4j/values/storable/PointValue.java @@ -21,15 +21,21 @@ import java.util.Arrays; import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.neo4j.graphdb.spatial.CRS; import org.neo4j.graphdb.spatial.Coordinate; import org.neo4j.graphdb.spatial.Point; +import org.neo4j.values.AnyValue; import org.neo4j.values.ValueMapper; import org.neo4j.values.utils.PrettyPrinter; +import org.neo4j.values.virtual.MapValue; import static java.lang.String.format; import static java.util.Collections.singletonList; +import static org.neo4j.values.storable.ValueGroup.NUMBER; +import static org.neo4j.values.storable.ValueGroup.TEXT; public class PointValue extends ScalarValue implements Point, Comparable { @@ -255,4 +261,240 @@ public boolean withinRange( PointValue lower, boolean includeLower, PointValue u } return true; } + + public static PointValue fromMap( MapValue map ) + { + AnyValue[] fields = new Value[PointValueField.__MAX_VALUE__.ordinal()]; + for ( PointValueField f : PointValueField.values() ) + { + if ( f != PointValueField.__MAX_VALUE__ ) + { + AnyValue fieldValue = map.get( f.name().toLowerCase() ); + fields[f.ordinal()] = fieldValue != Values.NO_VALUE ? fieldValue : null; + } + } + return fromInputFields( fields ); + } + +// private static Pattern keyValuePattern = Pattern.compile( +// "\\s*+((?x)|(?y)|(?longitude)|(?latitude))\\s*+:\\s*+(?\\S+)|((?crs)\\s*+:\\s*+(?\\S+))" ); +// private static Pattern keyValuePattern = Pattern.compile( "(?[a-z_A-Z]\\w*+)\\s*:\\s*(?\\S+)" ); + private static Pattern mapPattern = Pattern.compile( "\\{(.*)\\}" ); + private static Pattern keyValuePattern = + Pattern.compile( "(?:\\A|,)\\s*+(?[a-z_A-Z]\\w*+)\\s*:\\s*(?[^\\s,]+)" ); + + private static Pattern quotesPattern = Pattern.compile( "^[\"']|[\"']$" ); + + public static PointValue parse( CharSequence text ) + { + Matcher mapMatcher = mapPattern.matcher( text ); + if ( !(mapMatcher.find() && mapMatcher.groupCount() == 1 ) ) + { + String errorMessage = format( "Failed to parse point value: '%s'", text ); + throw new IllegalArgumentException( errorMessage ); + } + + String mapContents = mapMatcher.group( 1 ); + if ( mapContents.isEmpty() ) + { + String errorMessage = format( "Failed to parse point value: '%s'", text ); + throw new IllegalArgumentException( errorMessage ); + } + + Matcher matcher = keyValuePattern.matcher( mapContents ); + if ( !(matcher.find() ) ) + { + String errorMessage = format( "Failed to parse point value: '%s'", text ); + throw new IllegalArgumentException( errorMessage ); + } + + Value[] fields = new Value[PointValueField.__MAX_VALUE__.ordinal()]; + + do + { + String key = matcher.group( "k" ); + if ( key != null ) + { + try + { + // NOTE: We let the key be case-insensitive here + PointValueField field = PointValueField.valueOf( PointValueField.class, key.toUpperCase() ); + + String value = matcher.group( "v" ); + if ( value != null ) + { + switch ( field.valueType() ) + { + case NUMBER: + { + DoubleValue doubleValue = Values.doubleValue( Double.parseDouble( value ) ); + fields[field.ordinal()] = doubleValue; + break; + } + + case TEXT: + { + // Eliminate any quoutes + String unquotedValue = quotesPattern.matcher( value ).replaceAll( "" ); + fields[field.ordinal()] = Values.stringValue( unquotedValue ); + break; + } + + default: + // Just ignore unknown fields + } + } + } + catch ( IllegalArgumentException e ) + { + // Ignore unknown fields + } + } + } while ( matcher.find() ); + + return fromInputFields( fields ); + } + + /** + * This contains the logic to decide the default coordinate reference system based on the input fields + */ + private static PointValue fromInputFields( AnyValue[] fields ) + { + CoordinateReferenceSystem crs; + double[] coordinates; + + AnyValue crsValue = fields[PointValueField.CRS.ordinal()]; + if ( crsValue != null ) + { + TextValue crsName = (TextValue) crsValue; + crs = CoordinateReferenceSystem.byName( crsName.stringValue() ); + if ( crs == null ) + { + throw new IllegalArgumentException( "Unknown coordinate reference system: " + crsName.stringValue() ); + } + } + else + { + crs = null; + } + + AnyValue xValue = fields[PointValueField.X.ordinal()]; + AnyValue yValue = fields[PointValueField.Y.ordinal()]; + AnyValue latitudeValue = fields[PointValueField.LATITUDE.ordinal()]; + AnyValue longitudeValue = fields[PointValueField.LONGITUDE.ordinal()]; + + if ( xValue != null && yValue != null ) + { + double x = ((NumberValue) xValue).doubleValue(); + double y = ((NumberValue) yValue).doubleValue(); + AnyValue zValue = fields[PointValueField.Z.ordinal()]; + + coordinates = zValue != null ? new double[]{x, y, ((NumberValue) zValue).doubleValue()} : new double[]{x, y}; + if ( crs == null ) + { + crs = coordinates.length == 3 ? CoordinateReferenceSystem.Cartesian_3D : CoordinateReferenceSystem.Cartesian; + } + } + else if ( latitudeValue != null && longitudeValue != null ) + { + double x = ((NumberValue) longitudeValue).doubleValue(); + double y = ((NumberValue) latitudeValue).doubleValue(); + AnyValue zValue = fields[PointValueField.Z.ordinal()]; + AnyValue heightValue = fields[PointValueField.HEIGHT.ordinal()]; + if ( zValue != null ) + { + coordinates = new double[]{x, y, ((NumberValue) zValue).doubleValue()}; + } + else if ( heightValue != null ) + { + coordinates = new double[]{x, y, ((NumberValue) heightValue).doubleValue()}; + } + else + { + coordinates = new double[]{x, y}; + } + if ( crs == null ) + { + crs = coordinates.length == 3 ? CoordinateReferenceSystem.WGS84_3D : CoordinateReferenceSystem.WGS84; + } + if ( !crs.isGeographic() ) + { + throw new IllegalArgumentException( "Geographic points does not support coordinate reference system: " + crs ); + } + } + else + { + if ( crs == CoordinateReferenceSystem.Cartesian ) + { + throw new IllegalArgumentException( "A " + CoordinateReferenceSystem.Cartesian.getName() + " point must contain 'x' and 'y'" ); + } + else if ( crs == CoordinateReferenceSystem.Cartesian_3D ) + { + throw new IllegalArgumentException( "A " + CoordinateReferenceSystem.Cartesian_3D.getName() + " point must contain 'x', 'y' and 'z'" ); + } + else if ( crs == CoordinateReferenceSystem.WGS84 ) + { + throw new IllegalArgumentException( "A " + CoordinateReferenceSystem.WGS84.getName() + " point must contain 'latitude' and 'longitude'" ); + } + else if ( crs == CoordinateReferenceSystem.WGS84_3D ) + { + throw new IllegalArgumentException( "A " + CoordinateReferenceSystem.WGS84_3D.getName() + " point must contain 'latitude', 'longitude' and 'height'" ); + } + throw new IllegalArgumentException( "A point must contain either 'x' and 'y' or 'latitude' and 'longitude'" ); + } + + if ( crs.getDimension() != coordinates.length ) + { + throw new IllegalArgumentException( "Cannot create " + crs.getDimension() + "D point with " + coordinates.length + " coordinates" ); + } + return Values.pointValue( crs, coordinates ); + } + + private static enum PointValueField + { + X + { + @Override + ValueGroup valueType() { return NUMBER; } + }, + Y + { + @Override + ValueGroup valueType() { return NUMBER; } + }, + Z + { + @Override + ValueGroup valueType() { return NUMBER; } + }, + LATITUDE + { + @Override + ValueGroup valueType() + { + return NUMBER; + } + }, + LONGITUDE + { + @Override + ValueGroup valueType() + { + return NUMBER; + } + }, + HEIGHT + { + @Override + ValueGroup valueType() { return NUMBER; } + }, + CRS + { + @Override + ValueGroup valueType() { return TEXT; } + }, + __MAX_VALUE__; + + ValueGroup valueType() { return ValueGroup.NO_VALUE; } + } } diff --git a/community/values/src/test/java/org/neo4j/values/storable/PointTest.java b/community/values/src/test/java/org/neo4j/values/storable/PointTest.java index 62131accc493b..7eb497e11df3d 100644 --- a/community/values/src/test/java/org/neo4j/values/storable/PointTest.java +++ b/community/values/src/test/java/org/neo4j/values/storable/PointTest.java @@ -23,7 +23,9 @@ import static org.junit.Assert.assertTrue; import static org.neo4j.values.storable.CoordinateReferenceSystem.Cartesian; +import static org.neo4j.values.storable.CoordinateReferenceSystem.Cartesian_3D; import static org.neo4j.values.storable.CoordinateReferenceSystem.WGS84; +import static org.neo4j.values.storable.CoordinateReferenceSystem.WGS84_3D; import static org.neo4j.values.storable.Values.pointValue; import static org.neo4j.values.utils.AnyValueTestUtil.assertEqual; import static org.neo4j.values.utils.AnyValueTestUtil.assertNotEqual; @@ -74,4 +76,89 @@ public void shouldHaveValueGroup() assertTrue( pointValue( Cartesian, 1, 2 ).valueGroup() != null ); assertTrue( pointValue( WGS84, 1, 2 ).valueGroup() != null ); } + + @Test + public void shouldBeAbleToParsePoints() + { + assertEqual( pointValue( WGS84, 13.2, 56.7 ), + PointValue.parse( "{latitude: 56.7, longitude: 13.2}" ) ); + assertEqual( pointValue( WGS84, -74.0060, 40.7128 ), + PointValue.parse( "{latitude: 40.7128, longitude: -74.0060, crs: 'wgs-84'}" ) ); // - explicitly WGS84 + assertEqual( pointValue( Cartesian, -21, -45.3 ), + PointValue.parse( "{x: -21, y: -45.3}" ) ); // - default to cartesian 2D + assertEqual( pointValue( Cartesian, 17, -52.8 ), + PointValue.parse( "{x: 17, y: -52.8, crs: 'cartesian'}" ) ); // - explicit cartesian 2D + assertEqual( pointValue( WGS84_3D, 13.2, 56.7, 123.4 ), + PointValue.parse( "{latitude: 56.7, longitude: 13.2, height: 123.4}" ) ); // - defaults to WGS84-3D + assertEqual( pointValue( WGS84_3D, 13.2, 56.7, 123.4 ), + PointValue.parse( "{latitude: 56.7, longitude: 13.2, z: 123.4}" ) ); // - defaults to WGS84-3D + assertEqual( pointValue( WGS84_3D, -74.0060, 40.7128, 567.8 ), + PointValue.parse( "{latitude: 40.7128, longitude: -74.0060, height: 567.8, crs: 'wgs-84-3D'}" ) ); // - explicitly WGS84-3D + assertEqual( pointValue( Cartesian_3D, -21, -45.3, 7.2 ), + PointValue.parse( "{x: -21, y: -45.3, z: 7.2}" ) ); // - default to cartesian 3D + assertEqual( pointValue( Cartesian_3D, 17, -52.8, -83.1 ), + PointValue.parse( "{x: 17, y: -52.8, z: -83.1, crs: 'cartesian-3D'}" ) ); // - explicit cartesian 3D + } + + @Test + public void shouldBeAbleToParsePointWithUnquotedCrs() + { + assertEqual( pointValue( WGS84_3D, -74.0060, 40.7128, 567.8 ), + PointValue.parse( "{latitude: 40.7128, longitude: -74.0060, height: 567.8, crs:wgs-84-3D}" ) ); // - explicitly WGS84-3D, without quotes + } + + @Test + public void shouldBeAbleToParseWeirdlyFormattedPoints() + { + assertEqual( pointValue( WGS84, 1.0, 2.0 ), PointValue.parse( " \t\n { latitude : 2.0 ,longitude :1.0 } \t" ) ); + // TODO: Should some/all of these fail? + assertEqual( pointValue( WGS84, 1.0, 2.0 ), PointValue.parse( " \t\n { latitude : 2.0 ,longitude :1.0 , } \t" ) ); + assertEqual( pointValue( Cartesian, 2.0E-8, -1.0E7 ), PointValue.parse( " \t\n { x :+.2e-7,y: -1.0E07 , } \t" ) ); + assertEqual( pointValue( Cartesian, 2.0E-8, -1.0E7 ), PointValue.parse( " \t\n { x :+.2e-7,y: -1.0E07 , garbage} \t" ) ); + assertEqual( pointValue( Cartesian, 2.0E-8, -1.0E7 ), PointValue.parse( " \t\n { gar ba ge,x :+.2e-7,y: -1.0E07} \t" ) ); + } + + @Test + public void shouldBeAbleToParsePointAndIgnoreUnknownFields() + { + assertEqual( pointValue( WGS84, 1.0, 2.0 ), PointValue.parse( "{latitude:2.0,longitude:1.0,unknown_field:\"hello\"}" ) ); + } + + @Test + public void shouldNotBeAbleToParsePointsWithConflictingDuplicateFields() + { + // TODO: Make this fail + assertEqual( pointValue( WGS84, 1.0, 3.0 ), PointValue.parse( "{latitude: 2.0, longitude: 1.0, latitude: 3.0}" ) ); + assertEqual( pointValue( Cartesian, 1.0, 3.0 ), PointValue.parse( "{crs: 'cartesian', x: 2.0, x: 1.0, y: 3}" ) ); + assertEqual( pointValue( Cartesian, 1.0, 3.0 ), PointValue.parse( "{crs: 'invalid crs', x: 1.0, y: 3, crs: 'cartesian'}" ) ); + } + + @Test + public void shouldNotBeAbleToParseIncompletePoints() + { + assertCannotParse( "{latitude: 56.7, longitude:}" ); + assertCannotParse( "{latitude: 56.7}" ); + assertCannotParse( "{}" ); + assertCannotParse( "{only_a_key}" ); + assertCannotParse( "{crs:'WGS-84'}" ); + assertCannotParse( "{a:a}" ); + assertCannotParse( "{ : 2.0, x : 1.0 }" ); + assertCannotParse( "x:1,y:2" ); + assertCannotParse( "{x:1,y:'2'}" ); + assertCannotParse( "{crs:WGS-84 , lat:1, y:2}" ); + } + + private IllegalArgumentException assertCannotParse( String text ) + { + PointValue value; + try + { + value = PointValue.parse( text ); + } + catch ( IllegalArgumentException e ) + { + return e; + } + throw new AssertionError( String.format( "'%s' parsed to %s", text, value ) ); + } } diff --git a/community/values/src/test/java/org/neo4j/values/utils/AnyValueTestUtil.java b/community/values/src/test/java/org/neo4j/values/utils/AnyValueTestUtil.java index 0ab5cedb85396..1427f0535ff39 100644 --- a/community/values/src/test/java/org/neo4j/values/utils/AnyValueTestUtil.java +++ b/community/values/src/test/java/org/neo4j/values/utils/AnyValueTestUtil.java @@ -31,20 +31,22 @@ public class AnyValueTestUtil { public static void assertEqual( AnyValue a, AnyValue b ) { - assertTrue( - String.format( "%s should be equivalent to %s", a.getClass().getSimpleName(), b.getClass().getSimpleName() ), + assertTrue( formatMessage( "should be equivalent to", a, b ), a.equals( b ) ); assertTrue( - String.format( "%s should be equivalent to %s", a.getClass().getSimpleName(), b.getClass().getSimpleName() ), + formatMessage( "should be equivalent to", b, a ), b.equals( a ) ); - assertTrue( - String.format( "%s should be equal %s", a.getClass().getSimpleName(), b.getClass().getSimpleName() ), + assertTrue( formatMessage( "should be equal to", a, b ), a.ternaryEquals( b ) ); - assertTrue( - String.format( "%s should be equal %s", a.getClass().getSimpleName(), b.getClass().getSimpleName() ), + assertTrue( formatMessage( "should be equal to", b, a ), b.ternaryEquals( a ) ); - assertTrue( String.format( "%s should have same hashcode as %s", a.getClass().getSimpleName(), - b.getClass().getSimpleName() ), a.hashCode() == b.hashCode() ); + assertTrue( formatMessage( "should have same hashcode as", a, b ), + a.hashCode() == b.hashCode() ); + } + + private static String formatMessage( String should, AnyValue a, AnyValue b ) + { + return String.format( "%s(%s) %s %s(%s)", a.getClass().getSimpleName(), a.toString(), should, b.getClass().getSimpleName(), b.toString() ); } public static void assertEqualValues( AnyValue a, AnyValue b )