Skip to content

Commit

Permalink
Preventing storage of arrays of mixed CRS/mixed dimension points.
Browse files Browse the repository at this point in the history 
Added checks in both Kernel and Cypher, and some integration
tests for storing arrays of points.
  • Loading branch information
@sherfert @MishaDemianenko
sherfert authored and MishaDemianenko committed Nov 27, 2017
1 parent 7d39f66 commit a0c7a7d
Show file tree
Hide file tree
Showing 4 changed files with 172 additions and 11 deletions.
9 changes: 8 additions & 1 deletion ...ed-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/CastSupport.scala
View file
Expand Up @@ -91,7 +91,14 @@ object CastSupport {
case (_: FloatValue, _: NumberValue) => a case (_: FloatValue, _: NumberValue) => a
case (_: DoubleValue, _: NumberValue) => a case (_: DoubleValue, _: NumberValue) => a


case (_: PointValue, _: PointValue) => a case (p1: PointValue, p2: PointValue) =>
if (p1.getCoordinateReferenceSystem != p2.getCoordinateReferenceSystem) {
throw new CypherTypeException("Collections containing point values with different CRS can not be stored in properties.");
} else if(p1.coordinate().length != p2.coordinate().length) {
throw new CypherTypeException("Collections containing point values with different dimensions can not be stored in properties.");
} else {
p1
}


case (a, b) if a == Values.NO_VALUE || b == Values.NO_VALUE => throw new CypherTypeException( case (a, b) if a == Values.NO_VALUE || b == Values.NO_VALUE => throw new CypherTypeException(
"Collections containing null values can not be stored in properties.") "Collections containing null values can not be stored in properties.")
Expand Down
25 changes: 19 additions & 6 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/store/GeometryType.java
View file
Expand Up @@ -256,13 +256,26 @@ public static long[] encodePoint( int keyId, CoordinateReferenceSystem crs, doub
public static byte[] encodePointArray( PointValue[] points ) public static byte[] encodePointArray( PointValue[] points )
{ {
int dimension = points[0].coordinate().length; int dimension = points[0].coordinate().length;
CoordinateReferenceSystem crs = points[0].getCoordinateReferenceSystem();
for ( int i = 1; i < points.length; i++ )
{
if ( dimension != points[i].coordinate().length )
{
throw new IllegalArgumentException(
"Attempting to store array of points with inconsistent dimension. Point " + i + " has a different dimension." );
}
if ( !crs.equals( points[i].getCoordinateReferenceSystem() ) )
{
throw new IllegalArgumentException( "Attempting to store array of points with inconsistent CRS. Point " + i + " has a different CRS." );
}
}

double[] data = new double[points.length * dimension]; double[] data = new double[points.length * dimension];
for ( int i = 0; i < data.length; i++ ) for ( int i = 0; i < data.length; i++ )
{ {
data[i] = points[i / dimension].coordinate()[i % dimension]; data[i] = points[i / dimension].coordinate()[i % dimension];
} }
GeometryHeader geometryHeader = new GeometryHeader( GeometryType.GEOMETRY_POINT.gtype, dimension, GeometryHeader geometryHeader = new GeometryHeader( GeometryType.GEOMETRY_POINT.gtype, dimension, crs );
points[0].getCoordinateReferenceSystem() );
byte[] bytes = DynamicArrayStore.encodeFromNumbers( data, DynamicArrayStore.GEOMETRY_HEADER_SIZE ); byte[] bytes = DynamicArrayStore.encodeFromNumbers( data, DynamicArrayStore.GEOMETRY_HEADER_SIZE );
geometryHeader.writeArrayHeaderTo( bytes ); geometryHeader.writeArrayHeaderTo( bytes );
return bytes; return bytes;
Expand Down Expand Up @@ -300,16 +313,16 @@ private void writeArrayHeaderTo( byte[] bytes )
bytes[1] = (byte) geometryType; bytes[1] = (byte) geometryType;
bytes[2] = (byte) dimension; bytes[2] = (byte) dimension;
bytes[3] = (byte) crs.table.getTableId(); bytes[3] = (byte) crs.table.getTableId();
bytes[4] = (byte) (crs.code & 0xFFL); bytes[4] = (byte) (crs.code >> 8 & 0xFFL);
bytes[5] = (byte) (crs.code >> 8 & 0xFFL); bytes[5] = (byte) (crs.code & 0xFFL);
} }


static GeometryHeader fromArrayHeaderBytes( byte[] header ) static GeometryHeader fromArrayHeaderBytes( byte[] header )
{ {
int geometryType = Byte.toUnsignedInt( header[1] ); int geometryType = Byte.toUnsignedInt( header[1] );
int dimension = Byte.toUnsignedInt( header[2] ); int dimension = Byte.toUnsignedInt( header[2] );
int crsTableId = Byte.toUnsignedInt( header[3] ); int crsTableId = Byte.toUnsignedInt( header[3] );
int crsCode = Byte.toUnsignedInt( header[4] ) + (Byte.toUnsignedInt( header[5] ) << 8); int crsCode = Byte.toUnsignedInt( header[5] ) + (Byte.toUnsignedInt( header[4] ) << 8);
return new GeometryHeader( geometryType, dimension, crsTableId, crsCode ); return new GeometryHeader( geometryType, dimension, crsTableId, crsCode );
} }


Expand All @@ -318,7 +331,7 @@ public static GeometryHeader fromArrayHeaderByteBuffer( ByteBuffer buffer )
int geometryType = Byte.toUnsignedInt( buffer.get() ); int geometryType = Byte.toUnsignedInt( buffer.get() );
int dimension = Byte.toUnsignedInt( buffer.get() ); int dimension = Byte.toUnsignedInt( buffer.get() );
int crsTableId = Byte.toUnsignedInt( buffer.get() ); int crsTableId = Byte.toUnsignedInt( buffer.get() );
int crsCode = Byte.toUnsignedInt( buffer.get() ) + (Byte.toUnsignedInt( buffer.get() ) << 8); int crsCode = (Byte.toUnsignedInt( buffer.get() ) << 8) + Byte.toUnsignedInt( buffer.get() );
return new GeometryHeader( geometryType, dimension, crsTableId, crsCode ); return new GeometryHeader( geometryType, dimension, crsTableId, crsCode );
} }
} }
Expand Down
104 changes: 101 additions & 3 deletions community/kernel/src/test/java/org/neo4j/kernel/impl/store/TestArrayStore.java
View file
Expand Up @@ -44,6 +44,9 @@
import org.neo4j.test.rule.PageCacheRule; import org.neo4j.test.rule.PageCacheRule;
import org.neo4j.test.rule.TestDirectory; import org.neo4j.test.rule.TestDirectory;
import org.neo4j.test.rule.fs.DefaultFileSystemRule; import org.neo4j.test.rule.fs.DefaultFileSystemRule;
import org.neo4j.values.storable.CoordinateReferenceSystem;
import org.neo4j.values.storable.PointValue;
import org.neo4j.values.storable.Values;


import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -99,6 +102,15 @@ public void longArrayPropertiesShouldBeBitPacked() throws Exception
PropertyType.LONG, 34 ); PropertyType.LONG, 34 );
} }


@Test
public void doubleArrayPropertiesShouldBeBitPacked() throws Exception
{
assertBitPackedArrayGetsCorrectlySerializedAndDeserialized( new double[]{Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x7L )},
PropertyType.DOUBLE, 3 );
assertBitPackedArrayGetsCorrectlySerializedAndDeserialized( new double[]{Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x8L )},
PropertyType.DOUBLE, 4 );
}

@Test @Test
public void byteArrayPropertiesShouldNotBeBitPacked() throws Exception public void byteArrayPropertiesShouldNotBeBitPacked() throws Exception
{ {
Expand Down Expand Up @@ -128,23 +140,109 @@ public void stringArrayGetsStoredAsUtf8() throws Exception
} }
} }


@Test
public void pointArraysOfWgs84() throws Exception
{
PointValue[] array =
new PointValue[]{Values.pointValue( CoordinateReferenceSystem.WGS84, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x7L ) ),
Values.pointValue( CoordinateReferenceSystem.WGS84, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x1L ) )};
int numberOfBitsUsedForDoubles = 3;

assertPointArrayHasCorrectFormat( array, numberOfBitsUsedForDoubles );
}

@Test
public void pointArraysOfCartesian() throws Exception
{
PointValue[] array =
new PointValue[]{Values.pointValue( CoordinateReferenceSystem.Cartesian, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x7L ) ),
Values.pointValue( CoordinateReferenceSystem.Cartesian, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x1L ) )};
int numberOfBitsUsedForDoubles = 3;

assertPointArrayHasCorrectFormat( array, numberOfBitsUsedForDoubles );
}

@Test( expected = IllegalArgumentException.class )
public void pointArraysOfMixedCRS() throws Exception
{
PointValue[] array =
new PointValue[]{Values.pointValue( CoordinateReferenceSystem.Cartesian, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x7L ) ),
Values.pointValue( CoordinateReferenceSystem.WGS84, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x1L ) )};

Collection<DynamicRecord> records = new ArrayList<>();
arrayStore.allocateRecords( records, array );
}

@Test( expected = IllegalArgumentException.class )
public void pointArraysOfMixedDimension() throws Exception
{
PointValue[] array =
new PointValue[]{Values.pointValue( CoordinateReferenceSystem.Cartesian, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x7L ) ),
Values.pointValue( CoordinateReferenceSystem.Cartesian, Double.longBitsToDouble( 0x1L ), Double.longBitsToDouble( 0x1L ),
Double.longBitsToDouble( 0x4L ) )};

Collection<DynamicRecord> records = new ArrayList<>();
arrayStore.allocateRecords( records, array );
}

private void assertPointArrayHasCorrectFormat( PointValue[] array, int numberOfBitsUsedForDoubles )
{
Collection<DynamicRecord> records = new ArrayList<>();
arrayStore.allocateRecords( records, array );
Pair<byte[],byte[]> loaded = loadArray( records );
assertGeometryHeader( loaded.first(), GeometryType.GEOMETRY_POINT.gtype, 2, array[0].getCoordinateReferenceSystem().table.getTableId(),
array[0].getCoordinateReferenceSystem().code );

final int dimension = array[0].coordinate().length;
double[] pointDoubles = new double[array.length * dimension];
for ( int i = 0; i < pointDoubles.length; i++ )
{
pointDoubles[i] = array[i / dimension].coordinate()[i % dimension];
}

byte[] doubleHeader = Arrays.copyOf( loaded.other(), DynamicArrayStore.NUMBER_HEADER_SIZE );
byte[] doubleBody = Arrays.copyOfRange( loaded.other(), DynamicArrayStore.NUMBER_HEADER_SIZE, loaded.other().length );
assertNumericArrayHeaderAndContent( pointDoubles, PropertyType.DOUBLE, numberOfBitsUsedForDoubles, Pair.of( doubleHeader, doubleBody ) );
}

private void assertStringHeader( byte[] header, int itemCount ) private void assertStringHeader( byte[] header, int itemCount )
{ {
assertEquals( PropertyType.STRING.byteValue(), header[0] ); assertEquals( PropertyType.STRING.byteValue(), header[0] );
assertEquals( itemCount, ByteBuffer.wrap( header, 1, 4 ).getInt() ); assertEquals( itemCount, ByteBuffer.wrap( header, 1, 4 ).getInt() );
} }


private void assertGeometryHeader( byte[] header, int geometryTpe, int dimension, int crsTableId, int crsCode )
{
assertEquals( PropertyType.GEOMETRY.byteValue(), header[0] );
assertEquals( geometryTpe, header[1] );
assertEquals( dimension, header[2] );
assertEquals( crsTableId, header[3] );
assertEquals( crsCode, ByteBuffer.wrap( header, 4, 2 ).getShort() );
}

private void assertBitPackedArrayGetsCorrectlySerializedAndDeserialized( Object array, PropertyType type, private void assertBitPackedArrayGetsCorrectlySerializedAndDeserialized( Object array, PropertyType type,
int expectedBitsUsedPerItem ) int expectedBitsUsedPerItem )
{ {
Collection<DynamicRecord> records = storeArray( array ); Collection<DynamicRecord> records = storeArray( array );
Pair<byte[], byte[]> asBytes = loadArray( records ); Pair<byte[], byte[]> asBytes = loadArray( records );
assertArrayHeader( asBytes.first(), type, expectedBitsUsedPerItem ); assertNumericArrayHeaderAndContent( array, type, expectedBitsUsedPerItem, asBytes );
Bits bits = Bits.bitsFromBytes( asBytes.other() ); }

private void assertNumericArrayHeaderAndContent( Object array, PropertyType type, int expectedBitsUsedPerItem, Pair<byte[],byte[]> loadedBytesFromStore )
{
assertArrayHeader( loadedBytesFromStore.first(), type, expectedBitsUsedPerItem );
Bits bits = Bits.bitsFromBytes( loadedBytesFromStore.other() );
int length = Array.getLength( array ); int length = Array.getLength( array );
for ( int i = 0; i < length; i++ ) for ( int i = 0; i < length; i++ )
{ {
assertEquals( ((Number)Array.get( array, i )).longValue(), bits.getLong( expectedBitsUsedPerItem ) ); if ( array instanceof double[] )
{
assertEquals( Double.doubleToLongBits( Array.getDouble( array, i ) ), bits.getLong( expectedBitsUsedPerItem ) );
}
else
{
assertEquals( Array.getLong( array, i ), bits.getLong( expectedBitsUsedPerItem ) );
}
} }
} }


Expand Down
45 changes: 44 additions & 1 deletion .../src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialFunctionsAcceptanceTest.scala
View file
Expand Up @@ -361,7 +361,7 @@ class SpatialFunctionsAcceptanceTest extends ExecutionEngineFunSuite with Cypher
)))) ))))
} }


test("array of points should be readable from node property") { test("array of cartesian points should be readable from node property") {
// Given // Given
createLabeledNode("Place") createLabeledNode("Place")
graph.execute( graph.execute(
Expand All @@ -386,4 +386,47 @@ class SpatialFunctionsAcceptanceTest extends ExecutionEngineFunSuite with Cypher
CartesianPoint(3.0, 3.0, CRS.Cartesian) CartesianPoint(3.0, 3.0, CRS.Cartesian)
)) ))
} }

test("array of wgs84 points should be readable from node property") {
// Given
createLabeledNode("Place")
graph.execute(
"""
|UNWIND [1,2,3] as num
|WITH point({latitude: num, longitude: num}) as p
|WITH collect(p) as points
|MATCH (place:Place) SET place.location = points
|RETURN place.location as points
""".stripMargin)

// When
val result = executeWith(Configs.All, "MATCH (p:Place) RETURN p.location as points",
planComparisonStrategy = ComparePlansWithAssertion(_ should useOperatorWithText("Projection", "point"),
expectPlansToFail = Configs.AllRulePlanners))

// Then
val points = result.columnAs("points").toList.head.asInstanceOf[Array[_]]
points should equal(Array(
CartesianPoint(1.0, 1.0, CRS.WGS84),
CartesianPoint(2.0, 2.0, CRS.WGS84),
CartesianPoint(3.0, 3.0, CRS.WGS84)
))
}

test("array of mixed points should not be assignable to node property") {
// Given
createLabeledNode("Place")

// When
val config = expectedToSucceed - Configs.SlottedInterpreted - Configs.Cost3_1 - Configs.AllRulePlanners
val query =
"""
|WITH [point({x: 1, y: 2}), point({latitude: 1, longitude: 2})] as points
|MATCH (place:Place) SET place.location = points
|RETURN points
""".stripMargin

// Then
failWithError(config + Configs.Procs, query, Seq("Collections containing point values with different CRS can not be stored in properties."))
}
} }

0 comments on commit a0c7a7d

Please sign in to comment.