From e5132685770999fa1be038c7911e2be9bdd2a59f Mon Sep 17 00:00:00 2001 From: Satia Herfert Date: Wed, 21 Feb 2018 17:26:17 +0100 Subject: [PATCH] Implement ternary compare for PointValues --- .../predicates/ComparablePredicate.scala | 25 +++++--- .../org/neo4j/values/AnyValueComparator.java | 25 ++++++-- .../main/java/org/neo4j/values/AnyValues.java | 2 +- .../org/neo4j/values/TernaryComparator.java | 35 +++++++++++ .../org/neo4j/values/storable/PointValue.java | 31 +++++++++ .../java/org/neo4j/values/storable/Value.java | 9 +++ .../values/storable/ValueComparator.java | 21 ++++++- .../org/neo4j/values/storable/Values.java | 7 ++- .../SpatialFunctionsAcceptanceTest.scala | 63 +++++++++++++++++++ 9 files changed, 197 insertions(+), 21 deletions(-) create mode 100644 community/values/src/main/java/org/neo4j/values/TernaryComparator.java diff --git a/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/predicates/ComparablePredicate.scala b/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/predicates/ComparablePredicate.scala index 8a7b588d922c9..f23582c2e29a7 100644 --- a/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/predicates/ComparablePredicate.scala +++ b/community/cypher/interpreted-runtime/src/main/scala/org/neo4j/cypher/internal/runtime/interpreted/commands/predicates/ComparablePredicate.scala @@ -27,7 +27,7 @@ import org.neo4j.values.storable._ abstract sealed class ComparablePredicate(val left: Expression, val right: Expression) extends Predicate { - def compare(comparisonResult: Int): Boolean + def compare(comparisonResult: Option[Int]): Option[Boolean] def isMatch(m: ExecutionContext, state: QueryState): Option[Boolean] = { val l = left(m, state) @@ -37,15 +37,22 @@ abstract sealed class ComparablePredicate(val left: Expression, val right: Expre else (l, r) match { case (d: FloatingPointValue, _) if d.doubleValue().isNaN => None case (_, d: FloatingPointValue) if d.doubleValue().isNaN => None - case (n1: NumberValue, n2: NumberValue) => Some(compare(AnyValues.COMPARATOR.compare(n1, n2))) - case (n1: TextValue, n2: TextValue) => Some(compare(AnyValues.COMPARATOR.compare(n1, n2))) - case (n1: BooleanValue, n2: BooleanValue) => Some(compare(AnyValues.COMPARATOR.compare(n1, n2))) - case (n1: PointValue, n2: PointValue) => Some(compare(AnyValues.COMPARATOR.compare(n1, n2))) + case (n1: NumberValue, n2: NumberValue) => compare(nullToNone(AnyValues.COMPARATOR.ternaryCompare(n1, n2))) + case (n1: TextValue, n2: TextValue) => compare(nullToNone(AnyValues.COMPARATOR.ternaryCompare(n1, n2))) + case (n1: BooleanValue, n2: BooleanValue) => compare(nullToNone(AnyValues.COMPARATOR.ternaryCompare(n1, n2))) + case (n1: PointValue, n2: PointValue) => compare(nullToNone(AnyValues.COMPARATOR.ternaryCompare(n1, n2))) case _ => None } res } + private def nullToNone(i: java.lang.Integer) : Option[Int] = { + // Do NOT use Option here (as suggested by the warning). + // This would lead to NullPointerExceptions when i == null + if(i == null) None + else Some(i) + } + def sign: String override def toString = left.toString() + " " + sign + " " + right.toString() @@ -98,7 +105,7 @@ case class Equals(a: Expression, b: Expression) extends Predicate { case class LessThan(a: Expression, b: Expression) extends ComparablePredicate(a, b) { - def compare(comparisonResult: Int) = comparisonResult < 0 + override def compare(comparisonResult: Option[Int]): Option[Boolean] = comparisonResult.map(_ < 0) def sign: String = "<" @@ -107,7 +114,7 @@ case class LessThan(a: Expression, b: Expression) extends ComparablePredicate(a, case class GreaterThan(a: Expression, b: Expression) extends ComparablePredicate(a, b) { - def compare(comparisonResult: Int) = comparisonResult > 0 + override def compare(comparisonResult: Option[Int]): Option[Boolean] = comparisonResult.map(_ > 0) def sign: String = ">" @@ -116,7 +123,7 @@ case class GreaterThan(a: Expression, b: Expression) extends ComparablePredicate case class LessThanOrEqual(a: Expression, b: Expression) extends ComparablePredicate(a, b) { - def compare(comparisonResult: Int) = comparisonResult <= 0 + override def compare(comparisonResult: Option[Int]): Option[Boolean] = comparisonResult.map(_ <= 0) def sign: String = "<=" @@ -125,7 +132,7 @@ case class LessThanOrEqual(a: Expression, b: Expression) extends ComparablePredi case class GreaterThanOrEqual(a: Expression, b: Expression) extends ComparablePredicate(a, b) { - def compare(comparisonResult: Int) = comparisonResult >= 0 + override def compare(comparisonResult: Option[Int]): Option[Boolean] = comparisonResult.map(_ >= 0) def sign: String = ">=" diff --git a/community/values/src/main/java/org/neo4j/values/AnyValueComparator.java b/community/values/src/main/java/org/neo4j/values/AnyValueComparator.java index 3ca91110b430a..8f571c87626d7 100644 --- a/community/values/src/main/java/org/neo4j/values/AnyValueComparator.java +++ b/community/values/src/main/java/org/neo4j/values/AnyValueComparator.java @@ -20,28 +20,29 @@ package org.neo4j.values; import java.util.Comparator; +import java.util.function.BiFunction; import org.neo4j.values.storable.Value; +import org.neo4j.values.storable.ValueComparator; import org.neo4j.values.storable.Values; import org.neo4j.values.virtual.VirtualValueGroup; /** * Comparator for any values. */ -class AnyValueComparator implements Comparator +public class AnyValueComparator implements Comparator, TernaryComparator { - private final Comparator valueComparator; + private final ValueComparator valueComparator; private final Comparator virtualValueGroupComparator; - AnyValueComparator( Comparator valueComparator, + AnyValueComparator( ValueComparator valueComparator, Comparator virtualValueGroupComparator ) { this.valueComparator = valueComparator; this.virtualValueGroupComparator = virtualValueGroupComparator; } - @Override - public int compare( AnyValue v1, AnyValue v2 ) + private Integer cmp( AnyValue v1, AnyValue v2, BiFunction compareValues ) { assert v1 != null && v2 != null : "null values are not supported, use NoValue.NO_VALUE instead"; @@ -86,12 +87,24 @@ else if ( isSequence2 ) if ( x == 0 ) { //noinspection ConstantConditions - return isValue1 ? valueComparator.compare( (Value)v1, (Value)v2 ) : + return isValue1 ? compareValues.apply( (Value)v1, (Value)v2 ) : compareVirtualValues( (VirtualValue)v1, (VirtualValue)v2 ); } return x; } + @Override + public int compare( AnyValue v1, AnyValue v2 ) + { + return cmp( v1, v2, valueComparator::compare ); + } + + @Override + public Integer ternaryCompare( Value v1, Value v2 ) + { + return cmp( v1, v2, valueComparator::ternaryCompare ); + } + @Override public boolean equals( Object obj ) { diff --git a/community/values/src/main/java/org/neo4j/values/AnyValues.java b/community/values/src/main/java/org/neo4j/values/AnyValues.java index 6c11fbd1d1614..8781aff0a35f9 100644 --- a/community/values/src/main/java/org/neo4j/values/AnyValues.java +++ b/community/values/src/main/java/org/neo4j/values/AnyValues.java @@ -67,6 +67,6 @@ public final class AnyValues *
  • VOID (i.e. the type of null) * */ - public static final Comparator COMPARATOR = + public static final AnyValueComparator COMPARATOR = new AnyValueComparator( Values.COMPARATOR, VirtualValueGroup::compareTo ); } diff --git a/community/values/src/main/java/org/neo4j/values/TernaryComparator.java b/community/values/src/main/java/org/neo4j/values/TernaryComparator.java new file mode 100644 index 0000000000000..3766d248546cb --- /dev/null +++ b/community/values/src/main/java/org/neo4j/values/TernaryComparator.java @@ -0,0 +1,35 @@ +/* + * Copyright (c) 2002-2018 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.values; + +/** + * Comparator that allows {@code null}. + */ +@FunctionalInterface +public interface TernaryComparator +{ + /** + * Should return a negative integer if o1 is smaller than o2, + * a positive integer if o1 is bigger than o2, + * 0 if o1 and o2 are equal, and + * {@code null} if they are not comparable. + */ + Integer ternaryCompare(T o1, T o2); +} 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 bbc71a53c0f01..20c84da488029 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 @@ -140,6 +140,37 @@ else if ( this.coordinate.length < other.coordinate.length ) return 0; } + @Override + public Integer ternaryCompareTo( Value otherValue ) + { + if ( !(otherValue instanceof PointValue) ) + { + throw new IllegalArgumentException( "Cannot compare different values" ); + } + PointValue other = (PointValue) otherValue; + + if ( this.crs.getCode() != other.crs.getCode() || this.coordinate.length != other.coordinate.length ) + { + return null; + } + + int result = 0; + 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) || (cmpVal > 0 && result < 0)) + { + return null; + } + result = cmpVal; + } + } + + return result; + } + @Override public Point asObjectCopy() { diff --git a/community/values/src/main/java/org/neo4j/values/storable/Value.java b/community/values/src/main/java/org/neo4j/values/storable/Value.java index 577a5e5175ff1..9f9d3a77957ee 100644 --- a/community/values/src/main/java/org/neo4j/values/storable/Value.java +++ b/community/values/src/main/java/org/neo4j/values/storable/Value.java @@ -172,6 +172,15 @@ public Boolean ternaryEquals( AnyValue other ) public abstract int compareTo( Value other ); + /** + * Should return {@code null} for values that cannot be compared + * under Comparability semantics. + */ + public Integer ternaryCompareTo( Value other) + { + return compareTo( other ); + } + @Override public void writeTo( AnyValueWriter writer ) throws E { diff --git a/community/values/src/main/java/org/neo4j/values/storable/ValueComparator.java b/community/values/src/main/java/org/neo4j/values/storable/ValueComparator.java index 8f59a93fdc28f..d850f352faee7 100644 --- a/community/values/src/main/java/org/neo4j/values/storable/ValueComparator.java +++ b/community/values/src/main/java/org/neo4j/values/storable/ValueComparator.java @@ -21,12 +21,12 @@ import java.util.Comparator; -import static java.lang.String.format; +import org.neo4j.values.TernaryComparator; /** * Comparator for values. Usable for sorting values, for example during index range scans. */ -public class ValueComparator implements Comparator +public class ValueComparator implements Comparator, TernaryComparator { private final Comparator valueGroupComparator; @@ -53,6 +53,23 @@ public int compare( Value v1, Value v2 ) return x; } + @Override + public Integer ternaryCompare( Value v1, Value v2 ) + { + assert v1 != null && v2 != null : "null values are not supported, use NoValue.NO_VALUE instead"; + + ValueGroup id1 = v1.valueGroup(); + ValueGroup id2 = v2.valueGroup(); + + int x = valueGroupComparator.compare( id1, id2 ); + + if ( x == 0 ) + { + return v1.ternaryCompareTo( v2 ); + } + return x; + } + @Override public boolean equals( Object obj ) { diff --git a/community/values/src/main/java/org/neo4j/values/storable/Values.java b/community/values/src/main/java/org/neo4j/values/storable/Values.java index 7cd697adccffd..85b71f29d6fec 100644 --- a/community/values/src/main/java/org/neo4j/values/storable/Values.java +++ b/community/values/src/main/java/org/neo4j/values/storable/Values.java @@ -81,7 +81,6 @@ public final class Values public static final ArrayValue EMPTY_LONG_ARRAY = Values.longArray( new long[0] ); public static final ArrayValue EMPTY_FLOAT_ARRAY = Values.floatArray( new float[0] ); public static final ArrayValue EMPTY_DOUBLE_ARRAY = Values.doubleArray( new double[0] ); - public static final ArrayValue EMPTY_POINT_ARRAY = Values.pointArray( new PointValue[0] ); public static final TextArray EMPTY_TEXT_ARRAY = Values.stringArray(); private Values() @@ -90,9 +89,11 @@ private Values() /** * Default value comparator. Will correctly compare all storable values and order the value groups according the - * to comparability group. + * to orderability group. + * + * To get Comparability semantics, use .ternaryCompare */ - public static final Comparator COMPARATOR = new ValueComparator( ValueGroup::compareTo ); + public static final ValueComparator COMPARATOR = new ValueComparator( ValueGroup::compareTo ); public static boolean isNumberValue( Object value ) { 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 82ea8f51c08da..22963c101347c 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 @@ -389,6 +389,69 @@ class SpatialFunctionsAcceptanceTest extends ExecutionEngineFunSuite with Cypher result.toList should equal(List(Map("point" -> Values.pointValue(CoordinateReferenceSystem.WGS84, 12.78, 56.7)))) } + // TODO add 3D here too + test("inequality on cartesian points") { + // case same point + shouldCompareLike("point({x: 0, y: 0})", "point({x: 0, y: 0})", aBiggerB = false, aSmallerB = false) + + // case top right quadrant + shouldCompareLike("point({x: 1, y: 1})", "point({x: 0, y: 0})", aBiggerB = true, aSmallerB = false) + // case bottom left quadrant + shouldCompareLike("point({x: -1, y: -1})", "point({x: 0, y: 0})", aBiggerB = false, aSmallerB = true) + // case top left quadrant + shouldCompareLike("point({x: -1, y: 1})", "point({x: 0, y: 0})", aBiggerB = null, aSmallerB = null) + // case bottom right quadrant + shouldCompareLike("point({x: 1, y: -1})", "point({x: 0, y: 0})", aBiggerB = null, aSmallerB = null) + + // case staight top + shouldCompareLike("point({x: 0, y: 1})", "point({x: 0, y: 0})", aBiggerB = true, aSmallerB = false) + // case staight right + shouldCompareLike("point({x: 1, y: 0})", "point({x: 0, y: 0})", aBiggerB = true, aSmallerB = false) + // case staight bottom + shouldCompareLike("point({x: 0, y: -1})", "point({x: 0, y: 0})", aBiggerB = false, aSmallerB = true) + // case staight left + shouldCompareLike("point({x: -1, y: 0})", "point({x: 0, y: 0})", aBiggerB = false, aSmallerB = true) + } + + // TODO what about the poles!? + test("inequality on geographic points") { + // case same point + shouldCompareLike("point({longitude: 0, latitude: 0})", "point({longitude: 0, latitude: 0})", aBiggerB = false, aSmallerB = false) + + // case top right quadrant + shouldCompareLike("point({longitude: 1, latitude: 1})", "point({longitude: 0, latitude: 0})", aBiggerB = true, aSmallerB = false) + // case bottom left quadrant + shouldCompareLike("point({longitude: -1, latitude: -1})", "point({longitude: 0, latitude: 0})", aBiggerB = false, aSmallerB = true) + // case top left quadrant + shouldCompareLike("point({longitude: -1, latitude: 1})", "point({longitude: 0, latitude: 0})", aBiggerB = null, aSmallerB = null) + // case bottom right quadrant + shouldCompareLike("point({longitude: 1, latitude: -1})", "point({longitude: 0, latitude: 0})", aBiggerB = null, aSmallerB = null) + + // case staight top + shouldCompareLike("point({longitude: 0, latitude: 1})", "point({longitude: 0, latitude: 0})", aBiggerB = true, aSmallerB = false) + // case staight right + shouldCompareLike("point({longitude: 1, latitude: 0})", "point({longitude: 0, latitude: 0})", aBiggerB = true, aSmallerB = false) + // case staight bottom + shouldCompareLike("point({longitude: 0, latitude: -1})", "point({longitude: 0, latitude: 0})", aBiggerB = false, aSmallerB = true) + // case staight left + shouldCompareLike("point({longitude: -1, latitude: 0})", "point({longitude: 0, latitude: 0})", aBiggerB = false, aSmallerB = true) + } + + test("inequality on mixed points") { + shouldCompareLike("point({longitude: 0, latitude: 0})", "point({x: 0, y: 0})", aBiggerB = null, aSmallerB = null) + } + + private def shouldCompareLike(a: String, b: String, aBiggerB: Any, aSmallerB: Any) = { + val query = + s"""WITH $a as a, $b as b + |RETURN a > b, a < b + """.stripMargin + + val pointConfig = Configs.Interpreted - Configs.BackwardsCompatibility - Configs.AllRulePlanners + val result = executeWith(pointConfig, query).toList + result should equal(List(Map("a > b" -> aBiggerB, "a < b" -> aSmallerB))) + } + test("indexed points far apart in cartesian space - range query greaterThan") { // Given graph.createIndex("Place", "location")