diff --git a/community/cypher/runtime-util/src/main/java/org/neo4j/cypher/operations/CypherBoolean.java b/community/cypher/runtime-util/src/main/java/org/neo4j/cypher/operations/CypherBoolean.java index ec631f68898e..65174445280b 100644 --- a/community/cypher/runtime-util/src/main/java/org/neo4j/cypher/operations/CypherBoolean.java +++ b/community/cypher/runtime-util/src/main/java/org/neo4j/cypher/operations/CypherBoolean.java @@ -233,7 +233,8 @@ else if ( lhs instanceof BooleanValue && rhs instanceof BooleanValue ) } else if ( lhs instanceof PointValue && rhs instanceof PointValue ) { - return lessThanOrEqual( AnyValues.TERNARY_COMPARATOR.ternaryCompare( lhs, rhs ) ); + // The ternary comparator cannot handle the '=' part of the >= and <= cases, so we need to switch to the within function + return ((PointValue) lhs).withinRange( null, false, (PointValue) rhs, true ) ? TRUE : FALSE; } else if ( lhs instanceof DateValue && rhs instanceof DateValue ) { @@ -329,7 +330,8 @@ else if ( lhs instanceof BooleanValue && rhs instanceof BooleanValue ) } else if ( lhs instanceof PointValue && rhs instanceof PointValue ) { - return greaterThanOrEqual( AnyValues.TERNARY_COMPARATOR.ternaryCompare( lhs, rhs ) ); + // The ternary comparator cannot handle the '=' part of the >= and <= cases, so we need to switch to the within function + return ((PointValue) lhs).withinRange( (PointValue) rhs, true, null, false ) ? TRUE : FALSE; } else if ( lhs instanceof DateValue && rhs instanceof DateValue ) { 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 422e12b4574c..e823bbae8509 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 @@ -393,6 +393,14 @@ public boolean toInclusive() { return toInclusive; } + + /** + * @return true if the order defined for this type can also be relied on for bounds comparisons. + */ + public boolean isRegularOrder() + { + return true; + } } public static final class GeometryRangePredicate extends RangePredicate @@ -437,6 +445,15 @@ public PointValue to() { return to; } + + /** + * The order defined for spatial types cannot be used for bounds comparisons. + * @return false + */ + public boolean isRegularOrder() + { + return false; + } } public static final class NumberRangePredicate extends RangePredicate diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultNodeValueIndexCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultNodeValueIndexCursor.java index 8719e3f729c7..6dabadadcf5c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultNodeValueIndexCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultNodeValueIndexCursor.java @@ -287,18 +287,13 @@ private void rangeQuery( IndexDescriptor descriptor, IndexQuery.RangePredicate changes = - TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( txState, descriptor, valueGroup, predicate.fromValue(), - predicate.fromInclusive(), predicate.toValue(), predicate.toInclusive() ); + ReadableDiffSets changes = TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( txState, descriptor, predicate ); addedWithValues = changes.getAdded().iterator(); removed = removed( txState, changes.getRemoved() ); } else { - LongDiffSets changes = TxStateIndexChanges.indexUpdatesForRangeSeek( txState, - descriptor, valueGroup, - predicate.fromValue(), predicate.fromInclusive(), - predicate.toValue(), predicate.toInclusive() ); + LongDiffSets changes = TxStateIndexChanges.indexUpdatesForRangeSeek( txState, descriptor, predicate ); added = changes.augment( ImmutableEmptyLongIterator.INSTANCE ); removed = removed( txState, changes ); } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/TxStateIndexChanges.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/TxStateIndexChanges.java index 81b58e12bf7b..a0e2ff475b4e 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/TxStateIndexChanges.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/TxStateIndexChanges.java @@ -21,7 +21,6 @@ import org.eclipse.collections.impl.UnmodifiableMap; -import java.util.Collection; import java.util.Map; import java.util.NavigableMap; @@ -34,7 +33,6 @@ import org.neo4j.storageengine.api.txstate.ReadableTransactionState; import org.neo4j.values.storable.TextValue; import org.neo4j.values.storable.Value; -import org.neo4j.values.storable.ValueGroup; import org.neo4j.values.storable.ValueTuple; import org.neo4j.values.storable.Values; @@ -144,9 +142,10 @@ static LongDiffSets indexUpdatesForSeek( ReadableTransactionState txState, Index return LongDiffSets.EMPTY; } - static LongDiffSets indexUpdatesForRangeSeek( ReadableTransactionState txState, IndexDescriptor descriptor, ValueGroup valueGroup, Value lower, - boolean includeLower, Value upper, boolean includeUpper ) + static LongDiffSets indexUpdatesForRangeSeek( ReadableTransactionState txState, IndexDescriptor descriptor, IndexQuery.RangePredicate predicate ) { + Value lower = predicate.fromValue(); + Value upper = predicate.toValue(); if ( lower == null || upper == null ) { throw new IllegalStateException( "Use Values.NO_VALUE to encode the lack of a bound" ); @@ -166,46 +165,48 @@ static LongDiffSets indexUpdatesForRangeSeek( ReadableTransactionState txState, if ( lower == NO_VALUE ) { - selectedLower = ValueTuple.of( Values.minValue( valueGroup, upper ) ); + selectedLower = ValueTuple.of( Values.minValue( predicate.valueGroup(), upper ) ); selectedIncludeLower = true; } else { selectedLower = ValueTuple.of( lower ); - selectedIncludeLower = includeLower; + selectedIncludeLower = predicate.fromInclusive(); } if ( upper == NO_VALUE ) { - selectedUpper = ValueTuple.of( Values.maxValue( valueGroup, lower ) ); + selectedUpper = ValueTuple.of( Values.maxValue( predicate.valueGroup(), lower ) ); selectedIncludeUpper = false; } else { selectedUpper = ValueTuple.of( upper ); - selectedIncludeUpper = includeUpper; + selectedIncludeUpper = predicate.toInclusive(); } - return indexUpdatesForRangeSeek( sortedUpdates, selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper ); - } - - private static LongDiffSets indexUpdatesForRangeSeek( NavigableMap sortedUpdates, ValueTuple lower, boolean includeLower, - ValueTuple upper, boolean includeUpper ) - { MutableLongDiffSetsImpl diffs = new MutableLongDiffSetsImpl(); - Collection inRange = sortedUpdates.subMap( lower, includeLower, upper, includeUpper ).values(); - for ( LongDiffSets diffForSpecificValue : inRange ) + Map inRange = sortedUpdates.subMap( selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper ); + for ( Map.Entry entry : inRange.entrySet() ) { - diffs.addAll( diffForSpecificValue.getAdded() ); - diffs.removeAll( diffForSpecificValue.getRemoved() ); + ValueTuple values = entry.getKey(); + LongDiffSets diffForSpecificValue = entry.getValue(); + // The TreeMap cannot perfectly order multi-dimensional types (spatial) and need additional filtering out false positives + if ( predicate.isRegularOrder() || predicate.acceptsValue( values.getOnlyValue() ) ) + { + diffs.addAll( diffForSpecificValue.getAdded() ); + diffs.removeAll( diffForSpecificValue.getRemoved() ); + } } return diffs; } static ReadableDiffSets indexUpdatesWithValuesForRangeSeek( ReadableTransactionState txState, IndexDescriptor descriptor, - ValueGroup valueGroup, Value lower, boolean includeLower, Value upper, boolean includeUpper ) + IndexQuery.RangePredicate predicate ) { + Value lower = predicate.fromValue(); + Value upper = predicate.toValue(); if ( lower == null || upper == null ) { throw new IllegalStateException( "Use Values.NO_VALUE to encode the lack of a bound" ); @@ -225,32 +226,26 @@ static ReadableDiffSets indexUpdatesWithValuesForRangeSe if ( lower == NO_VALUE ) { - selectedLower = ValueTuple.of( Values.minValue( valueGroup, upper ) ); + selectedLower = ValueTuple.of( Values.minValue( predicate.valueGroup(), upper ) ); selectedIncludeLower = true; } else { selectedLower = ValueTuple.of( lower ); - selectedIncludeLower = includeLower; + selectedIncludeLower = predicate.fromInclusive(); } if ( upper == NO_VALUE ) { - selectedUpper = ValueTuple.of( Values.maxValue( valueGroup, lower ) ); + selectedUpper = ValueTuple.of( Values.maxValue( predicate.valueGroup(), lower ) ); selectedIncludeUpper = false; } else { selectedUpper = ValueTuple.of( upper ); - selectedIncludeUpper = includeUpper; + selectedIncludeUpper = predicate.toInclusive(); } - return indexUpdatesWithValuesForRangeSeek( sortedUpdates, selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper ); - } - - private static ReadableDiffSets indexUpdatesWithValuesForRangeSeek( NavigableMap sortedUpdates, - ValueTuple selectedLower, boolean selectedIncludeLower, ValueTuple selectedUpper, boolean selectedIncludeUpper ) - { DiffSets diffs = new DiffSets<>(); Map inRange = sortedUpdates.subMap( selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper ); @@ -260,8 +255,12 @@ private static ReadableDiffSets indexUpdatesWithValuesFo Value[] valuesArray = values.getValues(); LongDiffSets diffForSpecificValue = entry.getValue(); - diffForSpecificValue.getAdded().each( nodeId -> diffs.add( new NodeWithPropertyValues( nodeId, valuesArray ) ) ); - diffForSpecificValue.getRemoved().each( nodeId -> diffs.remove( new NodeWithPropertyValues( nodeId, valuesArray ) ) ); + // The TreeMap cannot perfectly order multi-dimensional types (spatial) and need additional filtering out false positives + if ( predicate.isRegularOrder() || predicate.acceptsValue( values.getOnlyValue() ) ) + { + diffForSpecificValue.getAdded().each( nodeId -> diffs.add( new NodeWithPropertyValues( nodeId, valuesArray ) ) ); + diffForSpecificValue.getRemoved().each( nodeId -> diffs.remove( new NodeWithPropertyValues( nodeId, valuesArray ) ) ); + } } return diffs; } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/TxStateIndexChangesTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/TxStateIndexChangesTest.java index 9d8ad22cd5f2..60906934e8be 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/TxStateIndexChangesTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/TxStateIndexChangesTest.java @@ -217,10 +217,9 @@ private DynamicTest rangeTest( ReadableTransactionState state, Value lo, boolean { return DynamicTest.dynamicTest( String.format( "range seek: lo=%s (incl: %s), hi=%s (incl: %s)", lo, includeLo, hi, includeHi ), () -> { - final LongDiffSets diffSets = - TxStateIndexChanges.indexUpdatesForRangeSeek( state, index, ValueGroup.NUMBER, lo, includeLo, hi, includeHi ); + final LongDiffSets diffSets = TxStateIndexChanges.indexUpdatesForRangeSeek( state, index, IndexQuery.range( -1, lo, includeLo, hi, includeHi ) ); final ReadableDiffSets diffSets2 = - TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( state, index, ValueGroup.NUMBER, lo, includeLo, hi, includeHi ); + TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( state, index, IndexQuery.range( -1, lo, includeLo, hi, includeHi ) ); final LongSet expectedNodeIds = LongSets.immutable.ofAll( Arrays.stream( expected ).mapToLong( NodeWithPropertyValues::getNodeId ) ); assertEquals( expectedNodeIds, diffSets.getAdded() ); 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 f2532017dceb..e880abe5a07e 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 @@ -171,9 +171,14 @@ Comparison unsafeTernaryCompareTo( Value otherValue ) for ( int i = 0; i < coordinate.length; i++ ) { int cmpVal = Double.compare( this.coordinate[i], other.coordinate[i] ); - if ( cmpVal != 0 && cmpVal != result ) + if ( cmpVal == 0 && result != 0 ) { - if ( (cmpVal < 0 && result > 0) || (cmpVal > 0 && result < 0) ) + // Equal on one dimension, but not others, is not defined + return Comparison.UNDEFINED; + } + if ( cmpVal != result ) + { + if ( (cmpVal < 0 && result > 0) || (cmpVal > 0 && result < 0) || (i > 0 && result == 0) ) { return Comparison.UNDEFINED; } @@ -278,7 +283,7 @@ public boolean withinRange( PointValue lower, boolean includeLower, PointValue u { if ( lower == null && upper == null ) { - return false; + return true; } if ( (lower != null) && (this.crs.getCode() != lower.crs.getCode() || this.coordinate.length != lower.coordinate.length) ) { 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 7194725be570..b41a650a47c5 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 @@ -22,9 +22,11 @@ import org.hamcrest.CoreMatchers; import org.junit.jupiter.api.Test; +import org.neo4j.values.Comparison; import org.neo4j.values.utils.InvalidValuesArgumentException; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -83,6 +85,51 @@ void shouldHaveValueGroup() assertNotNull( pointValue( WGS84, 1, 2 ).valueGroup() ); } + //------------------------------------------------------------- + // Comparison tests + + @Test + public void shouldCompareTwoPoints() + { + assertThat( "Two identical points should be equal", pointValue( Cartesian, 1, 2 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 0 ) ); + assertThat( "Different CRS should compare CRS codes", pointValue( Cartesian, 1, 2 ).compareTo( pointValue( WGS84, 1, 2 ) ), equalTo( 1 ) ); + assertThat( "Point greater on both dimensions is greater", pointValue( Cartesian, 2, 3 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) ); + assertThat( "Point greater on first dimensions is greater", pointValue( Cartesian, 2, 2 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) ); + assertThat( "Point greater on second dimensions is greater", pointValue( Cartesian, 1, 3 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) ); + assertThat( "Point smaller on both dimensions is smaller", pointValue( Cartesian, 0, 1 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) ); + assertThat( "Point smaller on first dimensions is smaller", pointValue( Cartesian, 0, 2 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) ); + assertThat( "Point smaller on second dimensions is smaller", pointValue( Cartesian, 1, 1 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) ); + assertThat( "Point greater on first and smaller on second dimensions is greater", + pointValue( Cartesian, 2, 1 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) ); + assertThat( "Point smaller on first and greater on second dimensions is smaller", + pointValue( Cartesian, 0, 3 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) ); + } + + @Test + public void shouldTernaryCompareTwoPoints() + { + assertThat( "Two identical points should be equal", pointValue( Cartesian, 1, 2 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), + equalTo( Comparison.EQUAL ) ); + assertThat( "Different CRS should be incomparable", pointValue( Cartesian, 1, 2 ).unsafeTernaryCompareTo( pointValue( WGS84, 1, 2 ) ), + equalTo( Comparison.UNDEFINED ) ); + assertThat( "Point greater on both dimensions is greater", pointValue( Cartesian, 2, 3 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), + equalTo( Comparison.GREATER_THAN ) ); + assertThat( "Point greater on first dimensions is UNDEFINED", pointValue( Cartesian, 2, 2 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), + equalTo( Comparison.UNDEFINED ) ); + assertThat( "Point greater on second dimensions is UNDEFINED", pointValue( Cartesian, 1, 3 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), + equalTo( Comparison.UNDEFINED ) ); + assertThat( "Point smaller on both dimensions is smaller", pointValue( Cartesian, 0, 1 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), + equalTo( Comparison.SMALLER_THAN ) ); + assertThat( "Point smaller on first dimensions is UNDEFINED", pointValue( Cartesian, 0, 2 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), + equalTo( Comparison.UNDEFINED ) ); + assertThat( "Point smaller on second dimensions is UNDEFINED", pointValue( Cartesian, 1, 1 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), + equalTo( Comparison.UNDEFINED ) ); + assertThat( "Point greater on first and smaller on second dimensions is UNDEFINED", + pointValue( Cartesian, 2, 1 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( Comparison.UNDEFINED ) ); + assertThat( "Point smaller on first and greater on second dimensions is UNDEFINED", + pointValue( Cartesian, 0, 3 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( Comparison.UNDEFINED ) ); + } + //------------------------------------------------------------- // Parser tests diff --git a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexResultsAcceptanceTest.scala b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexResultsAcceptanceTest.scala index d24f4b44f23c..6d0013f73f06 100644 --- a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexResultsAcceptanceTest.scala +++ b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/SpatialIndexResultsAcceptanceTest.scala @@ -573,12 +573,18 @@ class SpatialIndexResultsAcceptanceTest extends IndexingTestSupport { ).toList.foreach { case (op, expected) => // When - val includingBorder = innerExecuteDeprecated(s"MATCH (n:Label) WHERE n.prop $op point({x: 1, y: 5}) RETURN n") + val resultsWithIndex = innerExecuteDeprecated(s"MATCH (n:Label) WHERE n.prop $op point({x: 1, y: 5}) RETURN n").toList.map(_ ("n")) + val resultsWithoutIndex = innerExecuteDeprecated(s"MATCH (n) WHERE n.prop $op point({x: 1, y: 5}) RETURN n").toList.map(_ ("n")) // Then val expectAxes = op contains "=" - withClue(s"Should ${if(expectAxes) "" else "NOT "}find nodes that are on the axes defined by the search point when using operator '$op'") { - includingBorder.toList.map(_ ("n")).toSet should be(expected) + withClue(s"Should ${if(expectAxes) "" else "NOT "}find nodes that are on the axes defined by the search point when using operator '$op' and index") { + resultsWithIndex.toSet should be(expected) + } + + // And should get same results without an index + withClue(s"Should ${if(expectAxes) "" else "NOT "}find nodes that are on the axes defined by the search point when using operator '$op' and no index") { + resultsWithoutIndex.toSet should be(expected) } } // When running a spatial range query for points greater than or equal to the search point @@ -626,4 +632,28 @@ class SpatialIndexResultsAcceptanceTest extends IndexingTestSupport { assertRangeScanFor(">=", minPoint, "<=", maxPoint, vertices :+ nodeToFind: _*) assertRangeScanFor(">", minPoint, "<", maxPoint, nodeToFind) } + + test("should not return points on edges even when using transaction state") { + val atPoint = Values.pointValue(CoordinateReferenceSystem.WGS84, 1, 5) + val onAxisX = Values.pointValue(CoordinateReferenceSystem.WGS84, 1, 4) + val onAxisY = Values.pointValue(CoordinateReferenceSystem.WGS84, 0, 5) + val inRange = Values.pointValue(CoordinateReferenceSystem.WGS84, 0, 4) + val query = "MATCH (n:Label) WHERE n.prop < {prop} RETURN n, n.prop AS prop ORDER BY id(n)" + createIndex() + createIndexedNode(atPoint) + + def runTest(name: String): Unit = { + val results = innerExecuteDeprecated(query, scala.Predef.Map("prop" -> atPoint)) + println(results.executionPlanDescription()) + withClue(s"Should not find on-axis points when searching for < $atPoint ($name)") { + results.toList.map(_ ("prop")).toSet should be(Set(inRange)) + } + } + + graph.inTx { + Seq(onAxisX, onAxisY, inRange).foreach(createIndexedNode(_)) + runTest("nodes in same transaction") + } + runTest("no transaction state") + } }