diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexReader.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexReader.java index a2e208051fecb..66c810144908a 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexReader.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexReader.java @@ -210,19 +210,19 @@ public boolean hasFullNumberPrecision( IndexQuery... predicates ) return true; } - private void startSeekForInitializedRange( IndexProgressor.NodeValueClient cursor, KEY treeKeyFrom, KEY treeKeyTo ) + private void startSeekForInitializedRange( IndexProgressor.NodeValueClient client, KEY treeKeyFrom, KEY treeKeyTo ) { if ( layout.compare( treeKeyFrom, treeKeyTo ) > 0 ) { - cursor.initialize( IndexProgressor.EMPTY, propertyKeys ); + client.initialize( IndexProgressor.EMPTY, propertyKeys ); return; } try { RawCursor,IOException> seeker = tree.seek( treeKeyFrom, treeKeyTo ); openSeekers.add( seeker ); - IndexProgressor hitProgressor = new NumberHitIndexCursorProgressor<>( seeker, cursor, openSeekers ); - cursor.initialize( hitProgressor, propertyKeys ); + IndexProgressor hitProgressor = new NumberHitIndexCursorProgressor<>( seeker, client, openSeekers ); + client.initialize( hitProgressor, propertyKeys ); } catch ( IOException e ) { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NodeValueIterator.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NodeValueIterator.java index 49b94f674fc3e..c3a912a0e8a7f 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NodeValueIterator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NodeValueIterator.java @@ -21,15 +21,17 @@ import org.neo4j.collection.primitive.PrimitiveLongCollections; import org.neo4j.collection.primitive.PrimitiveLongIterator; +import org.neo4j.graphdb.Resource; import org.neo4j.storageengine.api.schema.IndexProgressor; import org.neo4j.values.storable.Value; /** * A {@link IndexProgressor} + {@link IndexProgressor.NodeValueClient} combo presented as a {@link PrimitiveLongIterator}. */ -public class NodeValueIterator extends PrimitiveLongCollections.PrimitiveLongBaseIterator implements IndexProgressor.NodeValueClient +public class NodeValueIterator extends PrimitiveLongCollections.PrimitiveLongBaseIterator + implements IndexProgressor.NodeValueClient, Resource { - private boolean closed; + private volatile boolean closed; private IndexProgressor progressor; @Override @@ -39,7 +41,7 @@ protected boolean fetchNext() // and feed result into this with node( long reference, Value... values ) if ( closed || !progressor.next() ) { - done(); + close(); return false; } return true; @@ -58,12 +60,12 @@ public boolean acceptNode( long reference, Value... values ) } @Override - public void done() + public void close() { if ( !closed ) { - progressor.close(); closed = true; + progressor.close(); } } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberHitIndexCursorProgressor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberHitIndexCursorProgressor.java index 4d34e7ad5ca85..374a5baf07fcb 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberHitIndexCursorProgressor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NumberHitIndexCursorProgressor.java @@ -30,15 +30,15 @@ public class NumberHitIndexCursorProgressor implements IndexProgressor { private final RawCursor,IOException> seeker; - private final NodeValueClient cursor; + private final NodeValueClient client; private final Collection,IOException>> toRemoveFromOnClose; private boolean closed; - NumberHitIndexCursorProgressor( RawCursor,IOException> seeker, NodeValueClient cursor, + NumberHitIndexCursorProgressor( RawCursor,IOException> seeker, NodeValueClient client, Collection,IOException>> toRemoveFromOnClose ) { this.seeker = seeker; - this.cursor = cursor; + this.client = client; this.toRemoveFromOnClose = toRemoveFromOnClose; } @@ -50,7 +50,7 @@ public boolean next() if ( seeker.next() ) { KEY key = seeker.get().key(); - cursor.acceptNode( key.entityId, key.asValue() ); + client.acceptNode( key.entityId, key.asValue() ); return true; } return false; diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/FusionIndexReader.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/FusionIndexReader.java index ba4a0e46c0764..f8da35f72030f 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/FusionIndexReader.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/fusion/FusionIndexReader.java @@ -132,7 +132,7 @@ public void query( IndexProgressor.NodeValueClient cursor, IndexOrder indexOrder // todo: There will be no ordering of the node ids here. Is this a problem? if ( predicates[0] instanceof ExistsPredicate ) { - MultiProgressorNodeValueCursor multiProgressor = new MultiProgressorNodeValueCursor( cursor, propertyKeys ); + BridgingIndexProgressor multiProgressor = new BridgingIndexProgressor( cursor, propertyKeys ); cursor.initialize( multiProgressor, propertyKeys ); nativeReader.query( multiProgressor, indexOrder, predicates[0] ); luceneReader.query( multiProgressor, indexOrder, predicates[0] ); @@ -161,16 +161,19 @@ public boolean hasFullNumberPrecision( IndexQuery... predicates ) return predicates[0] instanceof NumberRangePredicate && nativeReader.hasFullNumberPrecision( predicates ); } - private class MultiProgressorNodeValueCursor implements IndexProgressor.NodeValueClient, IndexProgressor + /** + * Combine multiple progressor to act like one single logical progressor seen from clients perspective. + */ + private class BridgingIndexProgressor implements IndexProgressor.NodeValueClient, IndexProgressor { - private final NodeValueClient cursor; + private final NodeValueClient client; private final int[] keys; private final Queue progressors; private IndexProgressor current; - MultiProgressorNodeValueCursor( NodeValueClient cursor, int[] keys ) + BridgingIndexProgressor( NodeValueClient client, int[] keys ) { - this.cursor = cursor; + this.client = client; this.keys = keys; progressors = new ArrayDeque<>(); } @@ -190,6 +193,7 @@ public boolean next() } else { + current.close(); current = progressors.poll(); } } @@ -223,13 +227,7 @@ private void assertKeysAlign( int[] keys ) @Override public boolean acceptNode( long reference, Value[] values ) { - return cursor.acceptNode( reference, values ); - } - - @Override - public void done() - { - cursor.done(); + return client.acceptNode( reference, values ); } } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/IndexCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/IndexCursor.java index 177c54cc63cdf..047012ee2d7f0 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/IndexCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/IndexCursor.java @@ -30,11 +30,6 @@ final void initialize( IndexProgressor progressor ) this.progressor = progressor; } - public final void done() - { - close(); - } - public final boolean next() { return progressor != null && progressor.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 63c83e37fbb2c..c472d74c14bf3 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 @@ -24,23 +24,58 @@ import org.neo4j.internal.kernel.api.IndexQuery; import org.neo4j.storageengine.api.schema.IndexProgressor; +import org.neo4j.storageengine.api.schema.IndexProgressor.NodeValueClient; import org.neo4j.values.storable.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. + * + * It works by acting as a man-in-the-middle between outer {@link NodeValueClient client} and inner {@link IndexProgressor}. + * Interaction goes like: + * + * Initialize: + * 

+ * client
+ *      -- query( client ) ->      filter = new filter(client)
+ *                                 filter -- query( filter ) ->        progressor
+ *                                 filter <- initialize(progressor) -- progressor
+ * client <- initialize(filter) -- filter
+ *
+ * + * Progress: + * 

+ * client -- next() ->       filter
+ *                           filter -- next() ->          progressor
+ *                                     <- acceptNode() --
+ *                                  -- :false ->
+ *                                     <- acceptNode() --
+ *                                  -- :false ->
+ *                           filter    <- acceptNode() --
+ * client <- acceptNode() -- filter
+ *        -- :true ->        filter -- :true ->           progressor
+ * client <----------------------------------------------
+ *
+ * + * Close: + * 

+ * client -- close() -> filter
+ *                      filter -- close() -> progressor
+ * client <---------------------------------
+ *
*/ -class NodeValueClientFilter implements IndexProgressor.NodeValueClient +class NodeValueClientFilter implements NodeValueClient, IndexProgressor { private static final Comparator ASCENDING_BY_KEY = Comparator.comparingInt( IndexQuery::propertyKeyId ); - private final IndexProgressor.NodeValueClient target; + private final NodeValueClient target; private final NodeCursor node; private final PropertyCursor property; private final IndexQuery[] filters; private int[] keys; + private IndexProgressor progressor; NodeValueClientFilter( - IndexProgressor.NodeValueClient target, + NodeValueClient target, NodeCursor node, PropertyCursor property, IndexQuery... filters ) { this.target = target; @@ -53,16 +88,9 @@ class NodeValueClientFilter implements IndexProgressor.NodeValueClient @Override public void initialize( IndexProgressor progressor, int[] propertyIds ) { + this.progressor = progressor; this.keys = propertyIds; - target.initialize( progressor, propertyIds ); - } - - @Override - public void done() - { - node.close(); - property.close(); - target.done(); + target.initialize( this, propertyIds ); } @Override @@ -88,6 +116,20 @@ public boolean acceptNode( long reference, Value[] values ) } } + @Override + public boolean next() + { + return progressor.next(); + } + + @Override + public void close() + { + node.close(); + property.close(); + progressor.close(); + } + private boolean filterByIndexValues( long reference, Value[] values ) { FILTERS: diff --git a/community/kernel/src/main/java/org/neo4j/storageengine/api/schema/IndexProgressor.java b/community/kernel/src/main/java/org/neo4j/storageengine/api/schema/IndexProgressor.java index 01195532019c6..c3c5df5a65df7 100644 --- a/community/kernel/src/main/java/org/neo4j/storageengine/api/schema/IndexProgressor.java +++ b/community/kernel/src/main/java/org/neo4j/storageengine/api/schema/IndexProgressor.java @@ -82,11 +82,6 @@ interface NodeValueClient * @return true if the entry is accepted, false otherwise */ boolean acceptNode( long reference, Value... values ); - - /** - * Called by progressor so signal that there are no more entries. - */ - void done(); } /** @@ -109,11 +104,6 @@ interface NodeLabelClient * @return true if the entry is accepted, false otherwise */ boolean acceptNode( long reference, LabelSet labels ); - - /** - * Called by progressor so signal that there are no more entries. - */ - void done(); } /** @@ -135,11 +125,6 @@ interface ExplicitClient * @return true if the entry is accepted, false otherwise */ boolean acceptEntity( long reference, float score ); - - /** - * Called by progressor so signal that there are no more entries. - */ - void done(); } IndexProgressor EMPTY = new IndexProgressor() diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexAccessorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexAccessorTest.java index 251f7893eabf9..0e9ccea810e57 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexAccessorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexAccessorTest.java @@ -22,7 +22,9 @@ import org.hamcrest.CoreMatchers; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.io.File; import java.io.IOException; @@ -61,9 +63,7 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import static org.neo4j.collection.primitive.PrimitiveLongCollections.EMPTY_LONG_ARRAY; import static org.neo4j.function.Predicates.all; import static org.neo4j.function.Predicates.alwaysTrue; @@ -90,6 +90,9 @@ public abstract class NativeSchemaNumberIndexAccessorTest accessor; + @Rule + public ExpectedException expected = ExpectedException.none(); + @Before public void setupAccessor() throws IOException { @@ -131,16 +134,11 @@ public void processMustThrowAfterClose() throws Exception IndexUpdater updater = accessor.newUpdater( ONLINE ); updater.close(); + // then + expected.expect( IllegalStateException.class ); + // when - try - { - updater.process( simpleUpdate() ); - fail( "Should have failed" ); - } - catch ( IllegalStateException e ) - { - // then good - } + updater.process( simpleUpdate() ); } @Test @@ -433,21 +431,15 @@ public void throwForUnsupportedIndexOrder() throws Exception IndexOrder unsupportedOrder = IndexOrder.DESCENDING; IndexQuery.ExactPredicate unsupportedQuery = IndexQuery.exact( 0, "Legolas" ); - try - { - // when - indexReader.query( new SimpleNodeValueClient(), unsupportedOrder, unsupportedQuery ); - fail( "Should have failed" ); - } - catch ( UnsupportedOperationException e ) - { - // then - assertThat( e.getMessage(), CoreMatchers.allOf( - CoreMatchers.containsString( "unsupported order" ), - CoreMatchers.containsString( unsupportedOrder.toString() ), - CoreMatchers.containsString( unsupportedQuery.toString() ) ) - ); - } + // then + expected.expect( UnsupportedOperationException.class ); + expected.expectMessage( CoreMatchers.allOf( + CoreMatchers.containsString( "unsupported order" ), + CoreMatchers.containsString( unsupportedOrder.toString() ), + CoreMatchers.containsString( unsupportedQuery.toString() ) ) ); + + // when + indexReader.query( new SimpleNodeValueClient(), unsupportedOrder, unsupportedQuery ); } @Test @@ -613,16 +605,11 @@ public void requestForSecondUpdaterMustThrow() throws Exception // given try ( IndexUpdater ignored = accessor.newUpdater( ONLINE ) ) { + // then + expected.expect( IllegalStateException.class ); + // when - try - { - accessor.newUpdater( ONLINE ); - fail( "Should have failed" ); - } - catch ( IllegalStateException e ) - { - // then good - } + accessor.newUpdater( ONLINE ); } } @@ -707,16 +694,11 @@ public void readingAfterDropShouldThrow() throws Exception // given accessor.drop(); - try - { - // when - accessor.newReader(); - fail( "Should have failed" ); - } - catch ( IllegalStateException e ) - { - // then good - } + // then + expected.expect( IllegalStateException.class ); + + // when + accessor.newReader(); } @Test @@ -725,16 +707,11 @@ public void writingAfterDropShouldThrow() throws Exception // given accessor.drop(); - try - { - // when - accessor.newUpdater( IndexUpdateMode.ONLINE ); - fail( "Should have failed" ); - } - catch ( IllegalStateException e ) - { - // then good - } + // then + expected.expect( IllegalStateException.class ); + + // when + accessor.newUpdater( IndexUpdateMode.ONLINE ); } @Test @@ -743,16 +720,11 @@ public void readingAfterCloseShouldThrow() throws Exception // given accessor.close(); - try - { - // when - accessor.newReader(); - fail( "Should have failed" ); - } - catch ( IllegalStateException e ) - { - // then good - } + // then + expected.expect( IllegalStateException.class ); + + // when + accessor.newReader(); } @Test @@ -761,16 +733,11 @@ public void writingAfterCloseShouldThrow() throws Exception // given accessor.close(); - try - { - // when - accessor.newUpdater( IndexUpdateMode.ONLINE ); - fail( "Should have failed" ); - } - catch ( IllegalStateException e ) - { - // then good - } + // then + expected.expect( IllegalStateException.class ); + + // when + accessor.newUpdater( IndexUpdateMode.ONLINE ); } @Test diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/IndexCursorFilterTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/IndexCursorFilterTest.java index 8a27137f70ec6..c64f32be5f837 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/IndexCursorFilterTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/IndexCursorFilterTest.java @@ -19,17 +19,13 @@ */ 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.List; -import org.apache.commons.lang3.builder.EqualsBuilder; -import org.apache.commons.lang3.builder.HashCodeBuilder; -import org.apache.commons.lang3.builder.ToStringBuilder; -import org.apache.commons.lang3.builder.ToStringStyle; -import org.junit.Rule; -import org.junit.Test; - import org.neo4j.internal.kernel.api.IndexQuery; import org.neo4j.storageengine.api.schema.IndexProgressor; import org.neo4j.storageengine.api.schema.IndexProgressor.NodeValueClient; @@ -56,11 +52,12 @@ public void shouldAcceptAllNodesOnNoFilters() throws Exception NodeValueClientFilter filter = initializeFilter( null ); // when + filter.next(); assertTrue( filter.acceptNode( 17, null ) ); - filter.done(); + filter.close(); // then - assertEvents( initialize(), new Event.Node( 17, null ), Event.DONE ); + assertEvents( initialize(), Event.NEXT, new Event.Node( 17, null ), Event.CLOSE ); } @Test @@ -70,11 +67,12 @@ public void shouldRejectNodeNotInUse() throws Exception NodeValueClientFilter filter = initializeFilter( null, IndexQuery.exists( 12 ) ); // when + filter.next(); assertFalse( filter.acceptNode( 17, null ) ); - filter.done(); + filter.close(); // then - assertEvents( initialize(), Event.DONE ); + assertEvents( initialize(), Event.NEXT, Event.CLOSE ); } @Test @@ -85,11 +83,12 @@ public void shouldRejectNodeWithNoProperties() throws Exception NodeValueClientFilter filter = initializeFilter( null, IndexQuery.exists( 12 ) ); // when + filter.next(); assertFalse( filter.acceptNode( 17, null ) ); - filter.done(); + filter.close(); // then - assertEvents( initialize(), Event.DONE ); + assertEvents( initialize(), Event.NEXT, Event.CLOSE ); } @Test @@ -101,11 +100,12 @@ public void shouldAcceptNodeWithMatchingProperty() throws Exception NodeValueClientFilter filter = initializeFilter( null, IndexQuery.exists( 12 ) ); // when + filter.next(); assertTrue( filter.acceptNode( 17, null ) ); - filter.done(); + filter.close(); // then - assertEvents( initialize(), new Event.Node( 17, null ), Event.DONE ); + assertEvents( initialize(), Event.NEXT, new Event.Node( 17, null ), Event.CLOSE ); } @Test @@ -117,11 +117,12 @@ public void shouldNotAcceptNodeWithoutMatchingProperty() throws Exception NodeValueClientFilter filter = initializeFilter( null, IndexQuery.exists( 12 ) ); // when + filter.next(); assertFalse( filter.acceptNode( 17, null ) ); - filter.done(); + filter.close(); // then - assertEvents( initialize(), Event.DONE ); + assertEvents( initialize(), Event.NEXT, Event.CLOSE ); } private NodeValueClientFilter initializeFilter( int[] keys, IndexQuery... filters ) @@ -148,12 +149,6 @@ public void initialize( IndexProgressor progressor, int[] propertyIds ) events.add( new Event.Initialize( progressor, propertyIds ) ); } - @Override - public void done() - { - events.add( Event.DONE ); - } - @Override public boolean acceptNode( long reference, Value[] values ) { @@ -164,13 +159,14 @@ public boolean acceptNode( long reference, Value[] values ) @Override public boolean next() { - throw new UnsupportedOperationException( "should not be called in these tests" ); + events.add( Event.NEXT ); + return true; } @Override public void close() { - throw new UnsupportedOperationException( "should not be called in these tests" ); + events.add( Event.CLOSE ); } private abstract static class Event @@ -185,14 +181,29 @@ static class Initialize extends Event this.progressor = progressor; this.keys = keys; } + + @Override + public String toString() + { + return "INITIALIZE(" + Arrays.toString( keys )+ ")"; + } } - static final Event DONE = new Event() + static final Event CLOSE = new Event() { @Override public String toString() { - return "DONE"; + return "CLOSE"; + } + }; + + static final Event NEXT = new Event() + { + @Override + public String toString() + { + return "NEXT"; } }; @@ -203,29 +214,27 @@ static class Node extends Event Node( long reference, Value[] values ) { - this.reference = reference; this.values = values; } + + @Override + public String toString() + { + return "Node(" + reference + "," + values + ")"; + } } - @SuppressWarnings( "EqualsWhichDoesntCheckParameterClass" ) @Override public final boolean equals( Object other ) { - return EqualsBuilder.reflectionEquals( this, other, true ); + return toString().equals( other.toString() ); } @Override public final int hashCode() { - return HashCodeBuilder.reflectionHashCode( this, false ); - } - - @Override - public String toString() - { - return ToStringBuilder.reflectionToString( this, ToStringStyle.SHORT_PREFIX_STYLE ); + return toString().hashCode(); } } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/SimpleNodeValueClient.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/SimpleNodeValueClient.java index 233a646375366..4dc6c182039e5 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/SimpleNodeValueClient.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/SimpleNodeValueClient.java @@ -30,7 +30,12 @@ public class SimpleNodeValueClient implements IndexProgressor.NodeValueClient public boolean next() { - return progressor.next(); + if ( progressor.next() ) + { + return true; + } + closeProgressor(); + return false; } @Override @@ -47,11 +52,12 @@ public boolean acceptNode( long reference, Value... values ) return true; } - @Override - public void done() + private void closeProgressor() { - reference = 0; - values = null; - progressor = null; + if ( progressor != null ) + { + progressor.close(); + progressor = null; + } } }