Addressed remaining PR comments for IndexDescriptor API work

neo4j · Jan 23, 2017 · 19527cd · 19527cd
1 parent d10e944
commit 19527cd
Show file tree

Hide file tree

Showing 11 changed files with 38 additions and 77 deletions.
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/schema/NodeMultiPropertyDescriptor.java b/community/kernel/src/main/java/org/neo4j/kernel/api/schema/NodeMultiPropertyDescriptor.java
@@ -91,13 +91,13 @@ public int[] getPropertyKeyIds()
     @Override
     public String propertyIdText()
     {
-        return Arrays.stream( propertyKeyIds ).mapToObj( id -> Integer.toString( id ) )
+        return Arrays.stream( propertyKeyIds ).mapToObj( Integer::toString )
                 .collect( Collectors.joining( "," ) );
     }
 
     public String propertyNameText( TokenNameLookup tokenNameLookup )
     {
-        return Arrays.stream( propertyKeyIds ).mapToObj( id -> tokenNameLookup.propertyKeyGetName( id ) )
+        return Arrays.stream( propertyKeyIds ).mapToObj( tokenNameLookup::propertyKeyGetName )
                 .collect( Collectors.joining( "," ) );
     }
 
@@ -135,6 +135,6 @@ public int compareTo( EntityPropertyDescriptor other )
             }
             return cmp;
         }
-        return -1;  //TODO: We can compare single and composite indexes, by sorting one type always above the other
+        return -1;
     }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/SchemaProcedure.java b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/SchemaProcedure.java
@@ -83,7 +83,7 @@ public GraphResult buildSchemaGraph()
                 {
                     IndexDescriptor index = indexDescriptorIterator.next();
                     String[] propertyNames = PropertyNameUtils.getPropertyKeys( statementTokenNameLookup, index.descriptor() );
-                    indexes.add( Arrays.stream( propertyNames ).collect( Collectors.joining( "," ) ) );
+                    indexes.add( String.join( ",", propertyNames ) );
                 }
                 properties.put( "indexes", indexes );
 

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/SchemaCache.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/SchemaCache.java
@@ -25,6 +25,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.neo4j.helpers.collection.Iterators;
 import org.neo4j.kernel.api.schema.NodePropertyDescriptor;
@@ -58,7 +59,7 @@ public class SchemaCache
 
     private final Collection<NodePropertyConstraint> nodeConstraints = new HashSet<>();
     private final Collection<RelationshipPropertyConstraint> relationshipConstraints = new HashSet<>();
-    private final Map<IndexDescriptor, CommittedIndexDescriptor> indexDescriptors = new HashMap<>();
+    private final Set<IndexDescriptor> indexDescriptors = new HashSet<>();
     private final ConstraintSemantics constraintSemantics;
 
     public SchemaCache( ConstraintSemantics constraintSemantics, Iterable<SchemaRule> initialRules )
@@ -148,9 +149,9 @@ else if ( rule instanceof IndexRule )
         {
             IndexRule indexRule = (IndexRule) rule;
             IndexDescriptor index = IndexDescriptorFactory.of( indexRule );
-            if ( !indexDescriptors.containsKey( index ) )
+            if ( !indexDescriptors.contains( index ) )
             {
-                indexDescriptors.put( index, new CommittedIndexDescriptor( index, indexRule.getId() ) );
+                indexDescriptors.add( index );
             }
         }
     }
@@ -172,33 +173,6 @@ public void load( List<SchemaRule> schemaRuleIterator )
         }
     }
 
-    // We could have had this class extend IndexDescriptor instead. That way we could have gotten the id
-    // from an IndexDescriptor instance directly. The problem is that it would only work for index descriptors
-    // instantiated by a SchemaCache. Perhaps that is always the case. Anyways, doing it like that resulted
-    // in unit test failures regarding the schema cache, so this way (the wrapping way) is a more generic
-    // and stable way of doing it.
-    private static class CommittedIndexDescriptor
-    {
-        private final IndexDescriptor descriptor;
-//        private final long id;
-
-        public CommittedIndexDescriptor( IndexDescriptor descriptor, long id )
-        {
-            this.descriptor = descriptor;
-//            this.id = id;
-        }
-
-        public IndexDescriptor getDescriptor()
-        {
-            return descriptor;
-        }
-
-//        public long getId()
-//        {
-//            return id;
-//        }
-    }
-
     public void removeSchemaRule( long id )
     {
         SchemaRule rule = rulesByIdMap.remove( id );
@@ -225,6 +199,6 @@ else if ( rule instanceof IndexRule )
     public IndexDescriptor indexDescriptor( NodePropertyDescriptor descriptor )
     {
         IndexDescriptor indexDescriptor = IndexDescriptorFactory.of( descriptor );
-        return indexDescriptors.containsKey(indexDescriptor) ? indexDescriptor : null;
+        return indexDescriptors.contains(indexDescriptor) ? indexDescriptor : null;
     }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/IndexDefinitionImpl.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/IndexDefinitionImpl.java
@@ -89,7 +89,7 @@ public boolean isConstraintIndex()
     @Override
     public int hashCode()
     {
-        return hashCode(label, propertyKeys);
+        return 31 * label.name().hashCode() + Arrays.hashCode( propertyKeys );
     }
 
     @Override
@@ -122,16 +122,4 @@ protected void assertInUnterminatedTransaction()
     {
         actions.assertInOpenTransaction();
     }
-
-    public static int hashCode(Label label, String[] propertyKeys)
-    {
-        final int prime = 31;
-        int result = 1;
-        result = prime * result + label.name().hashCode();
-        for ( String propertyKey : propertyKeys )
-        {
-            result = prime * result + propertyKey.hashCode();
-        }
-        return result;
-    }
 }
diff --git a/...nity/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/InternalSchemaActions.java b/...nity/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/InternalSchemaActions.java
@@ -36,15 +36,15 @@
  */
 public interface InternalSchemaActions
 {
-    IndexDefinition createIndexDefinition( Label label, String[] propertyKey );
+    IndexDefinition createIndexDefinition( Label label, String... propertyKey );
 
     void dropIndexDefinitions( IndexDefinition indexDefinition );
 
     ConstraintDefinition createPropertyUniquenessConstraint( IndexDefinition indexDefinition )
             throws IllegalTokenNameException, TooManyLabelsException, CreateConstraintFailureException,
                    AlreadyConstrainedException, AlreadyIndexedException;
 
-    ConstraintDefinition createPropertyExistenceConstraint( Label label, String[] propertyKey )
+    ConstraintDefinition createPropertyExistenceConstraint( Label label, String... propertyKey )
             throws IllegalTokenNameException, TooManyLabelsException, CreateConstraintFailureException,
             AlreadyConstrainedException;
 

diff --git a/...src/main/java/org/neo4j/kernel/impl/coreapi/schema/MultiPropertyConstraintDefinition.java b/...src/main/java/org/neo4j/kernel/impl/coreapi/schema/MultiPropertyConstraintDefinition.java
@@ -25,6 +25,7 @@
 import org.neo4j.graphdb.schema.ConstraintDefinition;
 import org.neo4j.graphdb.schema.ConstraintType;
 import org.neo4j.graphdb.schema.IndexDefinition;
+import org.neo4j.helpers.collection.Iterables;
 
 import static java.util.Arrays.asList;
 import static java.util.Objects.requireNonNull;
@@ -42,12 +43,7 @@ protected MultiPropertyConstraintDefinition( InternalSchemaActions actions, Stri
     protected MultiPropertyConstraintDefinition( InternalSchemaActions actions, IndexDefinition indexDefinition )
     {
         super( actions );
-
+        this.propertyKeys = requireNonEmpty( Iterables.asArray( String.class, indexDefinition.getPropertyKeys() ) );
-        List<String> indexList = new ArrayList<>();
-        indexDefinition.getPropertyKeys().forEach( index -> indexList.add( index ) );
-        String[] propertyKeys = indexList.toArray( new String[indexList.size()] );
-
-        this.propertyKeys = requireNonEmpty( propertyKeys );
     }
 
     private static String[] requireNonEmpty( String[] array )
@@ -60,7 +56,9 @@ private static String[] requireNonEmpty( String[] array )
         for ( String field : array )
         {
             if ( field == null )
-            { throw new NullPointerException(); }
+            {
+                throw new IllegalArgumentException( "Property constraints cannot have null property names" );
+            }
         }
         return array;
     }

diff --git a/...y/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/NodeConstraintDefinition.java b/...y/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/NodeConstraintDefinition.java
@@ -93,6 +93,6 @@ protected String propertyText()
     @Override
     public int hashCode()
     {
-        return IndexDefinitionImpl.hashCode( label, propertyKeys );
+        return 31 * label.name().hashCode() + Arrays.hashCode( propertyKeys );
     }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/PropertyNameUtils.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/PropertyNameUtils.java
@@ -20,11 +20,11 @@
 package org.neo4j.kernel.impl.coreapi.schema;
 
 import java.util.ArrayList;
-import java.util.Iterator;
 
 import org.neo4j.graphdb.Label;
 import org.neo4j.graphdb.NotFoundException;
 import org.neo4j.graphdb.schema.IndexDefinition;
+import org.neo4j.helpers.collection.Iterables;
 import org.neo4j.kernel.api.schema.IndexDescriptorFactory;
 import org.neo4j.kernel.api.schema.NodePropertyDescriptor;
 import org.neo4j.kernel.api.ReadOperations;
@@ -41,7 +41,11 @@
 
 public class PropertyNameUtils
 {
-    public static String getPropertyKeyNameAt( Iterable<String> properties, int propertyKeyIndex )
+    private PropertyNameUtils()
+    {
+    }
+
+    private static String getPropertyKeyNameAt( Iterable<String> properties, int propertyKeyIndex )
     {
         for ( String propertyKey : properties )
         {
@@ -54,7 +58,7 @@ public static String getPropertyKeyNameAt( Iterable<String> properties, int prop
         return null;
     }
 
-    public static IndexDescriptor getIndexDescriptor( ReadOperations readOperations, IndexDefinition index )
+    static IndexDescriptor getIndexDescriptor( ReadOperations readOperations, IndexDefinition index )
             throws SchemaRuleNotFoundException
     {
         int labelId = readOperations.labelGetForName( index.getLabel().name() );
@@ -83,7 +87,7 @@ private static NodePropertyDescriptor checkValidLabelAndProperties( Label label,
         return IndexDescriptorFactory.getNodePropertyDescriptor( labelId, propertyKeyIds );
     }
 
-    public static IndexDescriptor getIndexDescriptor( ReadOperations readOperations, Label label,
+    static IndexDescriptor getIndexDescriptor( ReadOperations readOperations, Label label,
             String[] propertyKeys )
             throws SchemaRuleNotFoundException
     {
@@ -131,9 +135,7 @@ public static int[] getPropertyKeyIds( ReadOperations statement, String[] proper
 
     public static int[] getPropertyKeyIds( ReadOperations statement, Iterable<String> propertyKeys )
     {
-        ArrayList<Integer> propertyKeyIds = new ArrayList<>();
+        return Iterables.stream( propertyKeys ).mapToInt( statement::propertyKeyGetForName ).toArray();
-        propertyKeys.forEach( index -> propertyKeyIds.add( statement.propertyKeyGetForName( index ) ) );
-        return propertyKeyIds.stream().mapToInt( i -> i ).toArray();
     }
 
     public static int[] getOrCreatePropertyKeyIds( SchemaWriteOperations statement, String[] propertyKeys )
@@ -151,10 +153,9 @@ public static int[] getOrCreatePropertyKeyIds( SchemaWriteOperations statement,
             throws IllegalTokenNameException
     {
         ArrayList<Integer> propertyKeyIds = new ArrayList<>();
-        Iterator<String> indexIterator = indexDefinition.getPropertyKeys().iterator();
+        for ( String s : indexDefinition.getPropertyKeys() )
-        while ( indexIterator.hasNext() )
         {
-            propertyKeyIds.add( statement.propertyKeyGetOrCreateForName( indexIterator.next() ) );
+            propertyKeyIds.add( statement.propertyKeyGetOrCreateForName( s ) );
         }
         return propertyKeyIds.stream().mapToInt( i -> i ).toArray();
     }

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/SchemaImpl.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/schema/SchemaImpl.java
@@ -125,8 +125,7 @@ public Iterable<IndexDefinition> getIndexes()
         }
     }
 
-    //TODO: Consider moving to PropertyNameUtils or IndexDescriptorFactory or similar
+    private IndexDefinition descriptorToDefinition( final ReadOperations statement, IndexDescriptor index,
-    private IndexDefinition from( final ReadOperations statement, IndexDescriptor index,
             final boolean constraintIndex )
     {
         try
@@ -144,7 +143,9 @@ private IndexDefinition from( final ReadOperations statement, IndexDescriptor in
     private void addDefinitions( List<IndexDefinition> definitions, final ReadOperations statement,
                                  Iterator<IndexDescriptor> indexes, final boolean constraintIndex )
     {
-        addToCollection( map( rule -> from( statement, rule, constraintIndex ), indexes ), definitions );
+        addToCollection(
+                map( index -> descriptorToDefinition( statement, index, constraintIndex ), indexes ),
+                definitions );
     }
 
     @Override
@@ -373,7 +374,7 @@ public GDBSchemaActions( Supplier<Statement> statementSupplier )
         }
 
         @Override
-        public IndexDefinition createIndexDefinition( Label label, String[] propertyKeys )
+        public IndexDefinition createIndexDefinition( Label label, String... propertyKeys )
         {
             try ( Statement statement = ctxSupplier.get() )
             {
@@ -464,7 +465,7 @@ public ConstraintDefinition createPropertyUniquenessConstraint( IndexDefinition
         }
 
         @Override
-        public ConstraintDefinition createPropertyExistenceConstraint( Label label, String[] propertyKeys )
+        public ConstraintDefinition createPropertyExistenceConstraint( Label label, String... propertyKeys )
         {
             try ( Statement statement = ctxSupplier.get() )
             {

diff --git a/...kernel/src/main/java/org/neo4j/kernel/impl/store/record/UniquePropertyConstraintRule.java b/...kernel/src/main/java/org/neo4j/kernel/impl/store/record/UniquePropertyConstraintRule.java
@@ -46,8 +46,7 @@ private UniquePropertyConstraintRule( long id, NodePropertyDescriptor descriptor
     {
         super( id, descriptor, Kind.UNIQUENESS_CONSTRAINT );
         this.ownedIndexRule = ownedIndexRule;
-        //TODO: Find a better way of asserting this
+        assert !descriptor.isComposite(); // Only uniqueness of a single property supported for now
-//        assert propertyKeyIds.length == 1; // Only uniqueness of a single property supported for now
     }
 
     @Override

diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java
@@ -1127,7 +1127,7 @@ private int[] getOrCreatePropertyKeyIds( String[] properties )
         }
 
         @Override
-        public IndexDefinition createIndexDefinition( Label label, String[] propertyKeys )
+        public IndexDefinition createIndexDefinition( Label label, String... propertyKeys )
         {
             int labelId = getOrCreateLabelId( label.name() );
             int[] propertyKeyIds = getOrCreatePropertyKeyIds( propertyKeys );
@@ -1159,7 +1159,7 @@ public ConstraintDefinition createPropertyUniquenessConstraint( IndexDefinition
         }
 
         @Override
-        public ConstraintDefinition createPropertyExistenceConstraint( Label label, String[] propertyKeys )
+        public ConstraintDefinition createPropertyExistenceConstraint( Label label, String... propertyKeys )
         {
             int labelId = getOrCreateLabelId( label.name() );
             int[] propertyKeyIds = getOrCreatePropertyKeyIds( propertyKeys );