From 99bc3ac9e3b51247c68d1ed03761f21b20660693 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Thu, 4 Jan 2018 13:14:02 +0100 Subject: [PATCH] Cleaned up geometry range queries to use correct CRS --- .../neo4j/internal/kernel/api/IndexQuery.java | 43 +++++++++---------- .../impl/index/schema/SpatialKnownIndex.java | 8 ++-- .../impl/index/schema/SpatialLayout.java | 11 ++++- .../index/schema/SpatialLayoutNonUnique.java | 6 +++ .../index/schema/SpatialLayoutUnique.java | 6 +++ .../schema/SpatialSchemaIndexReader.java | 18 +++++--- .../impl/index/schema/SpatialSchemaKey.java | 12 +++++- .../fusion/SpatialFusionIndexReader.java | 2 +- .../SpatialFunctionsAcceptanceTest.scala | 38 ++++++++++++++++ .../SpatialIndexAcceptanceTest.scala | 31 ++++++++++--- 10 files changed, 131 insertions(+), 44 deletions(-) diff --git a/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/IndexQuery.java b/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/IndexQuery.java index 0c39396947ba..a647f23ba785 100644 --- a/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/IndexQuery.java +++ b/community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/IndexQuery.java @@ -24,6 +24,7 @@ import org.apache.commons.lang3.builder.ToStringBuilder; import org.apache.commons.lang3.builder.ToStringStyle; +import org.neo4j.values.storable.CoordinateReferenceSystem; import org.neo4j.values.storable.PointValue; import org.neo4j.values.storable.TextValue; import org.neo4j.values.storable.Value; @@ -31,8 +32,6 @@ import org.neo4j.values.storable.ValueTuple; import org.neo4j.values.storable.Values; -import static org.neo4j.values.storable.Values.NO_VALUE; - public abstract class IndexQuery { /** @@ -228,7 +227,7 @@ public IndexQueryType type() @Override public boolean acceptsValue( Value value ) { - return value != null && value != NO_VALUE; + return value != null && value != Values.NO_VALUE; } @Override @@ -309,7 +308,7 @@ public boolean acceptsValue( Value value ) } if ( Values.isNumberValue( value ) ) { - if ( from != NO_VALUE ) + if ( from != Values.NO_VALUE ) { int compare = Values.COMPARATOR.compare( value, from ); if ( compare < 0 || !fromInclusive && compare == 0 ) @@ -317,7 +316,7 @@ public boolean acceptsValue( Value value ) return false; } } - if ( to != NO_VALUE ) + if ( to != Values.NO_VALUE ) { int compare = Values.COMPARATOR.compare( value, to ); if ( compare > 0 || !toInclusive && compare == 0 ) @@ -369,18 +368,24 @@ public boolean toInclusive() public static final class GeometryRangePredicate extends IndexQuery { - private final Value from; + private final PointValue from; private final boolean fromInclusive; - private final Value to; + private final PointValue to; private final boolean toInclusive; + private final CoordinateReferenceSystem crs; GeometryRangePredicate( int propertyKeyId, PointValue from, boolean fromInclusive, PointValue to, boolean toInclusive ) { super( propertyKeyId ); - this.from = from != null ? from : NO_VALUE; + if ( from == null && to == null ) + { + throw new IllegalArgumentException( "Cannot create GeometryRangePredicate without at least one bound" ); + } + this.from = from; this.fromInclusive = fromInclusive; - this.to = to != null ? to : NO_VALUE; + this.to = to; this.toInclusive = toInclusive; + this.crs = from != null ? from.getCoordinateReferenceSystem() : to.getCoordinateReferenceSystem(); } @Override @@ -400,8 +405,7 @@ public boolean acceptsValue( Value value ) if ( value instanceof PointValue ) { PointValue point = (PointValue) value; - //TODO Use Hilbert Space Filling Curves for comparison - return point.withinRange( from(), fromInclusive, to(), toInclusive ); + return point.withinRange( from, fromInclusive, to, toInclusive ); } return false; } @@ -414,22 +418,17 @@ public ValueGroup valueGroup() public PointValue from() { - return (PointValue) from.asObject(); + return from; } public PointValue to() { - return (PointValue) to.asObject(); - } - - public Value fromAsValue() - { - return from; + return to; } - public Value toAsValue() + public CoordinateReferenceSystem crs() { - return to; + return crs; } public boolean fromInclusive() @@ -490,7 +489,7 @@ public boolean acceptsValue( Value value ) { return false; } - if ( from != NO_VALUE ) + if ( from != Values.NO_VALUE ) { int compare = Values.COMPARATOR.compare( value, from ); if ( compare < 0 || !fromInclusive && compare == 0 ) @@ -498,7 +497,7 @@ public boolean acceptsValue( Value value ) return false; } } - if ( to != NO_VALUE ) + if ( to != Values.NO_VALUE ) { int compare = Values.COMPARATOR.compare( value, to ); if ( compare > 0 || !toInclusive && compare == 0 ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialKnownIndex.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialKnownIndex.java index e7f19f1938b0..e9732e19c004 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialKnownIndex.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialKnownIndex.java @@ -125,10 +125,10 @@ private NativeSchemaIndexPopulator makeIndexPopulator( IndexDescriptor descripto switch ( descriptor.type() ) { case GENERAL: - return new NativeNonUniqueSchemaIndexPopulator<>( pageCache, fs, indexFile, new SpatialLayoutNonUnique(), samplingConfig, monitor, descriptor, + return new NativeNonUniqueSchemaIndexPopulator<>( pageCache, fs, indexFile, new SpatialLayoutNonUnique(crs), samplingConfig, monitor, descriptor, indexId ); case UNIQUE: - return new NativeUniqueSchemaIndexPopulator<>( pageCache, fs, indexFile, new SpatialLayoutUnique(), monitor, descriptor, indexId ); + return new NativeUniqueSchemaIndexPopulator<>( pageCache, fs, indexFile, new SpatialLayoutUnique(crs), monitor, descriptor, indexId ); default: throw new UnsupportedOperationException( "Can not create index populator of type " + descriptor.type() ); } @@ -165,10 +165,10 @@ private SpatialSchemaIndexAccessor makeOnlineAccessor( IndexDescriptor descripto switch ( descriptor.type() ) { case GENERAL: - layout = new SpatialLayoutNonUnique(); + layout = new SpatialLayoutNonUnique(crs); break; case UNIQUE: - layout = new SpatialLayoutUnique(); + layout = new SpatialLayoutUnique(crs); break; default: throw new UnsupportedOperationException( "Can not create index accessor of type " + descriptor.type() ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayout.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayout.java index 40ae00867dfe..d824cd0ae048 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayout.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayout.java @@ -21,16 +21,24 @@ import org.neo4j.index.internal.gbptree.Layout; import org.neo4j.io.pagecache.PageCursor; +import org.neo4j.values.storable.CoordinateReferenceSystem; /** * {@link Layout} for numbers where numbers doesn't need to be unique. */ public abstract class SpatialLayout extends Layout.Adapter { + private CoordinateReferenceSystem crs; + + public SpatialLayout( CoordinateReferenceSystem crs ) + { + this.crs = crs; + } + @Override public SpatialSchemaKey newKey() { - return new SpatialSchemaKey(); + return new SpatialSchemaKey(crs); } @Override @@ -41,6 +49,7 @@ public SpatialSchemaKey copyKey( SpatialSchemaKey key, into.rawValueBits = key.rawValueBits; into.setEntityId( key.getEntityId() ); into.setEntityIdIsSpecialTieBreaker( key.getEntityIdIsSpecialTieBreaker() ); + into.crs = key.crs; return into; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutNonUnique.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutNonUnique.java index 5a840cbe3331..1303a1c1c052 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutNonUnique.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutNonUnique.java @@ -20,6 +20,7 @@ package org.neo4j.kernel.impl.index.schema; import org.neo4j.index.internal.gbptree.Layout; +import org.neo4j.values.storable.CoordinateReferenceSystem; public class SpatialLayoutNonUnique extends SpatialLayout { @@ -28,6 +29,11 @@ public class SpatialLayoutNonUnique extends SpatialLayout public static final int MINOR_VERSION = 1; public static long IDENTIFIER = Layout.namedIdentifier( IDENTIFIER_NAME, NativeSchemaValue.SIZE ); + public SpatialLayoutNonUnique( CoordinateReferenceSystem crs ) + { + super(crs); + } + @Override public long identifier() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutUnique.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutUnique.java index 6f6981e1ee0f..c0e99e918608 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutUnique.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialLayoutUnique.java @@ -20,6 +20,7 @@ package org.neo4j.kernel.impl.index.schema; import org.neo4j.index.internal.gbptree.Layout; +import org.neo4j.values.storable.CoordinateReferenceSystem; /** * {@link Layout} for numbers where numbers need to be unique. @@ -31,6 +32,11 @@ public class SpatialLayoutUnique extends SpatialLayout public static final int MINOR_VERSION = 1; public static long IDENTIFIER = Layout.namedIdentifier( IDENTIFIER_NAME, NativeSchemaValue.SIZE ); + public SpatialLayoutUnique( CoordinateReferenceSystem crs ) + { + super(crs); + } + @Override public long identifier() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaIndexReader.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaIndexReader.java index de0a592a5a24..0c2b4d7be0c1 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaIndexReader.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaIndexReader.java @@ -31,12 +31,11 @@ import org.neo4j.kernel.api.schema.index.IndexDescriptor; import org.neo4j.kernel.impl.api.index.sampling.IndexSamplingConfig; import org.neo4j.storageengine.api.schema.IndexProgressor; -import org.neo4j.values.storable.Value; -import org.neo4j.values.storable.ValueGroup; +import org.neo4j.values.storable.PointValue; import static java.lang.String.format; -public class SpatialSchemaIndexReader extends NativeSchemaIndexReader +public class SpatialSchemaIndexReader extends NativeSchemaIndexReader { SpatialSchemaIndexReader( GBPTree tree, Layout layout, IndexSamplingConfig samplingConfig, IndexDescriptor descriptor ) { @@ -76,6 +75,11 @@ void initializeRangeForQuery( KEY treeKeyFrom, KEY treeKeyTo, IndexQuery[] predi break; case rangeGeometric: GeometryRangePredicate rangePredicate = (GeometryRangePredicate) predicate; + if ( !rangePredicate.crs().equals( treeKeyTo.crs ) ) + { + throw new IllegalArgumentException( + "IndexQuery on spatial index with mismatching CoordinateReferenceSystem: " + rangePredicate.crs() + " != " + treeKeyTo.crs ); + } initFromForRange( rangePredicate, treeKeyFrom ); initToForRange( rangePredicate, treeKeyTo ); break; @@ -86,8 +90,8 @@ void initializeRangeForQuery( KEY treeKeyFrom, KEY treeKeyTo, IndexQuery[] predi private void initToForRange( GeometryRangePredicate rangePredicate, KEY treeKeyTo ) { - Value toValue = rangePredicate.toAsValue(); - if ( toValue.valueGroup() == ValueGroup.NO_VALUE ) + PointValue toValue = rangePredicate.to(); + if ( toValue == null ) { treeKeyTo.initAsHighest(); } @@ -100,8 +104,8 @@ private void initToForRange( GeometryRangePredicate rangePredicate, KEY treeKeyT private void initFromForRange( GeometryRangePredicate rangePredicate, KEY treeKeyFrom ) { - Value fromValue = rangePredicate.fromAsValue(); - if ( fromValue.valueGroup() == ValueGroup.NO_VALUE ) + PointValue fromValue = rangePredicate.from(); + if ( fromValue == null ) { treeKeyFrom.initAsLowest(); } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java index 2771492dfc33..8bfb9c80bd11 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/SpatialSchemaKey.java @@ -48,6 +48,12 @@ class SpatialSchemaKey implements NativeSchemaKey byte type; long rawValueBits; + CoordinateReferenceSystem crs; + + public SpatialSchemaKey( CoordinateReferenceSystem crs ) + { + this.crs = crs; + } @Override public void setEntityIdIsSpecialTieBreaker( boolean entityIdIsSpecialTieBreaker ) @@ -97,9 +103,10 @@ public NumberValue asValue() @Override public void initAsLowest() { + //TODO: Get dimension double[] limit = new double[2]; Arrays.fill(limit, Double.NEGATIVE_INFINITY); - writePoint( CoordinateReferenceSystem.WGS84, limit ); + writePoint( crs, limit ); entityId = Long.MIN_VALUE; entityIdIsSpecialTieBreaker = true; } @@ -107,9 +114,10 @@ public void initAsLowest() @Override public void initAsHighest() { + //TODO: Get dimension double[] limit = new double[2]; Arrays.fill(limit, Double.POSITIVE_INFINITY); - writePoint( CoordinateReferenceSystem.WGS84, limit ); + writePoint( crs, limit ); entityId = Long.MAX_VALUE; entityIdIsSpecialTieBreaker = true; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/SpatialFusionIndexReader.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/SpatialFusionIndexReader.java index e44f08d04404..59a265d1af4e 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/SpatialFusionIndexReader.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/SpatialFusionIndexReader.java @@ -108,7 +108,7 @@ private IndexReader selectIf( IndexQuery... predicates ) throws IndexNotApplicab } else if ( predicates[0] instanceof GeometryRangePredicate ) { - return readerMap.get( ((GeometryRangePredicate) predicates[0]).from().getCoordinateReferenceSystem() ); + return readerMap.get( ((GeometryRangePredicate) predicates[0]).crs() ); } return null; } diff --git a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialFunctionsAcceptanceTest.scala b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialFunctionsAcceptanceTest.scala index a840009e090c..bb6995fe7969 100644 --- a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialFunctionsAcceptanceTest.scala +++ b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialFunctionsAcceptanceTest.scala @@ -443,6 +443,44 @@ class SpatialFunctionsAcceptanceTest extends ExecutionEngineFunSuite with Cypher assert(result.isEmpty) } + test("indexed point - range query 4 - multiple CRS") { + // Given + graph.createIndex("Place", "location") + graph.execute("CREATE (p:Place) SET p.location = point({y: 56.7, x: 12.78, crs: 'cartesian'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 55.7, longitude: 11.78, crs: 'WGS-84'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 50.7, longitude: 12.78, crs: 'WGS-84'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 56.7, longitude: 10.78, crs: 'WGS-84'})") + + // When + val result = innerExecuteDeprecated("CYPHER runtime=slotted MATCH (p:Place) WHERE p.location >= point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'}) RETURN p.location as point", Map.empty) + + // Then + val plan = result.executionPlanDescription() + plan should useOperatorWithText("Projection", "point") + plan should useOperatorWithText("NodeIndexSeekByRange", ":Place(location)") + result.toList should equal(List(Map("point" -> Values.pointValue(CoordinateReferenceSystem.WGS84, 12.78, 56.7)))) + } + + test("indexed point - range query 5 - multiple inequalities") { + // Given + graph.createIndex("Place", "location") + graph.execute("CREATE (p:Place) SET p.location = point({y: 55.7, x: 11.78, crs: 'cartesian'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 55.7, longitude: 11.78, crs: 'WGS-84'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 50.7, longitude: 12.78, crs: 'WGS-84'})") + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 56.7, longitude: 10.78, crs: 'WGS-84'})") + + // When + val result = innerExecuteDeprecated("CYPHER runtime=slotted MATCH (p:Place) WHERE p.location >= point({latitude: 55.7, longitude: 11.78, crs: 'WGS-84'}) AND p.location < point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'}) RETURN p.location as point", Map.empty) + + // Then + val plan = result.executionPlanDescription() + plan should useOperatorWithText("Projection", "point") + plan should useOperatorWithText("NodeIndexSeekByRange", ":Place(location)") + result.toList should equal(List(Map("point" -> Values.pointValue(CoordinateReferenceSystem.WGS84, 11.78, 55.7)))) + } + test("array of points should be assignable to node property") { // Given createLabeledNode("Place") diff --git a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexAcceptanceTest.scala b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexAcceptanceTest.scala index 6080b79f8556..d2a8012e7b31 100644 --- a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexAcceptanceTest.scala +++ b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexAcceptanceTest.scala @@ -85,7 +85,7 @@ class SpatialIndexAcceptanceTest extends CypherFunSuite with GraphDatabaseTestSu testPointRead("MATCH (p:Place) WHERE p.location = point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'}) RETURN p.location as point", Values.pointValue(CoordinateReferenceSystem.WGS84, 12.78, 56.7)) } - test("should handle multiple different types of points and also survive restart") { + test("indexScan should handle multiple different types of points and also survive restart") { graph.createIndex("Place", "location") graph.execute("CREATE (p:Place) SET p.location = point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'})") @@ -99,6 +99,24 @@ class SpatialIndexAcceptanceTest extends CypherFunSuite with GraphDatabaseTestSu testPointScan(query, Values.pointValue(CoordinateReferenceSystem.WGS84, 12.78, 56.7), Values.pointValue(CoordinateReferenceSystem.Cartesian, 1.0, 2.78)) } + test("indexSeek should handle multiple different types of points and also survive restart") { + graph.createIndex("Place", "location") + + graph.execute("CREATE (p:Place) SET p.location = point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'})") + graph.execute("CREATE (p:Place) SET p.location = point({x: 1.0, y: 2.78, crs: 'cartesian'})") + + val query1 = "MATCH (p:Place) WHERE p.location = point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'}) RETURN p.location AS point" + val query2 = "MATCH (p:Place) WHERE p.location = point({x: 1.0, y: 2.78, crs: 'cartesian'}) RETURN p.location AS point" + + testPointRead(query1, Values.pointValue(CoordinateReferenceSystem.WGS84, 12.78, 56.7)) + testPointRead(query2, Values.pointValue(CoordinateReferenceSystem.Cartesian, 1.0, 2.78)) + + restartGraphDatabase() + + testPointRead(query1, Values.pointValue(CoordinateReferenceSystem.WGS84, 12.78, 56.7)) + testPointRead(query2, Values.pointValue(CoordinateReferenceSystem.Cartesian, 1.0, 2.78)) + } + test("overwriting indexed property should work") { graph.createIndex("Place", "location") createLabeledNode("Place") @@ -129,18 +147,17 @@ class SpatialIndexAcceptanceTest extends CypherFunSuite with GraphDatabaseTestSu testPointRead("MATCH (p:Place) WHERE p.location = point({latitude: 56.7, longitude: 12.78, crs: 'WGS-84'}) RETURN p.location as point", Values.pointValue(CoordinateReferenceSystem.WGS84, 12.78, 56.7)) } - private def testPointRead(query: String, expected: PointValue): Unit = { + private def testPointRead(query: String, expected: PointValue*): Unit = { val result = graph.execute(query) val plan = result.getExecutionPlanDescription.toString plan should include ("NodeIndexSeek") plan should include (":Place(location)") - // Then - val point = result.columnAs("point").next().asInstanceOf[Point] - point should equal(expected) - // And CRS names should equal - point.getCRS.getHref should equal("http://spatialreference.org/ref/epsg/4326/") + + val points = result.columnAs("point").stream().collect(Collectors.toSet) + expected.foreach(p => assert(points.contains(p))) + points.size() should be(expected.size) } private def testPointScan(query: String, expected: PointValue*): Unit = {