Skip to content

Commit

Permalink
Improvements from review
Browse files Browse the repository at this point in the history
 - Misc javadoc comment clarifications
 - some refactoring of composite findNodes
  • Loading branch information
fickludd committed Mar 19, 2018
1 parent 63f29f4 commit aa9b98b
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 46 deletions.
Expand Up @@ -137,8 +137,7 @@ public interface GraphDatabaseService
/** /**
* Returns all nodes having the label, and the wanted property values. * Returns all nodes having the label, and the wanted property values.
* If an online index is found, it will be used to look up the requested * If an online index is found, it will be used to look up the requested
* nodes. The specified properties do not have to be provided in the same order * nodes.
* as they were defined in the index.
* <p> * <p>
* If no indexes exist for the label with all provided properties, the database will * If no indexes exist for the label with all provided properties, the database will
* scan all labeled nodes looking for matching nodes. * scan all labeled nodes looking for matching nodes.
Expand All @@ -162,14 +161,13 @@ public interface GraphDatabaseService
*/ */
default ResourceIterator<Node> findNodes( Label label, String key1, Object value1, String key2, Object value2 ) default ResourceIterator<Node> findNodes( Label label, String key1, Object value1, String key2, Object value2 )
{ {
throw new UnsupportedOperationException( "Composite findNodes is not supported by this GraphDatabaseService" ); throw new UnsupportedOperationException( "findNodes by multiple property names and values is not supported." );
} }


/** /**
* Returns all nodes having the label, and the wanted property values. * Returns all nodes having the label, and the wanted property values.
* If an online index is found, it will be used to look up the requested * If an online index is found, it will be used to look up the requested
* nodes. The specified properties do not have to be provided in the same order * nodes.
* as they were defined in the index.
* <p> * <p>
* If no indexes exist for the label with all provided properties, the database will * If no indexes exist for the label with all provided properties, the database will
* scan all labeled nodes looking for matching nodes. * scan all labeled nodes looking for matching nodes.
Expand Down Expand Up @@ -198,14 +196,13 @@ default ResourceIterator<Node> findNodes( Label label,
String key2, Object value2, String key2, Object value2,
String key3, Object value3 ) String key3, Object value3 )
{ {
throw new UnsupportedOperationException( "Composite findNodes is not supported by this GraphDatabaseService" ); throw new UnsupportedOperationException( "findNodes by multiple property names and values is not supported." );
} }


/** /**
* Returns all nodes having the label, and the wanted property values. * Returns all nodes having the label, and the wanted property values.
* If an online index is found, it will be used to look up the requested * If an online index is found, it will be used to look up the requested
* nodes. The specified properties do not have to be provided in the same order * nodes.
* as they were defined in the index.
* <p> * <p>
* If no indexes exist for the label with all provided properties, the database will * If no indexes exist for the label with all provided properties, the database will
* scan all labeled nodes looking for matching nodes. * scan all labeled nodes looking for matching nodes.
Expand All @@ -226,7 +223,7 @@ default ResourceIterator<Node> findNodes( Label label,
*/ */
default ResourceIterator<Node> findNodes( Label label, Map<String, Object> propertyValues ) default ResourceIterator<Node> findNodes( Label label, Map<String, Object> propertyValues )
{ {
throw new UnsupportedOperationException( "Composite findNodes is not supported by this GraphDatabaseService" ); throw new UnsupportedOperationException( "findNodes by multiple property names and values is not supported." );
} }


/** /**
Expand Down Expand Up @@ -258,7 +255,7 @@ default ResourceIterator<Node> findNodes( Label label, Map<String, Object> prope
*/ */
default ResourceIterator<Node> findNodes( Label label, String key, String template, StringSearchMode searchMode ) default ResourceIterator<Node> findNodes( Label label, String key, String template, StringSearchMode searchMode )
{ {
throw new UnsupportedOperationException( "Specialized string queries are not supported by this GraphDatabaseService" ); throw new UnsupportedOperationException( "Specialized string queries are not supported" );
} }


/** /**
Expand Down
Expand Up @@ -44,7 +44,7 @@ public enum StringSearchMode
*/ */
SUFFIX, SUFFIX,
/** /**
* The value must contain the template. Only exact matches are supported. * The value must contain the template exactly. Regular expressions are not supported.
*/ */
CONTAINS; CONTAINS;
} }
Expand Up @@ -1018,7 +1018,8 @@ public PrimitiveLongReadableDiffSets indexUpdatesForScan( SchemaIndexDescriptor
@Override @Override
public PrimitiveLongReadableDiffSets indexUpdatesForSuffixOrContains( SchemaIndexDescriptor descriptor, IndexQuery query ) public PrimitiveLongReadableDiffSets indexUpdatesForSuffixOrContains( SchemaIndexDescriptor descriptor, IndexQuery query )
{ {
descriptor.schema().getPropertyId(); // assert single property index assert descriptor.schema().getPropertyIds().length == 0 :
"Suffix and contains queries are only supported for single property queries";


if ( indexUpdates == null ) if ( indexUpdates == null )
{ {
Expand Down
Expand Up @@ -76,6 +76,7 @@
import org.neo4j.io.IOUtils; import org.neo4j.io.IOUtils;
import org.neo4j.kernel.GraphDatabaseQueryService; import org.neo4j.kernel.GraphDatabaseQueryService;
import org.neo4j.kernel.api.KernelTransaction; import org.neo4j.kernel.api.KernelTransaction;
import org.neo4j.kernel.api.SilentTokenNameLookup;
import org.neo4j.kernel.api.Statement; import org.neo4j.kernel.api.Statement;
import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.exceptions.Status;
import org.neo4j.kernel.api.explicitindex.AutoIndexing; import org.neo4j.kernel.api.explicitindex.AutoIndexing;
Expand Down Expand Up @@ -617,7 +618,7 @@ public ResourceIterator<Node> findNodes( Label label, Map<String,Object> propert
KernelTransaction transaction = statementContext.getKernelTransactionBoundToThisThread( true ); KernelTransaction transaction = statementContext.getKernelTransactionBoundToThisThread( true );
TokenRead tokenRead = transaction.tokenRead(); TokenRead tokenRead = transaction.tokenRead();
int labelId = tokenRead.nodeLabel( label.name() ); int labelId = tokenRead.nodeLabel( label.name() );
IndexQuery[] queries = new IndexQuery[propertyValues.size()]; IndexQuery.ExactPredicate[] queries = new IndexQuery.ExactPredicate[propertyValues.size()];
int i = 0; int i = 0;
for ( Map.Entry<String,Object> entry : propertyValues.entrySet() ) for ( Map.Entry<String,Object> entry : propertyValues.entrySet() )
{ {
Expand Down Expand Up @@ -722,7 +723,8 @@ private ResourceIterator<Node> nodesByLabelAndProperty( KernelTransaction transa
return getNodesByLabelAndPropertyWithoutIndex( statement, labelId, query ); return getNodesByLabelAndPropertyWithoutIndex( statement, labelId, query );
} }


private ResourceIterator<Node> nodesByLabelAndProperties( KernelTransaction transaction, int labelId, IndexQuery... queries ) private ResourceIterator<Node> nodesByLabelAndProperties(
KernelTransaction transaction, int labelId, IndexQuery.ExactPredicate... queries )
{ {
Statement statement = transaction.acquireStatement(); Statement statement = transaction.acquireStatement();
Read read = transaction.dataRead(); Read read = transaction.dataRead();
Expand All @@ -733,33 +735,54 @@ private ResourceIterator<Node> nodesByLabelAndProperties( KernelTransaction tran
return emptyResourceIterator(); return emptyResourceIterator();
} }


int[] propertyIds = getSortedPropertyIds( queries ); int[] propertyIds = getPropertyIds( queries );
int[] workingCopy = new int[propertyIds.length]; CapableIndexReference index = findMatchingIndex( transaction, labelId, propertyIds );


Iterator<CapableIndexReference> indexes = transaction.schemaRead().indexesGetForLabel( labelId ); if ( index != CapableIndexReference.NO_INDEX )
while ( indexes.hasNext() )
{ {
CapableIndexReference index = indexes.next(); try
int[] original = index.properties();
if ( hasSamePropertyIds( original, workingCopy, propertyIds ) )
{ {
// Ha! We found an index - let's use it to find matching nodes NodeValueIndexCursor cursor = transaction.cursors().allocateNodeValueIndexCursor();
try read.nodeIndexSeek( index, cursor, IndexOrder.NONE, getReorderedIndexQueries( index.properties(), queries ) );
{ return new NodeCursorResourceIterator<>( cursor, statement, this::newNodeProxy );
NodeValueIndexCursor cursor = transaction.cursors().allocateNodeValueIndexCursor(); }
read.nodeIndexSeek( index, cursor, IndexOrder.NONE, getReorderedIndexQueries( original, queries ) ); catch ( KernelException e )
{
// weird at this point but ignore and fallback to a label scan
}
}
return getNodesByLabelAndPropertyWithoutIndex( statement, labelId, queries );
}


return new NodeCursorResourceIterator<>( cursor, statement, this::newNodeProxy ); private CapableIndexReference findMatchingIndex( KernelTransaction transaction, int labelId, int[] propertyIds )
} {
catch ( KernelException e ) CapableIndexReference index = transaction.schemaRead().index( labelId, propertyIds );
if ( index != CapableIndexReference.NO_INDEX )
{
// index found with property order matching the query
return index;
}
else
{
// attempt to find matching index with different property order
Arrays.sort( propertyIds );
assertNoDuplicates( propertyIds, transaction.tokenRead() );

int[] workingCopy = new int[propertyIds.length];

Iterator<CapableIndexReference> indexes = transaction.schemaRead().indexesGetForLabel( labelId );
while ( indexes.hasNext() )
{
index = indexes.next();
int[] original = index.properties();
if ( hasSamePropertyIds( original, workingCopy, propertyIds ) )
{ {
// weird at this point but ignore and fallback to a label scan // Ha! We found an index with the same properties in another order
break; return index;
} }
} }
return CapableIndexReference.NO_INDEX;
} }

return getNodesByLabelAndPropertyWithoutIndex( statement, labelId, queries );
} }


private IndexQuery[] getReorderedIndexQueries( int[] indexPropertyIds, IndexQuery[] queries ) private IndexQuery[] getReorderedIndexQueries( int[] indexPropertyIds, IndexQuery[] queries )
Expand Down Expand Up @@ -791,14 +814,13 @@ private boolean hasSamePropertyIds( int[] original, int[] workingCopy, int[] pro
return false; return false;
} }


private int[] getSortedPropertyIds( IndexQuery[] queries ) private int[] getPropertyIds( IndexQuery[] queries )
{ {
int[] propertyIds = new int[queries.length]; int[] propertyIds = new int[queries.length];
for ( int i = 0; i < queries.length; i++ ) for ( int i = 0; i < queries.length; i++ )
{ {
propertyIds[i] = queries[i].propertyKeyId(); propertyIds[i] = queries[i].propertyKeyId();
} }
Arrays.sort( propertyIds );
return propertyIds; return propertyIds;
} }


Expand All @@ -809,11 +831,27 @@ private boolean isInvalidQuery( int labelId, IndexQuery[] queries )
{ {
int propertyKeyId = query.propertyKeyId(); int propertyKeyId = query.propertyKeyId();
invalidQuery = invalidQuery || propertyKeyId == NO_SUCH_PROPERTY_KEY; invalidQuery = invalidQuery || propertyKeyId == NO_SUCH_PROPERTY_KEY;

} }
return invalidQuery; return invalidQuery;
} }


private void assertNoDuplicates( int[] propertyIds, TokenRead tokenRead )
{
int prev = propertyIds[0];
for ( int i = 1; i < propertyIds.length; i++ )
{
int curr = propertyIds[i];
if ( curr == prev )
{
SilentTokenNameLookup tokenLookup = new SilentTokenNameLookup( tokenRead );
throw new IllegalArgumentException(
format( "Provided two queries for property %s. Only one query per property key can be performed",
tokenLookup.propertyKeyGetName( curr ) ) );
}
prev = curr;
}
}

private ResourceIterator<Node> getNodesByLabelAndPropertyWithoutIndex( private ResourceIterator<Node> getNodesByLabelAndPropertyWithoutIndex(
Statement statement, int labelId, IndexQuery... queries ) Statement statement, int labelId, IndexQuery... queries )
{ {
Expand Down Expand Up @@ -1030,13 +1068,19 @@ private boolean hasPropertiesWithValues()
{ {
for ( IndexQuery query : queries ) for ( IndexQuery query : queries )
{ {
if ( propertyCursor.propertyKey() == query.propertyKeyId() && if ( propertyCursor.propertyKey() == query.propertyKeyId() )
query.acceptsValueAt( propertyCursor ) )
{ {
targetCount--; if ( query.acceptsValueAt( propertyCursor ) )
if ( targetCount == 0 ) {
targetCount--;
if ( targetCount == 0 )
{
return true;
}
}
else
{ {
return true; return false;
} }
} }
} }
Expand Down
Expand Up @@ -85,6 +85,7 @@ public void initialize( SchemaIndexDescriptor descriptor, IndexProgressor progre
break; break;


case stringPrefix: case stringPrefix:
assert query.length == 1;
prefixQuery( descriptor, (IndexQuery.StringPrefixPredicate) firstPredicate ); prefixQuery( descriptor, (IndexQuery.StringPrefixPredicate) firstPredicate );
break; break;


Expand Down
Expand Up @@ -32,11 +32,12 @@
import java.util.HashMap; import java.util.HashMap;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.concurrent.TimeUnit;


import org.neo4j.collection.primitive.Primitive; import org.neo4j.collection.primitive.Primitive;
import org.neo4j.collection.primitive.PrimitiveLongIterator; import org.neo4j.collection.primitive.PrimitiveLongIterator;
import org.neo4j.collection.primitive.PrimitiveLongSet; import org.neo4j.collection.primitive.PrimitiveLongSet;
import org.neo4j.test.mockito.matcher.Neo4jMatchers; import org.neo4j.graphdb.schema.IndexCreator;
import org.neo4j.test.rule.ImpermanentDatabaseRule; import org.neo4j.test.rule.ImpermanentDatabaseRule;


import static org.hamcrest.core.IsEqual.equalTo; import static org.hamcrest.core.IsEqual.equalTo;
Expand Down Expand Up @@ -84,8 +85,24 @@ public void setup()
db = dbRule.getGraphDatabaseAPI(); db = dbRule.getGraphDatabaseAPI();
if ( withIndex ) if ( withIndex )
{ {
Neo4jMatchers.createIndex( db, LABEL, keys ); try ( org.neo4j.graphdb.Transaction tx = db.beginTx() )
Neo4jMatchers.createIndex( db, LABEL, keys[0] ); {
db.schema().indexFor( LABEL ).on( keys[0] ).create();

IndexCreator indexCreator = db.schema().indexFor( LABEL );
for ( String key : keys )
{
indexCreator = indexCreator.on( key );
}
indexCreator.create();
tx.success();
}

try ( org.neo4j.graphdb.Transaction tx = db.beginTx() )
{
db.schema().awaitIndexesOnline( 5, TimeUnit.MINUTES );
tx.success();
}
} }
} }


Expand Down
Expand Up @@ -26,11 +26,11 @@
import org.junit.rules.TestName; import org.junit.rules.TestName;


import java.util.Map; import java.util.Map;
import java.util.concurrent.TimeUnit;


import org.neo4j.collection.primitive.Primitive; import org.neo4j.collection.primitive.Primitive;
import org.neo4j.collection.primitive.PrimitiveLongIterator; import org.neo4j.collection.primitive.PrimitiveLongIterator;
import org.neo4j.collection.primitive.PrimitiveLongSet; import org.neo4j.collection.primitive.PrimitiveLongSet;
import org.neo4j.test.mockito.matcher.Neo4jMatchers;
import org.neo4j.test.rule.ImpermanentDatabaseRule; import org.neo4j.test.rule.ImpermanentDatabaseRule;


import static org.hamcrest.core.IsEqual.equalTo; import static org.hamcrest.core.IsEqual.equalTo;
Expand Down Expand Up @@ -71,7 +71,17 @@ public void setup()
db = dbRule.getGraphDatabaseAPI(); db = dbRule.getGraphDatabaseAPI();
if ( withIndex ) if ( withIndex )
{ {
Neo4jMatchers.createIndex( db, LABEL, KEY ); try ( org.neo4j.graphdb.Transaction tx = db.beginTx() )
{
db.schema().indexFor( LABEL ).on( KEY ).create();
tx.success();
}

try ( org.neo4j.graphdb.Transaction tx = db.beginTx() )
{
db.schema().awaitIndexesOnline( 5, TimeUnit.MINUTES );
tx.success();
}
} }
} }


Expand Down

0 comments on commit aa9b98b

Please sign in to comment.