diff --git a/community/community-it/index-it/src/test/java/org/neo4j/kernel/api/impl/schema/GenericIndexValidationIT.java b/community/community-it/index-it/src/test/java/org/neo4j/kernel/api/impl/schema/GenericIndexValidationIT.java
index a367af21689aa..9ad9a63f1f0ef 100644
--- a/community/community-it/index-it/src/test/java/org/neo4j/kernel/api/impl/schema/GenericIndexValidationIT.java
+++ b/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;
diff --git a/community/community-it/kernel-it/src/test/java/org/neo4j/kernel/impl/newapi/NodeValueIndexCursorNativeGBPTree10Test.java b/community/community-it/kernel-it/src/test/java/org/neo4j/kernel/impl/newapi/NodeValueIndexCursorNativeGBPTree10Test.java
new file mode 100644
index 0000000000000..aa95b5f26c4ce
--- /dev/null
+++ b/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 .
+ */
+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;
+ }
+}
diff --git a/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeValueIndexCursorTestBase.java b/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeValueIndexCursorTestBase.java
index 0a487efb68c49..e51512bfdf373 100644
--- a/community/kernel-api/src/test/java/org/neo4j/internal/kernel/api/NodeValueIndexCursorTestBase.java
+++ b/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() );
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilter.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilter.java
index 11715442c688b..9957f77f7dca8 100644
--- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilter.java
+++ b/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 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 );
- }
}
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java
index be21088a0bc21..abb8e52ef77f6 100644
--- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java
+++ b/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,7 +125,8 @@ private IndexProgressor.NodeValueClient withFullValuePrecision( DefaultNodeValue
{
if ( !reader.hasFullValuePrecision( q ) )
{
- filters[j++] = q;
+ filters[i] = q;
+ count++;
}
}
break;
@@ -133,9 +134,10 @@ private IndexProgressor.NodeValueClient withFullValuePrecision( DefaultNodeValue
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 );
}
diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilterTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilterTest.java
index 085f2dc522d3d..61121605a850c 100644
--- a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilterTest.java
+++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/NodeValueClientFilterTest.java
@@ -19,11 +19,15 @@
*/
package org.neo4j.kernel.impl.newapi;
+import org.junit.Rule;
import org.junit.Test;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
import org.neo4j.internal.kernel.api.IndexQuery;
import org.neo4j.internal.kernel.api.Read;
@@ -33,6 +37,7 @@
import org.neo4j.storageengine.api.schema.IndexDescriptor;
import org.neo4j.storageengine.api.schema.IndexProgressor;
import org.neo4j.storageengine.api.schema.IndexProgressor.NodeValueClient;
+import org.neo4j.test.rule.RandomRule;
import org.neo4j.values.storable.Value;
import static org.junit.Assert.assertEquals;
@@ -40,10 +45,14 @@
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import static org.neo4j.helpers.collection.MapUtil.genericMap;
+import static org.neo4j.values.storable.Values.NO_VALUE;
import static org.neo4j.values.storable.Values.stringValue;
public class NodeValueClientFilterTest implements IndexProgressor, NodeValueClient
{
+ @Rule
+ public final RandomRule random = new RandomRule();
+
private final Read read = mock( Read.class );
private final List events = new ArrayList<>();
private final StubNodeCursor node = new StubNodeCursor();
@@ -128,11 +137,57 @@ public void shouldNotAcceptNodeWithoutMatchingProperty()
assertEvents( initialize(), Event.NEXT, Event.CLOSE );
}
+ @Test
+ public void shouldConsultProvidedFiltersForMixOfValuesAndNoValues()
+ {
+ shouldConsultProvidedFilters( Function.identity() );
+ }
+
+ @Test
+ public void shouldConsultProvidedFiltersForNullValues()
+ {
+ shouldConsultProvidedFilters( v -> null );
+ }
+
+ private void shouldConsultProvidedFilters( Function filterValues )
+ {
+ // given
+ long nodeReference = 123;
+ int labelId = 10;
+ int slots = random.nextInt( 3, 8 );
+ IndexQuery[] filters = new IndexQuery[slots];
+ Value[] actualValues = new Value[slots];
+ Value[] values = new Value[slots];
+ Map properties = new HashMap<>();
+ int[] propertyKeyIds = new int[slots];
+ for ( int i = 0; i < slots; i++ )
+ {
+ actualValues[i] = random.nextValue();
+ int propertyKeyId = i;
+ propertyKeyIds[i] = propertyKeyId;
+ if ( random.nextBoolean() )
+ {
+ filters[i] = IndexQuery.exact( propertyKeyId, actualValues[i].asObjectCopy() );
+ }
+ values[i] = random.nextBoolean() ? NO_VALUE : actualValues[i];
+ properties.put( propertyKeyId, actualValues[i] );
+ }
+ node.withNode( nodeReference, new long[]{labelId}, properties );
+
+ // when
+ NodeValueClientFilter filter = new NodeValueClientFilter( this, node, property, read, filters );
+ filter.initialize( TestIndexDescriptorFactory.forLabel( labelId, propertyKeyIds ), this, null, true );
+ boolean accepted = filter.acceptNode( nodeReference, filterValues.apply( values ) );
+
+ // then
+ assertTrue( accepted );
+ }
+
private NodeValueClientFilter initializeFilter( IndexQuery... filters )
{
NodeValueClientFilter filter = new NodeValueClientFilter(
this, node, property, read, filters );
- filter.initialize( TestIndexDescriptorFactory.forLabel( 11), this, null, true );
+ filter.initialize( TestIndexDescriptorFactory.forLabel( 11 ), this, null, true );
return filter;
}