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.
neo4j · Mar 6, 2018 · dd28773 · dd28773
1 parent 3f4611b
commit dd28773
Show file tree

Hide file tree

Showing 12 changed files with 462 additions and 86 deletions.
diff --git a/community/csv/pom.xml b/community/csv/pom.xml
@@ -64,6 +64,11 @@ the relevant Commercial Agreement.
       <artifactId>neo4j-collections</artifactId>
       <version>${project.version}</version>
     </dependency>
+    <dependency>
+      <groupId>org.neo4j</groupId>
+      <artifactId>neo4j-values</artifactId>
+      <version>${project.version}</version>
+    </dependency>
 
     <!-- test dependencies -->
     <dependency>

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
@@ -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<long[]> longArray;
     private final Extractor<float[]> floatArray;
     private final Extractor<double[]> 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<double[]> doubleArray()
         return doubleArray;
     }
 
+    public PointExtractor point_()
+    {
+        return point_;
+    }
+
     private abstract static class AbstractExtractor<T> implements Extractor<T>
     {
         private final String name;
@@ -911,6 +923,35 @@ protected void extract0( char[] data, int offset, int length )
         }
     }
 
+    public static class PointExtractor extends AbstractSingleValueExtractor<AnyValue>
+    {
+        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/...la/org/neo4j/cypher/internal/runtime/interpreted/commands/expressions/PointFunction.scala b/...la/org/neo4j/cypher/internal/runtime/interpreted/commands/expressions/PointFunction.scala
@@ -22,20 +22,19 @@ 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) {
   override def compute(value: AnyValue, ctx: ExecutionContext, state: QueryState): AnyValue = value match {
     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
@@ -254,70 +254,6 @@ public static MapValue asMapValue( Map<String,Object> 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<String,AnyValue> mapValues( Map<String,Object> map )
     {
         HashMap<String,AnyValue> 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.
+     * <p>
+     * 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
@@ -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<? extends PrimitiveRecord,Void> p
         int propertyKey = getOrCreatePropertyKeyId( propertyName );
         RecordAccess<PropertyRecord,PrimitiveRecord> 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<PropertyBlock> propertiesIterator( Map<String, Object> properti
             protected PropertyBlock underlyingObjectToObject( Entry<String, Object> 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
@@ -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
@@ -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<Value
             Value[] values = new Value[propertyCount];
             for ( int i = 0; i < propertyCount; i++ )
             {
-                values[i] = Values.of( entity.propertyValue( i ) );
+                values[i] = ValueUtils.asValue( entity.propertyValue( i ) );
             }
             size += valueSizeCalculator.applyAsInt( values );
         }

diff --git a/...rnel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputBatchImportIT.java b/...rnel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputBatchImportIT.java
@@ -34,6 +34,7 @@
 import java.util.Map;
 import java.util.Random;
 import java.util.Set;
+import java.util.TreeMap;
 import java.util.UUID;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
@@ -66,6 +67,7 @@
 import org.neo4j.unsafe.impl.batchimport.input.Collector;
 import org.neo4j.unsafe.impl.batchimport.input.Group;
 import org.neo4j.unsafe.impl.batchimport.input.Input;
+import org.neo4j.values.storable.PointValue;
 
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.junit.Assert.assertEquals;
@@ -170,6 +172,7 @@ private List<InputEntity> 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<InputEntity> 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<InputEntity> nodeData,
         // Build up expected data for the verification below
         Map<String/*id*/, InputEntity> expectedNodes = new HashMap<>();
         Map<String,String[]> expectedNodeNames = new HashMap<>();
+        Map<String, Map<String,Object>> expectedNodeProperties = new HashMap<>();
         Map<String/*start node name*/, Map<String/*end node name*/, Map<String, AtomicInteger>>> expectedRelationships =
                 new AutoCreatingHashMap<>( nested( String.class, nested( String.class, values( AtomicInteger.class ) ) ) );
         Map<String, AtomicLong> expectedNodeCounts = new AutoCreatingHashMap<>( values( AtomicLong.class ) );
         Map<String, Map<String, Map<String, AtomicLong>>> 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<InputEntity> nodeData,
                 String name = (String) node.getProperty( "name" );
                 String[] labels = expectedNodeNames.remove( name );
                 assertEquals( asSet( labels ), names( node.getLabels() ) );
+                Map<String,Object> expectedProperties = expectedNodeProperties.remove( name );
+                Map<String,Object> 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<InputEntity> relationshipData,
             Map<String, InputEntity> expectedNodes,
             Map<String, String[]> expectedNodeNames,
+            Map<String, Map<String,Object>> expectedNodeProperties,
             Map<String, Map<String, Map<String, AtomicInteger>>> expectedRelationships,
             Map<String, AtomicLong> nodeCounts,
             Map<String, Map<String, Map<String, AtomicLong>>> 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<String,Object> 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 )