Skip to content

Commit

Permalink
Reduce object allocation in Comparator
Browse files Browse the repository at this point in the history
  • Loading branch information
pontusmelke committed Apr 19, 2018
1 parent ddbdd39 commit fab77b1
Show file tree
Hide file tree
Showing 10 changed files with 166 additions and 61 deletions.
Expand Up @@ -22,7 +22,7 @@ package org.neo4j.cypher.internal.runtime.interpreted.commands.predicates
import org.neo4j.cypher.internal.runtime.interpreted.ExecutionContext
import org.neo4j.cypher.internal.runtime.interpreted.commands.expressions.{Expression, Literal, Variable}
import org.neo4j.cypher.internal.runtime.interpreted.pipes.QueryState
import org.neo4j.values.AnyValues
import org.neo4j.values.{AnyValues, Comparison}
import org.neo4j.values.storable._

abstract sealed class ComparablePredicate(val left: Expression, val right: Expression) extends Predicate {
Expand All @@ -37,25 +37,25 @@ 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) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: TextValue, n2: TextValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: BooleanValue, n2: BooleanValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: PointValue, n2: PointValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: DateValue, n2: DateValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: LocalTimeValue, n2: LocalTimeValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: TimeValue, n2: TimeValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: LocalDateTimeValue, n2: LocalDateTimeValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: DateTimeValue, n2: DateTimeValue) => compare(nullToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: NumberValue, n2: NumberValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: TextValue, n2: TextValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: BooleanValue, n2: BooleanValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: PointValue, n2: PointValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: DateValue, n2: DateValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: LocalTimeValue, n2: LocalTimeValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: TimeValue, n2: TimeValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: LocalDateTimeValue, n2: LocalDateTimeValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case (n1: DateTimeValue, n2: DateTimeValue) => compare(undefinedToNone(AnyValues.TERNARY_COMPARATOR.ternaryCompare(n1, n2)))
case _ => None
}
res
}

private def nullToNone(i: java.lang.Integer) : Option[Int] = {
private def undefinedToNone(i: Comparison) : 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)
if(i == Comparison.UNDEFINED) None
else Some(i.value())
}

def sign: String
Expand Down
Expand Up @@ -20,9 +20,9 @@
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;

Expand All @@ -31,35 +31,32 @@
*/
class AnyValueComparator implements Comparator<AnyValue>, TernaryComparator<AnyValue>
{
private final Comparator<Value> valueComparator;
private final TernaryComparator<Value> ternaryValueComparator;
private final Comparator<VirtualValueGroup> virtualValueGroupComparator;
private final ValueComparator valueComparator;

AnyValueComparator( Comparator<Value> valueComparator, TernaryComparator<Value> ternaryValueComparator,
Comparator<VirtualValueGroup> virtualValueGroupComparator )
AnyValueComparator( ValueComparator valueComparator, Comparator<VirtualValueGroup> virtualValueGroupComparator )
{
this.valueComparator = valueComparator;
this.ternaryValueComparator = ternaryValueComparator;
this.virtualValueGroupComparator = virtualValueGroupComparator;
this.valueComparator = valueComparator;
}

private Integer cmp( AnyValue v1, AnyValue v2, BiFunction<Value, Value, Integer> compareValues )
private Comparison cmp( AnyValue v1, AnyValue v2, boolean ternary )
{
assert v1 != null && v2 != null : "null values are not supported, use NoValue.NO_VALUE instead";

// NO_VALUE is bigger than all other values, need to check for that up
// front
if ( v1 == v2 )
{
return 0;
return Comparison.EQUAL;
}
if ( v1 == Values.NO_VALUE )
{
return 1;
return Comparison.GREATER_THAN;
}
if ( v2 == Values.NO_VALUE )
{
return -1;
return Comparison.SMALLER_THAN;
}

// We must handle sequences as a special case, as they can be both storable and virtual
Expand All @@ -68,15 +65,15 @@ private Integer cmp( AnyValue v1, AnyValue v2, BiFunction<Value, Value, Integer>

if ( isSequence1 && isSequence2 )
{
return compareSequences( (SequenceValue)v1, (SequenceValue)v2 );
return Comparison.from( compareSequences( (SequenceValue) v1, (SequenceValue) v2 ) );
}
else if ( isSequence1 )
{
return compareSequenceAndNonSequence( (SequenceValue)v1, v2 );
return Comparison.from( compareSequenceAndNonSequence( (SequenceValue) v1, v2 ) );
}
else if ( isSequence2 )
{
return -compareSequenceAndNonSequence( (SequenceValue)v2, v1 );
return Comparison.from( -compareSequenceAndNonSequence( (SequenceValue) v2, v1 ) );
}

// Handle remaining AnyValues
Expand All @@ -91,29 +88,35 @@ else if ( isSequence2 )
// Do not turn this into ?-operator
if ( isValue1 )
{
// This return Integer (null possible!)
return compareValues.apply( (Value) v1, (Value) v2 );
if ( ternary )
{
return valueComparator.ternaryCompare( (Value) v1, (Value) v2 );
}
else
{
return Comparison.from( valueComparator.compare( (Value) v1, (Value) v2 ) );
}
}
else
{
// This returns int
return compareVirtualValues( (VirtualValue)v1, (VirtualValue)v2 );
return Comparison.from( compareVirtualValues( (VirtualValue) v1, (VirtualValue) v2 ) );
}

}
return x;
return Comparison.from( x );
}

@Override
public int compare( AnyValue v1, AnyValue v2 )
{
return cmp( v1, v2, valueComparator::compare );
return cmp( v1, v2, false ).value();
}

@Override
public Integer ternaryCompare( AnyValue v1, AnyValue v2 )
public Comparison ternaryCompare( AnyValue v1, AnyValue v2 )
{
return cmp( v1, v2, ternaryValueComparator::ternaryCompare );
return cmp( v1, v2, true );
}

@Override
Expand Down Expand Up @@ -151,7 +154,7 @@ private int compareSequenceAndNonSequence( SequenceValue v1, AnyValue v2 )
}
else
{
return virtualValueGroupComparator.compare( VirtualValueGroup.LIST, ((VirtualValue)v2).valueGroup() );
return virtualValueGroupComparator.compare( VirtualValueGroup.LIST, ((VirtualValue) v2).valueGroup() );
}
}

Expand Down
Expand Up @@ -67,7 +67,7 @@ public final class AnyValues
* <li> VOID (i.e. the type of null)
* </ul>
*/
private static final AnyValueComparator comp = new AnyValueComparator( Values.COMPARATOR, Values.TERNARY_COMPARATOR, VirtualValueGroup::compareTo );
private static final AnyValueComparator comp = new AnyValueComparator( Values.COMPARATOR, VirtualValueGroup::compareTo );
public static final Comparator<AnyValue> COMPARATOR = comp;
public static final TernaryComparator<AnyValue> TERNARY_COMPARATOR = comp;

Expand Down
103 changes: 103 additions & 0 deletions community/values/src/main/java/org/neo4j/values/Comparison.java
@@ -0,0 +1,103 @@
/*
* 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;

/**
* Defines the result of a ternary comparison.
* <p>
* In a ternary comparison the result may not only be greater than, equal or smaller than but the
* result can also be undefined.
*/
public enum Comparison
{
GREATER_THAN
{
@Override
public int value()
{
return 1;
}

},
EQUAL
{
@Override
public int value()
{
return 0;
}

},
SMALLER_THAN
{
@Override
public int value()
{
return -1;
}

},
UNDEFINED
{
@Override
public int value()
{
throw new IllegalStateException(
"This value is undefined and can't handle primitive comparisons" );
}

};

/**
* Integer representation of comparison
* <p>
* Returns a positive integer if {@link Comparison#GREATER_THAN} than, negative integer for
* {@link Comparison#SMALLER_THAN},
* and zero for {@link Comparison#EQUAL}
*
* @return a positive number if result is greater than, a negative number if the result is smaller than or zero
* if equal.
* @throws IllegalStateException if the result is undefined.
*/
public abstract int value();

/**
* Maps an integer value to comparison result.
*
* @param i the integer to be mapped to a Comparison
* @return {@link Comparison#GREATER_THAN} than if positive, {@link Comparison#SMALLER_THAN} if negative or
* {@link Comparison#EQUAL} if zero
*/
public static Comparison from( int i )
{
if ( i > 0 )
{
return GREATER_THAN;
}
else if ( i < 0 )
{
return SMALLER_THAN;
}
else
{
return EQUAL;
}
}
}
Expand Up @@ -20,7 +20,7 @@
package org.neo4j.values;

/**
* Comparator that allows {@code null}.
* Comparator that allows undefined comparison.
*/
@FunctionalInterface
public interface TernaryComparator<T>
Expand All @@ -31,5 +31,5 @@ public interface TernaryComparator<T>
* 0 if o1 and o2 are equal, and
* {@code null} if they are not comparable.
*/
Integer ternaryCompare( T o1, T o2 );
Comparison ternaryCompare( T o1, T o2 );
}
Expand Up @@ -28,6 +28,7 @@
import org.neo4j.graphdb.spatial.Coordinate;
import org.neo4j.graphdb.spatial.Point;
import org.neo4j.values.AnyValue;
import org.neo4j.values.Comparison;
import org.neo4j.values.ValueMapper;
import org.neo4j.values.utils.InvalidValuesArgumentException;
import org.neo4j.values.utils.PrettyPrinter;
Expand Down Expand Up @@ -156,13 +157,13 @@ int unsafeCompareTo( Value otherValue )
}

@Override
Integer unsafeTernaryCompareTo( Value otherValue )
Comparison unsafeTernaryCompareTo( Value otherValue )
{
PointValue other = (PointValue) otherValue;

if ( this.crs.getCode() != other.crs.getCode() || this.coordinate.length != other.coordinate.length )
{
return null;
return Comparison.UNDEFINED;
}

int result = 0;
Expand All @@ -173,13 +174,13 @@ Integer unsafeTernaryCompareTo( Value otherValue )
{
if ( (cmpVal < 0 && result > 0) || (cmpVal > 0 && result < 0) )
{
return null;
return Comparison.UNDEFINED;
}
result = cmpVal;
}
}

return result;
return Comparison.from( result );
}

@Override
Expand Down Expand Up @@ -257,19 +258,17 @@ public boolean withinRange( PointValue lower, boolean includeLower, PointValue u
{
if ( lower != null )
{
Integer compareLower = this.unsafeTernaryCompareTo( lower );
if ( compareLower == null || compareLower < 0 || compareLower == 0 && !includeLower )
Comparison compareLower = this.unsafeTernaryCompareTo( lower );
if ( compareLower == Comparison.UNDEFINED || compareLower == Comparison.SMALLER_THAN || compareLower == Comparison.EQUAL && !includeLower )
{
return false;
}
}
if ( upper != null )
{
Integer compareUpper = this.unsafeTernaryCompareTo( upper );
if ( compareUpper == null || compareUpper > 0 || compareUpper == 0 && !includeUpper )
{
return false;
}
Comparison compareUpper = this.unsafeTernaryCompareTo( upper );
return compareUpper != Comparison.UNDEFINED && compareUpper != Comparison.GREATER_THAN &&
(compareUpper != Comparison.EQUAL || includeUpper);
}
return true;
}
Expand Down
Expand Up @@ -30,6 +30,7 @@
import org.neo4j.graphdb.spatial.Geometry;
import org.neo4j.values.AnyValue;
import org.neo4j.values.AnyValueWriter;
import org.neo4j.values.Comparison;
import org.neo4j.values.SequenceValue;
import org.neo4j.values.utils.InvalidValuesArgumentException;

Expand Down Expand Up @@ -187,9 +188,9 @@ public Boolean ternaryEquals( AnyValue other )
* Should return {@code null} for values that cannot be compared
* under Comparability semantics.
*/
Integer unsafeTernaryCompareTo( Value other )
Comparison unsafeTernaryCompareTo( Value other )
{
return unsafeCompareTo( other );
return Comparison.from( unsafeCompareTo( other ) );
}

@Override
Expand Down

0 comments on commit fab77b1

Please sign in to comment.