Stores actual value in NumberKey

This to be able to handle cases where two longs coerce to the same double value
and to handle that comparison needs to be made on actual values in the key.
This actually opened up an opportunity for reducing the entry size by only storing
the actual value and implementing comparison for those directly.

For the future: if the comparison logic on the actual values are too slow then
the key will have to include the compareValue too, inflating the entry size again.

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java

@@ -104,7 +104,7 @@ private void assertOpen()
     }
 
     private static <KEY extends NumberKey, VALUE extends NumberValue> void processRemove( KEY treeKey,
-            IndexEntryUpdate update, Writer<KEY,VALUE> writer ) throws IOException
+            IndexEntryUpdate<?> update, Writer<KEY,VALUE> writer ) throws IOException
     {
         // todo Do we need to verify that we actually removed something at all?
         // todo Difference between online and recovery?
@@ -113,7 +113,7 @@ private static <KEY extends NumberKey, VALUE extends NumberValue> void processRe
     }
 
     private static <KEY extends NumberKey, VALUE extends NumberValue> void processChange( KEY treeKey, VALUE treeValue,
-            IndexEntryUpdate update, Writer<KEY,VALUE> writer,
+            IndexEntryUpdate<?> update, Writer<KEY,VALUE> writer,
             ConflictDetectingValueMerger<KEY,VALUE> conflictDetectingValueMerger )
             throws IOException, IndexEntryConflictException
     {
@@ -128,7 +128,7 @@ private static <KEY extends NumberKey, VALUE extends NumberValue> void processCh
     }
 
     static <KEY extends NumberKey, VALUE extends NumberValue> void processAdd( KEY treeKey, VALUE treeValue,
-            IndexEntryUpdate update, Writer<KEY,VALUE> writer,
+            IndexEntryUpdate<?> update, Writer<KEY,VALUE> writer,
             ConflictDetectingValueMerger<KEY,VALUE> conflictDetectingValueMerger )
             throws IOException, IndexEntryConflictException
     {
@@ -138,7 +138,7 @@ static <KEY extends NumberKey, VALUE extends NumberValue> void processAdd( KEY t
         assertNoConflict( update, conflictDetectingValueMerger );
     }
 
-    private static <KEY extends NumberKey, VALUE extends NumberValue> void assertNoConflict( IndexEntryUpdate update,
+    private static <KEY extends NumberKey, VALUE extends NumberValue> void assertNoConflict( IndexEntryUpdate<?> update,
             ConflictDetectingValueMerger<KEY,VALUE> conflictDetectingValueMerger ) throws IndexEntryConflictException
     {
         if ( conflictDetectingValueMerger.wasConflict() )

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NonUniqueNumberLayout.java

@@ -35,7 +35,7 @@ public long identifier()
     @Override
     public int compare( NumberKey o1, NumberKey o2 )
     {
-        int compare = Double.compare( o1.value, o2.value );
-        return compare != 0 ? compare : Long.compare( o1.entityId, o2.entityId );
+        int comparison = o1.compareValueTo( o2 );
+        return comparison != 0 ? comparison : Long.compare( o1.entityId, o2.entityId );
     }
 }

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberKey.java

@@ -25,18 +25,28 @@
 import static org.neo4j.kernel.impl.index.schema.NumberValueConversion.assertValidSingleNumber;
 
 /**
- * Includes comparison value and entity id (to be able to handle non-unique values).
- * Comparison value is basically any number as a double, a conversion which is lossy by nature,
- * especially for higher decimal values. Actual value is stored in {@link NumberValue}
- * for ability to filter accidental coersions directly internally.
+ * Includes value and entity id (to be able to handle non-unique values).
+ * A value can be any {@link Number} and is represented as a {@code long} to store the raw bits and a type
+ * to say if it's a long, double or float.
+ *
+ * Distinction between double and float exists because coersions between each other and long may differ.
+ * TODO this should be figured out and potentially reduced to long, double types only.
  */
 class NumberKey
 {
     static final int SIZE =
-            Long.SIZE + /* compare value (double represented by long) */
-            Long.SIZE;  /* entityId */
+            Byte.BYTES + /* type of value */
+            Long.BYTES + /* raw value bits */
+
+            // TODO this could use 6 bytes instead and have the highest 2 bits stored in the type byte
+            Long.BYTES;  /* entityId */
+
+    static final byte TYPE_LONG = 0;
+    static final byte TYPE_FLOAT = 1;
+    static final byte TYPE_DOUBLE = 2;
 
-    double value;
+    byte type;
+    long rawValueBits;
     long entityId;
 
     /**
@@ -48,35 +58,126 @@ class NumberKey
      */
     boolean entityIdIsSpecialTieBreaker;
 
-    public void from( long entityId, Value[] values )
+    void from( long entityId, Value[] values )
     {
-        this.value = assertValidSingleNumber( values ).doubleValue();
+        extractValue( assertValidSingleNumber( values ) );
         this.entityId = entityId;
         entityIdIsSpecialTieBreaker = false;
     }
 
     String propertiesAsString()
     {
-        return String.valueOf( value );
+        return String.valueOf( toNumberValue() );
     }
 
     void initAsLowest()
     {
-        value = Double.NEGATIVE_INFINITY;
+        rawValueBits = Double.doubleToLongBits( Double.NEGATIVE_INFINITY );
+        type = TYPE_DOUBLE;
         entityId = Long.MIN_VALUE;
         entityIdIsSpecialTieBreaker = true;
     }
 
     void initAsHighest()
     {
-        value = Double.POSITIVE_INFINITY;
+        rawValueBits = Double.doubleToLongBits( Double.POSITIVE_INFINITY );
+        type = TYPE_DOUBLE;
         entityId = Long.MAX_VALUE;
         entityIdIsSpecialTieBreaker = true;
     }
 
+    /**
+     * Compares the value of this key to that of another key.
+     * This method is expected to be called in scenarios where inconsistent reads may happen (and later retried).
+     *
+     * @param other the {@link NumberKey} to compare to.
+     * @return comparison against the {@code other} {@link NumberKey}.
+     */
+    int compareValueTo( NumberKey other )
+    {
+        return type == TYPE_LONG && other.type == TYPE_LONG
+                // If both are long values then compare them directly, w/o going through double.
+                // This is because at high values longs have higher precision, or double lower rather,
+                // than double values, so converting them to doubles and comparing would have false positives.
+                ? Long.compare( rawValueBits, other.rawValueBits )
+
+                // Otherwise convert both to double and compare, with the reasoning that the long precision
+                // cannot be upheld anyway and double precious being higher than float precision.
+                : Double.compare( doubleValue(), other.doubleValue() );
+    }
+
+    /**
+     * @return the value as double, with potential precision loss.
+     */
+    private double doubleValue()
+    {
+        switch ( type )
+        {
+        case TYPE_LONG:
+            return rawValueBits;
+        case TYPE_FLOAT:
+            return Float.intBitsToFloat( (int) rawValueBits );
+        case TYPE_DOUBLE:
+            return Double.longBitsToDouble( rawValueBits );
+        default:
+            // This is interesting: because of the nature of the page cache and the point in time this method
+            // is called we cannot really throw exception here if type is something unexpected - it may simply
+            // have been an inconsistent read, which will be retried.
+            // It's not for us to decide here, so let's return NaN here.
+            return Double.NaN;
+        }
+    }
+
+    /**
+     * Extracts data from a {@link Number} into state of this {@link NumberKey} instance.
+     *
+     * @param value actual {@link Number} value.
+     */
+    private void extractValue( Number value )
+    {
+        if ( value instanceof Double )
+        {
+            type = TYPE_DOUBLE;
+            rawValueBits = Double.doubleToLongBits( (Double) value );
+        }
+        else if ( value instanceof Float )
+        {
+            type = TYPE_FLOAT;
+            rawValueBits = Float.floatToIntBits( (Float) value );
+        }
+        else
+        {
+            type = TYPE_LONG;
+            rawValueBits = value.longValue();
+        }
+    }
+
+    /**
+     * Useful for getting the value as {@link Number} for e.g. printing or converting to {@link String}.
+     * This method isn't and should not be called on a hot path.
+     *
+     * @return a {@link Number} of correct type, i.e. {@link Long}, {@link Float} or {@link Double}.
+     */
+    private Number toNumberValue()
+    {
+        switch ( type )
+        {
+        case TYPE_LONG:
+            return rawValueBits;
+        case TYPE_FLOAT:
+            return Float.intBitsToFloat( (int)rawValueBits );
+        case TYPE_DOUBLE:
+            return Double.longBitsToDouble( rawValueBits );
+        default:
+            // Unlike in compareValueTo() it is assumed here that the value have been consistently read
+            // and that the value is put to some actual use.
+            throw new IllegalArgumentException( "Unexpected type " + type );
+        }
+    }
+
     @Override
     public String toString()
     {
-        return "compareValue=" + value + ",entityId=" + entityId;
+        return "type=" + type + ",rawValue=" + rawValueBits + ",value=" + toNumberValue() + ",entityId=" + entityId;
     }
 }

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberLayout.java

@@ -37,7 +37,8 @@ public NumberKey newKey()
     public NumberKey copyKey( NumberKey key,
             NumberKey into )
     {
-        into.value = key.value;
+        into.type = key.type;
+        into.rawValueBits = key.rawValueBits;
         into.entityId = key.entityId;
         into.entityIdIsSpecialTieBreaker = key.entityIdIsSpecialTieBreaker;
         return into;
@@ -46,7 +47,7 @@ public NumberKey copyKey( NumberKey key,
     @Override
     public NumberValue newValue()
     {
-        return new NumberValue();
+        return NumberValue.INSTANCE;
     }
 
     @Override
@@ -64,29 +65,27 @@ public int valueSize()
     @Override
     public void writeKey( PageCursor cursor, NumberKey key )
     {
-        cursor.putLong( Double.doubleToRawLongBits( key.value ) );
+        cursor.putByte( key.type );
+        cursor.putLong( key.rawValueBits );
         cursor.putLong( key.entityId );
     }
 
     @Override
-    public void writeValue( PageCursor cursor, NumberValue key )
+    public void writeValue( PageCursor cursor, NumberValue value )
     {
-        cursor.putByte( key.type );
-        cursor.putLong( key.rawValueBits );
     }
 
     @Override
     public void readKey( PageCursor cursor, NumberKey into )
     {
-        into.value = Double.longBitsToDouble( cursor.getLong() );
+        into.type = cursor.getByte();
+        into.rawValueBits = cursor.getLong();
         into.entityId = cursor.getLong();
     }
 
     @Override
     public void readValue( PageCursor cursor, NumberValue into )
     {
-        into.type = cursor.getByte();
-        into.rawValueBits = cursor.getLong();
     }
 
     @Override

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberValue.java

@@ -22,62 +22,28 @@
 import org.neo4j.index.internal.gbptree.GBPTree;
 import org.neo4j.values.Value;
 
-import static org.neo4j.kernel.impl.index.schema.NumberValueConversion.toValue;
-
 /**
  * Value in a {@link GBPTree} handling numbers suitable for schema indexing.
- * Contains actual number for internal filtering after accidental query hits due to double value coersion.
+ *
+ * NOTE:  For the time being no data exists in {@link NumberValue}, but since the layout is under development
+ * it's very convenient to have this class still exist so that it's very easy to try out different types
+ * of layouts without changing the entire stack of arguments. In the end it may just be that this class
+ * will be deleted, but for now it sticks around.
  */
 class NumberValue
 {
-    static final int SIZE =
-            Byte.SIZE + /* type */
-            Long.SIZE;  /* value bits */
-
-    static final byte LONG = 0;
-    static final byte FLOAT = 1;
-    static final byte DOUBLE = 2;
+    static final int SIZE = 0;
 
-    byte type;
-    long rawValueBits;
+    static final NumberValue INSTANCE = new NumberValue();
 
     void from( Value[] values )
     {
-        extractValue( NumberValueConversion.assertValidSingleNumber( values ) );
-    }
-
-    byte type()
-    {
-        return type;
-    }
-
-    long rawValueBits()
-    {
-        return rawValueBits;
-    }
-
-    private void extractValue( Number value )
-    {
-        if ( value instanceof Double )
-        {
-            type = DOUBLE;
-            rawValueBits = Double.doubleToLongBits( (Double) value );
-        }
-        else if ( value instanceof Float )
-        {
-            type = FLOAT;
-            rawValueBits = Float.floatToIntBits( (Float) value );
-        }
-        else
-        {
-            type = LONG;
-            rawValueBits = value.longValue();
-        }
+        // not needed a.t.m.
     }
 
     @Override
     public String toString()
     {
-        return "type=" + type + ",rawValue=" + rawValueBits + ",value=" + toValue( type, rawValueBits );
+        return "[no value]";
     }
 }

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberValueConversion.java

@@ -22,9 +22,9 @@
 import org.neo4j.values.Value;
 import org.neo4j.values.Values;
 
-import static org.neo4j.kernel.impl.index.schema.NumberValue.DOUBLE;
-import static org.neo4j.kernel.impl.index.schema.NumberValue.FLOAT;
-import static org.neo4j.kernel.impl.index.schema.NumberValue.LONG;
+import static org.neo4j.kernel.impl.index.schema.NumberKey.TYPE_DOUBLE;
+import static org.neo4j.kernel.impl.index.schema.NumberKey.TYPE_FLOAT;
+import static org.neo4j.kernel.impl.index.schema.NumberKey.TYPE_LONG;
 
 /**
  * Utilities for converting number values to and from different representations.
@@ -54,11 +54,11 @@ static Number toValue( byte type, long rawValueBits )
     {
         switch ( type )
         {
-        case LONG:
+        case TYPE_LONG:
             return rawValueBits;
-        case FLOAT:
+        case TYPE_FLOAT:
             return Float.intBitsToFloat( (int)rawValueBits );
-        case DOUBLE:
+        case TYPE_DOUBLE:
             return Double.longBitsToDouble( rawValueBits );
         default:
             throw new IllegalArgumentException( "Unexpected type " + type );

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/UniqueNumberLayout.java

@@ -32,16 +32,16 @@ class UniqueNumberLayout extends NumberLayout
     public long identifier()
     {
         // todo Is Number.Value.SIZE a good checksum?
-        return Layout.namedIdentifier( IDENTIFIER_NAME, NumberValue.SIZE );
+        return Layout.namedIdentifier( IDENTIFIER_NAME, NumberKey.SIZE );
     }
 
     @Override
     public int compare( NumberKey o1, NumberKey o2 )
     {
-        int comparison = Double.compare( o1.value, o2.value );
+        int comparison = o1.compareValueTo( o2 );
         if ( comparison == 0 )
         {
-            // This is a special case where we need also compare entityId support inclusive/exclusive
+            // This is a special case where we need also compare entityId to support inclusive/exclusive
             if ( o1.entityIdIsSpecialTieBreaker || o2.entityIdIsSpecialTieBreaker )
             {
                 return Long.compare( o1.entityId, o2.entityId );

community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java

@@ -80,8 +80,6 @@ private static NumberKey key( long entityId, Object value )
 
     private static NumberValue value( Object value )
     {
-        NumberValue result = new NumberValue();
-        result.from( new Object[] {value} );
-        return result;
+        return NumberValue.INSTANCE;
     }
 }