From 0e8c2dc6787dc3b739b309aa57044deb24def451 Mon Sep 17 00:00:00 2001 From: Pontus Melke Date: Sat, 8 Apr 2017 17:03:25 +0200 Subject: [PATCH] Handle missing properties when populating index If we create an index on some properties and some nodes are missing one or more of the indexed properties we should not fail with a silent null pointer exception in the populating, thread rendering the index unusable. --- .../kernel/impl/api/index/NodeUpdates.java | 21 ++++++----- .../CompositeIndexAcceptanceTest.scala | 37 +++++++++++++++++-- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/index/NodeUpdates.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/index/NodeUpdates.java index 92b6b84d9e466..1b1af331a8870 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/index/NodeUpdates.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/index/NodeUpdates.java @@ -24,11 +24,11 @@ import java.util.List; import org.neo4j.collection.primitive.Primitive; +import org.neo4j.collection.primitive.PrimitiveArrays; import org.neo4j.collection.primitive.PrimitiveIntCollection; import org.neo4j.collection.primitive.PrimitiveIntIterator; import org.neo4j.collection.primitive.PrimitiveIntObjectMap; import org.neo4j.collection.primitive.PrimitiveIntSet; -import org.neo4j.collection.primitive.PrimitiveArrays; import org.neo4j.helpers.collection.Iterables; import org.neo4j.kernel.api.index.IndexEntryUpdate; import org.neo4j.kernel.api.properties.DefinedProperty; @@ -131,17 +131,17 @@ public final long getNodeId() return nodeId; } - public long[] labelsChanged() + long[] labelsChanged() { return PrimitiveArrays.symmetricDifference( labelsBefore, labelsAfter ); } - public long[] labelsUnchanged() + long[] labelsUnchanged() { return PrimitiveArrays.intersect( labelsBefore, labelsAfter ); } - public PrimitiveIntCollection propertiesChanged() + PrimitiveIntCollection propertiesChanged() { assert !hasLoadedAdditionalProperties : "Calling propertiesChanged() is not valid after non-changed " + "properties have already been loaded."; @@ -231,7 +231,7 @@ else if ( !relevantBefore && relevantAfter ) nodeId, indexKey, valuesAfter( propertyIds ) ) ); } - else if ( relevantBefore && relevantAfter ) + else if ( relevantBefore ) { if ( valuesChanged( propertyIds ) ) { @@ -308,7 +308,8 @@ private boolean hasPropsBefore( int[] propertyIds ) { for ( int propertyId : propertyIds ) { - if ( !knownProperties.get( propertyId ).hasBefore() ) + PropertyValue propertyValue = knownProperties.get( propertyId ); + if ( propertyValue == null || !propertyValue.hasBefore() ) { return false; } @@ -320,7 +321,8 @@ private boolean hasPropsAfter( int[] propertyIds ) { for ( int propertyId : propertyIds ) { - if ( !knownProperties.get( propertyId ).hasAfter() ) + PropertyValue propertyValue = knownProperties.get( propertyId ); + if ( propertyValue == null || !propertyValue.hasAfter() ) { return false; } @@ -343,7 +345,8 @@ private Object[] valuesAfter( int[] propertyIds ) Object[] values = new Object[propertyIds.length]; for ( int i = 0; i < propertyIds.length; i++ ) { - values[i] = knownProperties.get( propertyIds[i] ).after; + PropertyValue propertyValue = knownProperties.get( propertyIds[i] ); + values[i] = propertyValue == null ? null : propertyValue.after; } return values; } @@ -444,7 +447,7 @@ enum PropertyValueType Before, After, UnChanged, - Changed; + Changed } private static class PropertyValue diff --git a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/CompositeIndexAcceptanceTest.scala b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/CompositeIndexAcceptanceTest.scala index 46288e3e3b019..e6a1f127692a2 100644 --- a/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/CompositeIndexAcceptanceTest.scala +++ b/enterprise/cypher/acceptance-spec-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/CompositeIndexAcceptanceTest.scala @@ -19,8 +19,8 @@ */ package org.neo4j.internal.cypher.acceptance -import org.neo4j.cypher.{CypherExecutionException, ExecutionEngineFunSuite, NewPlannerTestSupport, SyntaxException} -import org.neo4j.graphdb.{ConstraintViolationException, Node} +import org.neo4j.cypher.{ExecutionEngineFunSuite, NewPlannerTestSupport} +import org.neo4j.graphdb.Node import org.neo4j.kernel.GraphDatabaseQueryService import org.scalatest.matchers.{MatchResult, Matcher} @@ -162,7 +162,8 @@ class CompositeIndexAcceptanceTest extends ExecutionEngineFunSuite with NewPlann test("should be able to update composite index when only one property has changed") { graph.createIndex("Person", "firstname", "lastname") - val n = executeWithCostPlannerAndInterpretedRuntimeOnly("CREATE (n:Person {firstname:'Joe', lastname:'Soap'}) RETURN n").columnAs("n").toList(0).asInstanceOf[Node] + val n = executeWithCostPlannerAndInterpretedRuntimeOnly( + "CREATE (n:Person {firstname:'Joe', lastname:'Soap'}) RETURN n").columnAs("n").toList.head.asInstanceOf[Node] executeWithCostPlannerAndInterpretedRuntimeOnly("MATCH (n:Person) SET n.lastname = 'Bloggs'") val result = executeWithCostPlannerAndInterpretedRuntimeOnly("MATCH (n:Person) where n.firstname = 'Joe' and n.lastname = 'Bloggs' RETURN n") result should use("NodeIndexSeek") @@ -243,6 +244,36 @@ class CompositeIndexAcceptanceTest extends ExecutionEngineFunSuite with NewPlann result should evaluateTo(List(Map("x" -> 1), Map("x" -> 3), Map("x" -> 5), Map("x" -> 7), Map("x" -> 9))) } + test("should handle missing properties when populating index") { + // Given + createLabeledNode(Map("foo" -> 42), "PROFILES") + createLabeledNode(Map("foo" -> 42, "bar" -> 1337, "baz" -> 1980), "L") + + // When + graph.createIndex("L", "foo", "bar", "baz") + + // Then + graph should haveIndexes(":L(foo,bar,baz)") + val result = executeWithAllPlanners("MATCH (n:L {foo: 42, bar: 1337, baz: 1980}) RETURN count(n)") + result should evaluateTo(Seq(Map("count(n)" -> 1))) + result should useIndex(":L(foo,bar,baz") + } + + test("should handle missing properties when adding to index after creation") { + // Given + createLabeledNode(Map("foo" -> 42), "PROFILES") + + // When + graph.createIndex("L", "foo", "bar", "baz") + createLabeledNode(Map("foo" -> 42, "bar" -> 1337, "baz" -> 1980), "L") + + // Then + graph should haveIndexes(":L(foo,bar,baz)") + val result = executeWithAllPlanners("MATCH (n:L {foo: 42, bar: 1337, baz: 1980}) RETURN count(n)") + result should evaluateTo(Seq(Map("count(n)" -> 1))) + result should useIndex(":L(foo,bar,baz") + } + case class haveIndexes(expectedIndexes: String*) extends Matcher[GraphDatabaseQueryService] { def apply(graph: GraphDatabaseQueryService): MatchResult = { graph.inTx {