Implement ternary compare for PointValues

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 = ">="
 

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<AnyValue>
+public class AnyValueComparator implements Comparator<AnyValue>, TernaryComparator<Value>
 {
-    private final Comparator<Value> valueComparator;
+    private final ValueComparator valueComparator;
     private final Comparator<VirtualValueGroup> virtualValueGroupComparator;
 
-    AnyValueComparator( Comparator<Value> valueComparator,
+    AnyValueComparator( ValueComparator valueComparator,
             Comparator<VirtualValueGroup> virtualValueGroupComparator )
     {
         this.valueComparator = valueComparator;
         this.virtualValueGroupComparator = virtualValueGroupComparator;
     }
 
-    @Override
-    public int compare( AnyValue v1, AnyValue v2 )
+    private Integer cmp( AnyValue v1, AnyValue v2, BiFunction<Value, Value, Integer> 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 )
     {

community/values/src/main/java/org/neo4j/values/AnyValues.java

@@ -67,6 +67,6 @@ public final class AnyValues
      *  <li> VOID (i.e. the type of null)
      * </ul>
      */
-    public static final Comparator<AnyValue> COMPARATOR =
+    public static final AnyValueComparator COMPARATOR =
             new AnyValueComparator( Values.COMPARATOR, VirtualValueGroup::compareTo );
 }

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 <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.values;
+
+/**
+ * Comparator that allows {@code null}.
+ */
+@FunctionalInterface
+public interface TernaryComparator<T>
+{
+    /**
+     * 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);
+}

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()
     {

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 <E extends Exception> void writeTo( AnyValueWriter<E> writer ) throws E
     {

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<Value>
+public class ValueComparator implements Comparator<Value>, TernaryComparator<Value>
 {
     private final Comparator<ValueGroup> 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 )
     {

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<Value> COMPARATOR = new ValueComparator( ValueGroup::compareTo );
+    public static final ValueComparator COMPARATOR = new ValueComparator( ValueGroup::compareTo );
 
     public static boolean isNumberValue( Object value )
     {

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")