From aff65cef34429bc15f8e1cf82d811252cf2002b7 Mon Sep 17 00:00:00 2001 From: Mattias Persson Date: Sun, 19 Mar 2017 19:28:47 +0100 Subject: [PATCH] Overwriting property with same value no longer generates write command It seems obvious, but setting a node/relationship property to the same value as it already is doesn't require any change to that node/relationship. Even so this was the case previously. A command would be generated, property record would be updates on apply, indexing changes would be applied... all for nothing. --- .../api/StateHandlingStatementOperations.java | 107 ++++++++---------- .../StateHandlingStatementOperationsTest.java | 72 ++++++++++++ .../impl/event/ExpectedTransactionData.java | 18 ++- 3 files changed, 132 insertions(+), 65 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java index ce7d763958196..3a577f2d17175 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java @@ -60,6 +60,7 @@ import org.neo4j.kernel.api.exceptions.schema.TooManyLabelsException; import org.neo4j.kernel.api.index.IndexDescriptor; import org.neo4j.kernel.api.index.InternalIndexState; +import org.neo4j.kernel.api.legacyindex.AutoIndexOperations; import org.neo4j.kernel.api.legacyindex.AutoIndexing; import org.neo4j.kernel.api.properties.DefinedProperty; import org.neo4j.kernel.api.properties.Property; @@ -80,6 +81,7 @@ import org.neo4j.kernel.impl.index.LegacyIndexStore; import org.neo4j.kernel.impl.util.Cursors; import org.neo4j.register.Register.DoubleLongRegister; +import org.neo4j.storageengine.api.EntityItem; import org.neo4j.storageengine.api.EntityType; import org.neo4j.storageengine.api.LabelItem; import org.neo4j.storageengine.api.NodeItem; @@ -960,30 +962,45 @@ public Property nodeSetProperty( KernelStatement state, long nodeId, DefinedProp try ( Cursor cursor = nodeCursorById( state, nodeId ) ) { NodeItem node = cursor.get(); - Property existingProperty; - try ( Cursor properties = node.property( property.propertyKeyId() ) ) + Property existingProperty = readProperty( property.propertyKeyId(), node, EntityType.NODE ); + + if ( !property.equals( existingProperty ) ) { - if ( !properties.next() ) - { - autoIndexing.nodes().propertyAdded( ops, nodeId, property ); - existingProperty = Property.noProperty( property.propertyKeyId(), EntityType.NODE, node.id() ); - } - else - { - existingProperty = Property.property( properties.get().propertyKeyId(), properties.get().value() ); - autoIndexing.nodes().propertyChanged( ops, nodeId, existingProperty, property ); - } - } + autoIndexProperty( nodeId, property, ops, existingProperty, autoIndexing.nodes() ); - state.txState().nodeDoReplaceProperty( node.id(), existingProperty, property ); + state.txState().nodeDoReplaceProperty( node.id(), existingProperty, property ); - DefinedProperty before = definedPropertyOrNull( existingProperty ); - indexesUpdateProperty( state, node, property.propertyKeyId(), before, property ); + DefinedProperty before = definedPropertyOrNull( existingProperty ); + indexesUpdateProperty( state, node, property.propertyKeyId(), before, property ); + } return existingProperty; } } + private void autoIndexProperty( long entityId, DefinedProperty property, DataWriteOperations ops, + Property existingProperty, AutoIndexOperations indexing ) throws AutoIndexingKernelException + { + if ( existingProperty.isDefined() ) + { + indexing.propertyChanged( ops, entityId, existingProperty, property ); + } + else + { + indexing.propertyAdded( ops, entityId, property ); + } + } + + private Property readProperty( int propertyKeyId, EntityItem entity, EntityType entityType ) + { + try ( Cursor properties = entity.property( propertyKeyId ) ) + { + return properties.next() + ? Property.property( properties.get().propertyKeyId(), properties.get().value() ) + : Property.noProperty( propertyKeyId, entityType, entity.id() ); + } + } + @Override public Property relationshipSetProperty( KernelStatement state, long relationshipId, @@ -994,23 +1011,13 @@ public Property relationshipSetProperty( KernelStatement state, try ( Cursor cursor = relationshipCursorById( state, relationshipId ) ) { RelationshipItem relationship = cursor.get(); - Property existingProperty; - try ( Cursor properties = relationship.property( property.propertyKeyId() ) ) + Property existingProperty = readProperty( property.propertyKeyId(), relationship, EntityType.RELATIONSHIP ); + + if ( !property.equals( existingProperty ) ) { - if ( !properties.next() ) - { - autoIndexing.relationships().propertyAdded( ops, relationshipId, property ); - existingProperty = Property.noProperty( property.propertyKeyId(), EntityType.RELATIONSHIP, - relationship.id() ); - } - else - { - existingProperty = Property.property( properties.get().propertyKeyId(), properties.get().value() ); - autoIndexing.relationships().propertyChanged( ops, relationshipId, existingProperty, property ); - } + autoIndexProperty( relationshipId, property, ops, existingProperty, autoIndexing.relationships() ); + state.txState().relationshipDoReplaceProperty( relationship.id(), existingProperty, property ); } - - state.txState().relationshipDoReplaceProperty( relationship.id(), existingProperty, property ); return existingProperty; } } @@ -1034,22 +1041,13 @@ public Property nodeRemoveProperty( KernelStatement state, long nodeId, int prop try ( Cursor cursor = nodeCursorById( state, nodeId ) ) { NodeItem node = cursor.get(); - Property existingProperty; - try ( Cursor properties = node.property( propertyKeyId ) ) + Property existingProperty = readProperty( propertyKeyId, node, EntityType.NODE ); + if ( existingProperty.isDefined() ) { - if ( !properties.next() ) - { - existingProperty = Property.noProperty( propertyKeyId, EntityType.NODE, node.id() ); - } - else - { - existingProperty = Property.property( properties.get().propertyKeyId(), properties.get().value() ); - - autoIndexing.nodes().propertyRemoved( ops, nodeId, propertyKeyId ); - state.txState().nodeDoRemoveProperty( node.id(), (DefinedProperty) existingProperty ); + autoIndexing.nodes().propertyRemoved( ops, nodeId, propertyKeyId ); + state.txState().nodeDoRemoveProperty( node.id(), (DefinedProperty) existingProperty ); - indexesUpdateProperty( state, node, propertyKeyId, (DefinedProperty) existingProperty, null ); - } + indexesUpdateProperty( state, node, propertyKeyId, (DefinedProperty) existingProperty, null ); } return existingProperty; } @@ -1065,21 +1063,12 @@ public Property relationshipRemoveProperty( KernelStatement state, try ( Cursor cursor = relationshipCursorById( state, relationshipId ) ) { RelationshipItem relationship = cursor.get(); - Property existingProperty; - try ( Cursor properties = relationship.property( propertyKeyId ) ) + Property existingProperty = readProperty( propertyKeyId, relationship, EntityType.RELATIONSHIP ); + if ( existingProperty.isDefined() ) { - if ( !properties.next() ) - { - existingProperty = Property.noProperty( propertyKeyId, EntityType.RELATIONSHIP, relationship.id() ); - } - else - { - existingProperty = Property.property( properties.get().propertyKeyId(), properties.get().value() ); - - autoIndexing.relationships().propertyRemoved( ops, relationshipId, propertyKeyId ); - state.txState().relationshipDoRemoveProperty( relationship.id(), - (DefinedProperty) existingProperty ); - } + autoIndexing.relationships().propertyRemoved( ops, relationshipId, propertyKeyId ); + state.txState().relationshipDoRemoveProperty( relationship.id(), + (DefinedProperty) existingProperty ); } return existingProperty; } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/state/StateHandlingStatementOperationsTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/state/StateHandlingStatementOperationsTest.java index a0403f04312ba..d8b7a818dbb94 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/state/StateHandlingStatementOperationsTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/state/StateHandlingStatementOperationsTest.java @@ -36,6 +36,7 @@ import org.neo4j.kernel.api.constraints.UniquenessConstraint; import org.neo4j.kernel.api.exceptions.index.IndexNotFoundKernelException; import org.neo4j.kernel.api.index.IndexDescriptor; +import org.neo4j.kernel.api.properties.Property; import org.neo4j.kernel.api.txstate.TransactionState; import org.neo4j.kernel.impl.api.IndexReaderFactory; import org.neo4j.kernel.impl.api.KernelStatement; @@ -48,12 +49,15 @@ import org.neo4j.kernel.impl.util.diffsets.DiffSets; import org.neo4j.storageengine.api.LabelItem; import org.neo4j.storageengine.api.NodeItem; +import org.neo4j.storageengine.api.PropertyItem; +import org.neo4j.storageengine.api.RelationshipItem; import org.neo4j.storageengine.api.StorageStatement; import org.neo4j.storageengine.api.StoreReadLayer; import org.neo4j.storageengine.api.schema.IndexReader; import org.neo4j.storageengine.api.schema.LabelScanReader; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyInt; @@ -422,6 +426,74 @@ public void nodeCursorGetFromUniqueIndexSeekClosesIndexReader() throws Exception verify( indexReader ).close(); } + @Test + public void shouldNotRecordNodeSetPropertyOnSameValue() throws Exception + { + // GIVEN + int propertyKeyId = 5; + long nodeId = 0; + String value = "The value"; + KernelStatement kernelStatement = mock( KernelStatement.class ); + StoreStatement storeStatement = mock( StoreStatement.class ); + Cursor ourNode = nodeCursorWithProperty( propertyKeyId, value ); + when( storeStatement.acquireSingleNodeCursor( nodeId ) ).thenReturn( ourNode ); + when( kernelStatement.getStoreStatement() ).thenReturn( storeStatement ); + StateHandlingStatementOperations operations = newTxStateOps( mock( StoreReadLayer.class ) ); + + // WHEN + operations.nodeSetProperty( kernelStatement, nodeId, Property.stringProperty( propertyKeyId, value ) ); + + // THEN + assertFalse( kernelStatement.hasTxStateWithChanges() ); + } + + @Test + public void shouldNotRecordRelationshipSetPropertyOnSameValue() throws Exception + { + // GIVEN + int propertyKeyId = 5; + long relationshipId = 0; + String value = "The value"; + KernelStatement kernelStatement = mock( KernelStatement.class ); + StoreStatement storeStatement = mock( StoreStatement.class ); + Cursor ourRelationship = relationshipCursorWithProperty( propertyKeyId, value ); + when( storeStatement.acquireSingleRelationshipCursor( relationshipId ) ).thenReturn( ourRelationship ); + when( kernelStatement.getStoreStatement() ).thenReturn( storeStatement ); + StateHandlingStatementOperations operations = newTxStateOps( mock( StoreReadLayer.class ) ); + + // WHEN + operations.relationshipSetProperty( kernelStatement, relationshipId, + Property.stringProperty( propertyKeyId, value ) ); + + // THEN + assertFalse( kernelStatement.hasTxStateWithChanges() ); + } + + private Cursor nodeCursorWithProperty( int propertyKeyId, String value ) + { + Cursor propertyCursor = propertyCursor( propertyKeyId, value ); + NodeItem item = mock( NodeItem.class ); + when( item.property( propertyKeyId ) ).thenReturn( propertyCursor ); + return Cursors.cursor( item ); + } + + private Cursor relationshipCursorWithProperty( int propertyKeyId, String value ) + { + Cursor propertyCursor = propertyCursor( propertyKeyId, value ); + RelationshipItem item = mock( RelationshipItem.class ); + when( item.property( propertyKeyId ) ).thenReturn( propertyCursor ); + return Cursors.cursor( item ); + } + + private Cursor propertyCursor( int propertyKeyId, String value ) + { + PropertyItem propertyItem = mock( PropertyItem.class ); + when( propertyItem.propertyKeyId() ).thenReturn( propertyKeyId ); + when( propertyItem.value() ).thenReturn( value ); + Cursor propertyCursor = Cursors.cursor( propertyItem ); + return propertyCursor; + } + private StateHandlingStatementOperations newTxStateOps( StoreReadLayer delegate ) { return new StateHandlingStatementOperations( delegate, diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/event/ExpectedTransactionData.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/event/ExpectedTransactionData.java index 4cd9c3689fec2..33af8d4451f70 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/event/ExpectedTransactionData.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/event/ExpectedTransactionData.java @@ -126,17 +126,23 @@ void deletedRelationship( Relationship relationship ) void assignedProperty( Node node, String key, Object value, Object valueBeforeTx ) { valueBeforeTx = removeProperty( expectedRemovedNodeProperties, node, key, valueBeforeTx ); - Map> map = expectedAssignedNodeProperties.get( node ); - PropertyEntryImpl prev = map.get( key ); - map.put( key, property( node, key, value, prev != null ? prev.previouslyCommitedValue() : valueBeforeTx ) ); + if ( !value.equals( valueBeforeTx ) ) + { + Map> map = expectedAssignedNodeProperties.get( node ); + PropertyEntryImpl prev = map.get( key ); + map.put( key, property( node, key, value, prev != null ? prev.previouslyCommitedValue() : valueBeforeTx ) ); + } } void assignedProperty( Relationship rel, String key, Object value, Object valueBeforeTx ) { valueBeforeTx = removeProperty( expectedRemovedRelationshipProperties, rel, key, valueBeforeTx ); - Map> map = expectedAssignedRelationshipProperties.get( rel ); - PropertyEntryImpl prev = map.get( key ); - map.put( key, property( rel, key, value, prev != null ? prev.previouslyCommitedValue() : valueBeforeTx ) ); + if ( !value.equals( valueBeforeTx ) ) + { + Map> map = expectedAssignedRelationshipProperties.get( rel ); + PropertyEntryImpl prev = map.get( key ); + map.put( key, property( rel, key, value, prev != null ? prev.previouslyCommitedValue() : valueBeforeTx ) ); + } } void assignedLabel( Node node, Label label )