NodeValueClientFilter can handle value tuples containing NO_VALUE

Previously it was more an all-or-nothing where either there
were values provided from the index or there wasn't.
With the native composite index there may be value tuples
which contain some NO_VALUE and some actual values.
Therefore the implementation of acceptNode has been changed
to accommodate for it.

community/community-it/index-it/src/test/java/org/neo4j/kernel/api/impl/schema/GenericIndexValidationIT.java

@@ -360,9 +360,9 @@ private enum NamedDynamicValueGenerator
         durationArray( SIZE_DURATION, 144, i -> random.randomValues().nextDurationArrayRaw( i, i ) ),
         periodArray( SIZE_DURATION, 144, i -> random.randomValues().nextPeriodArrayRaw( i, i ) ),
         cartesianPointArray( SIZE_GEOMETRY, 503, i -> random.randomValues().nextCartesianPointArray( i, i ).asObjectCopy() ),
-        cartesian3DPointArray( SIZE_GEOMETRY, 503, i -> random.randomValues().nextCartesianPointArray( i, i ).asObjectCopy() ),
+        cartesian3DPointArray( SIZE_GEOMETRY, 503, i -> random.randomValues().nextCartesian3DPointArray( i, i ).asObjectCopy() ),
         geographicPointArray( SIZE_GEOMETRY, 503, i -> random.randomValues().nextGeographicPointArray( i, i ).asObjectCopy() ),
-        geographicDPointArray( SIZE_GEOMETRY, 503, i -> random.randomValues().nextGeographic3DPointArray( i, i ).asObjectCopy() );
+        geographic3DPointArray( SIZE_GEOMETRY, 503, i -> random.randomValues().nextGeographic3DPointArray( i, i ).asObjectCopy() );
 
         private final int singleArrayEntrySize;
         private final DynamicValueGenerator generator;

community/community-it/kernel-it/src/test/java/org/neo4j/kernel/impl/newapi/NodeValueIndexCursorNativeGBPTree10Test.java

@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2002-2018 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.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.kernel.impl.newapi;
+
+import org.neo4j.graphdb.factory.GraphDatabaseSettings;
+
+public class NodeValueIndexCursorNativeGBPTree10Test extends AbstractNodeValueIndexCursorTest
+{
+    @Override
+    public ReadTestSupport newTestSupport()
+    {
+        ReadTestSupport readTestSupport = new ReadTestSupport();
+        readTestSupport.addSetting( GraphDatabaseSettings.default_schema_provider, GraphDatabaseSettings.SchemaIndex.NATIVE_BTREE10.providerIdentifier() );
+        return readTestSupport;
+    }
+    @Override
+    protected String providerKey()
+    {
+        return "native-btree";
+    }
+    @Override
+    protected String providerVersion()
+    {
+        return "1.0";
+    }
+    @Override
+    protected boolean spatialRangeSupport()
+    {
+        return true;
+    }
+
+    @Override
+    protected boolean indexProvidesStringValues()
+    {
+        return true;
+    }
+
+    @Override
+    protected boolean indexProvidesNumericValues()
+    {
+        return true;
+    }
+}

community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeValueIndexCursorTestBase.java

@@ -772,7 +772,10 @@ private void assertFoundNodesAndNoValue( NodeValueIndexCursor node, int nodes, M
             long nodeReference = node.nodeReference();
             assertTrue( "all nodes are unique", uniqueIds.add( nodeReference ) );
 
-            assertFalse( node.hasValue() );
+            // We can't quite assert !node.hasValue() because even tho pure SpatialIndexReader is guaranteed to not return any values,
+            // where null could be used, the generic native index, especially when having composite keys including spatial values it's
+            // more of a gray area and some keys may be spatial, some not and therefore a proper Value[] will be extracted
+            // potentially containing some NO_VALUE values.
         }
 
         assertFalse( "no more than " + nodes + " nodes", node.next() );

community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilter.java

@@ -19,9 +19,6 @@
  */
 package org.neo4j.kernel.impl.newapi;
 
-import java.util.Arrays;
-import java.util.Comparator;
-
 import org.neo4j.internal.kernel.api.IndexQuery;
 import org.neo4j.internal.kernel.api.NodeCursor;
 import org.neo4j.internal.kernel.api.PropertyCursor;
@@ -31,6 +28,8 @@
 import org.neo4j.storageengine.api.schema.IndexProgressor.NodeValueClient;
 import org.neo4j.values.storable.Value;
 
+import static org.neo4j.values.storable.Values.NO_VALUE;
+
 /**
  * This class filters acceptNode() calls from an index progressor, to assert that exact entries returned from the
  * progressor really match the exact property values. See also org.neo4j.kernel.impl.api.LookupFilter.
@@ -70,13 +69,11 @@
  */
 class NodeValueClientFilter implements NodeValueClient, IndexProgressor
 {
-    private static final Comparator<IndexQuery> ASCENDING_BY_KEY = Comparator.comparingInt( IndexQuery::propertyKeyId );
     private final NodeValueClient target;
     private final NodeCursor node;
     private final PropertyCursor property;
     private final IndexQuery[] filters;
     private final org.neo4j.internal.kernel.api.Read read;
-    private int[] keys;
     private IndexProgressor progressor;
 
     NodeValueClientFilter(
@@ -88,14 +85,12 @@ class NodeValueClientFilter implements NodeValueClient, IndexProgressor
         this.property = property;
         this.filters = filters;
         this.read = read;
-        Arrays.sort( filters, ASCENDING_BY_KEY );
     }
 
     @Override
     public void initialize( IndexDescriptor descriptor, IndexProgressor progressor, IndexQuery[] query, boolean needsValues )
     {
         this.progressor = progressor;
-        this.keys = descriptor.schema().getPropertyIds();
         // We set needsValues to true for the target, since this will enable us to execute the cheaper filterByIndexValues
         // instead of filterByCursors if the target can provide values
         target.initialize( descriptor, this, query, true );
@@ -104,24 +99,77 @@ public void initialize( IndexDescriptor descriptor, IndexProgressor progressor,
     @Override
     public boolean acceptNode( long reference, Value[] values )
     {
-        if ( keys != null && values != null )
+        // First filter on these values, which come from the index. Some values will be NO_VALUE, because some indexed values cannot be read back.
+        // Those values will have to be read from the store using the propertyCursor and is done in one pass after this loop, if needed.
+        int storeLookups = 0;
+        if ( values == null )
         {
-            return filterByIndexValues( reference, values );
+            // values == null effectively means that all values are NO_VALUE so we certainly need the store lookup here
+            for ( IndexQuery filter : filters )
+            {
+                if ( filter != null )
+                {
+                    storeLookups++;
+                }
+            }
         }
         else
         {
-            read.singleNode( reference, node );
-            if ( node.next() )
+            for ( int i = 0; i < filters.length; i++ )
             {
-                node.properties( property );
+                IndexQuery filter = filters[i];
+                if ( filter != null )
+                {
+                    if ( values[i] == NO_VALUE )
+                    {
+                        storeLookups++;
+                    }
+                    else if ( !filter.acceptsValue( values[i] ) )
+                    {
+                        return false;
+                    }
+                }
             }
-            else
+        }
+
+        // If there were one or more NO_VALUE values above then open store cursor and read those values from the store,
+        // applying the same filtering as above, but with a loop designed to do only a single pass over the store values,
+        // because it's the most expensive part.
+        if ( storeLookups > 0 && !acceptByStoreFiltering( reference, storeLookups, values ) )
+        {
+            return false;
+        }
+        return target.acceptNode( reference, values );
+    }
+
+    private boolean acceptByStoreFiltering( long reference, int storeLookups, Value[] values )
+    {
+        // Initialize the property cursor scan
+        read.singleNode( reference, node );
+        if ( !node.next() )
+        {
+            // This node doesn't exist, therefore it cannot be accepted
+            property.close();
+            return false;
+        }
+        node.properties( property );
+
+        while ( storeLookups > 0 && property.next() )
+        {
+            for ( int i = 0; i < filters.length; i++ )
             {
-                property.close();
-                return false;
+                IndexQuery filter = filters[i];
+                if ( filter != null && (values == null || values[i] == NO_VALUE) && property.propertyKey() == filter.propertyKeyId() )
+                {
+                    if ( !filter.acceptsValueAt( property ) )
+                    {
+                        return false;
+                    }
+                    storeLookups--;
+                }
             }
-            return filterByCursors( reference, values );
         }
+        return storeLookups == 0;
     }
 
     @Override
@@ -143,54 +191,4 @@ public void close()
         property.close();
         progressor.close();
     }
-
-    private boolean filterByIndexValues( long reference, Value[] values )
-    {
-        FILTERS:
-        for ( IndexQuery filter : filters )
-        {
-            for ( int i = 0; i < keys.length; i++ )
-            {
-                if ( keys[i] == filter.propertyKeyId() )
-                {
-                    if ( !filter.acceptsValue( values[i] ) )
-                    {
-                        return false;
-                    }
-                    continue FILTERS;
-                }
-            }
-            assert false : "Cannot satisfy filter " + filter + " - no corresponding key!";
-            return false;
-        }
-        return target.acceptNode( reference, values );
-    }
-
-    private boolean filterByCursors( long reference, Value[] values )
-    {
-        // note that this way of checking if all filters are matched relies on the node not having duplicate properties
-        int accepted = 0;
-        PROPERTIES:
-        while ( property.next() )
-        {
-            for ( IndexQuery filter : filters )
-            {
-                if ( filter.propertyKeyId() == property.propertyKey() )
-                {
-                    if ( !filter.acceptsValueAt( property ) )
-                    {
-                        return false;
-                    }
-                    accepted++;
-                }
-                else if ( property.propertyKey() < filter.propertyKeyId() )
-                {
-                    continue PROPERTIES;
-                }
-            }
-        }
-        // if not all filters were matched, i.e. accepted < filters.length we reject
-        // otherwise we delegate to target
-        return accepted >= filters.length && target.acceptNode( reference, values );
-    }
 }

community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java

@@ -19,8 +19,6 @@
  */
 package org.neo4j.kernel.impl.newapi;
 
-import java.util.Arrays;
-
 import org.neo4j.internal.kernel.api.IndexOrder;
 import org.neo4j.internal.kernel.api.IndexQuery;
 import org.neo4j.internal.kernel.api.IndexReference;
@@ -107,16 +105,18 @@ private IndexProgressor.NodeValueClient withFullValuePrecision( DefaultNodeValue
         if ( !reader.hasFullValuePrecision( query ) )
         {
             IndexQuery[] filters = new IndexQuery[query.length];
-            int j = 0;
-            for ( IndexQuery q : query )
+            int count = 0;
+            for ( int i = 0; i < query.length; i++ )
             {
+                IndexQuery q = query[i];
                 switch ( q.type() )
                 {
                 case range:
                     ValueGroup valueGroup = q.valueGroup();
                     if ( ( valueGroup == NUMBER || valueGroup == GEOMETRY) && !reader.hasFullValuePrecision( q ) )
                     {
-                        filters[j++] = q;
+                        filters[i] = q;
+                        count++;
                     }
                     break;
                 case exact:
@@ -125,17 +125,19 @@ private IndexProgressor.NodeValueClient withFullValuePrecision( DefaultNodeValue
                     {
                         if ( !reader.hasFullValuePrecision( q ) )
                         {
-                            filters[j++] = q;
+                            filters[i] = q;
+                            count++;
                         }
                     }
                     break;
                 default:
                     break;
                 }
             }
-            if ( j > 0 )
+            if ( count > 0 )
             {
-                filters = Arrays.copyOf( filters, j );
+                // filters[] can contain null elements. The non-null elements are the filters and each sit in the designated slot
+                // matching the values from the index.
                 target = new NodeValueClientFilter( target, cursors.allocateNodeCursor(),
                         cursors.allocatePropertyCursor(), this, filters );
             }