Skip to content

Commit

Permalink
Do not store (unused) removed property values in tx state
Browse files Browse the repository at this point in the history
  • Loading branch information
Andrei Koval committed Mar 20, 2018
1 parent c87d945 commit 7130925
Show file tree
Hide file tree
Showing 8 changed files with 41 additions and 53 deletions.
6 changes: 3 additions & 3 deletions community/kernel/src/main/java/org/neo4j/kernel/api/txstate/TransactionState.java
View file
Expand Up @@ -57,11 +57,11 @@ public interface TransactionState extends ReadableTransactionState

void graphDoReplaceProperty( int propertyKeyId, Value replacedValue, Value newValue );

void nodeDoRemoveProperty( long nodeId, int propertyKeyId, Value removedValue );
void nodeDoRemoveProperty( long nodeId, int propertyKeyId );

void relationshipDoRemoveProperty( long relationshipId, int propertyKeyId, Value removedValue );
void relationshipDoRemoveProperty( long relationshipId, int propertyKeyId );

void graphDoRemoveProperty( int propertyKeyId, Value removedValue );
void graphDoRemoveProperty( int propertyKeyId );

void nodeDoAddLabel( int labelId, long nodeId );

Expand Down
7 changes: 3 additions & 4 deletions ...nity/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java
View file
Expand Up @@ -1096,7 +1096,7 @@ public Value nodeRemoveProperty( KernelStatement state, long nodeId, int propert
existingValue = properties.get().value();

autoIndexing.nodes().propertyRemoved( ops, nodeId, propertyKeyId );
state.txState().nodeDoRemoveProperty( node.id(), propertyKeyId, existingValue );
state.txState().nodeDoRemoveProperty( node.id(), propertyKeyId );

indexTxStateUpdater.onPropertyRemove( state, node, propertyKeyId, existingValue );
}
Expand All @@ -1122,8 +1122,7 @@ public Value relationshipRemoveProperty( KernelStatement state, long relationshi
existingValue = properties.get().value();

autoIndexing.relationships().propertyRemoved( ops, relationshipId, propertyKeyId );
state.txState()
.relationshipDoRemoveProperty( relationship.id(), propertyKeyId, existingValue );
state.txState().relationshipDoRemoveProperty( relationship.id(), propertyKeyId );
}
}
return existingValue;
Expand All @@ -1136,7 +1135,7 @@ public Value graphRemoveProperty( KernelStatement state, int propertyKeyId )
Value existingValue = graphGetProperty( state, propertyKeyId );
if ( existingValue != Values.NO_VALUE )
{
state.txState().graphDoRemoveProperty( propertyKeyId, existingValue );
state.txState().graphDoRemoveProperty( propertyKeyId );
}
return existingValue;
}
Expand Down
45 changes: 17 additions & 28 deletions ...nity/kernel/src/main/java/org/neo4j/kernel/impl/api/state/PropertyContainerStateImpl.java
View file
Expand Up @@ -19,7 +19,6 @@
*/
package org.neo4j.kernel.impl.api.state;

import java.util.Collections;
import java.util.Iterator;
import java.util.function.Predicate;

Expand All @@ -33,13 +32,13 @@

import static java.util.Collections.emptyIterator;

public class PropertyContainerStateImpl implements PropertyContainerState
class PropertyContainerStateImpl implements PropertyContainerState
{
private final long id;

private VersionedHashMap<Integer, Value> addedProperties;
private VersionedHashMap<Integer, Value> changedProperties;
private VersionedHashMap<Integer, Value> removedProperties;
private VersionedHashMap<Integer, Object> removedProperties;

private final Predicate<StorageProperty> excludePropertiesWeKnowAbout = new Predicate<StorageProperty>()
{
Expand All @@ -62,7 +61,7 @@ public long getId()
return id;
}

public void clear()
void clear()
{
if ( changedProperties != null )
{
Expand All @@ -80,13 +79,10 @@ public void clear()

void changeProperty( int propertyKeyId, Value value )
{
if ( addedProperties != null )
if ( addedProperties != null && addedProperties.containsKey( propertyKeyId ) )
{
if ( addedProperties.containsKey( propertyKeyId ) )
{
addedProperties.put( propertyKeyId, value );
return;
}
addedProperties.put( propertyKeyId, value );
return;
}

if ( changedProperties == null )
Expand All @@ -103,16 +99,12 @@ void changeProperty( int propertyKeyId, Value value )

void addProperty( int propertyKeyId, Value value )
{
if ( removedProperties != null )
if ( removedProperties != null && removedProperties.remove( propertyKeyId ) != null )
{
Value removed = removedProperties.remove( propertyKeyId );
if ( removed != null )
{
// This indicates the user did remove+add as two discrete steps, which should be translated to
// a single change operation.
changeProperty( propertyKeyId, value );
return;
}
// This indicates the user did remove+add as two discrete steps, which should be translated to
// a single change operation.
changeProperty( propertyKeyId, value );
return;
}
if ( addedProperties == null )
{
Expand All @@ -121,20 +113,17 @@ void addProperty( int propertyKeyId, Value value )
addedProperties.put( propertyKeyId, value );
}

public void removeProperty( int propertyKeyId, Value value )
void removeProperty( int propertyKeyId )
{
if ( addedProperties != null )
if ( addedProperties != null && addedProperties.remove( propertyKeyId ) != null )
{
if ( addedProperties.remove( propertyKeyId ) != null )
{
return;
}
return;
}
if ( removedProperties == null )
{
removedProperties = new VersionedHashMap<>();
}
removedProperties.put( propertyKeyId, value );
removedProperties.put( propertyKeyId, Boolean.TRUE );
if ( changedProperties != null )
{
changedProperties.remove( propertyKeyId );
Expand Down Expand Up @@ -235,8 +224,8 @@ public boolean isPropertyRemoved( int propertyKeyId )

private Iterator<StorageProperty> toPropertyIterator( VersionedHashMap<Integer,Value> propertyMap )
{
return propertyMap == null ? Collections.emptyIterator() :
Iterators.map(
return propertyMap == null ? emptyIterator() :
Iterators.map(
entry -> new PropertyKeyValue( entry.getKey(), entry.getValue() ),
propertyMap.entrySet().iterator()
);
Expand Down
12 changes: 6 additions & 6 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/TxState.java
View file
Expand Up @@ -569,23 +569,23 @@ public void graphDoReplaceProperty( int propertyKeyId, Value replacedValue, Valu
}

@Override
public void nodeDoRemoveProperty( long nodeId, int propertyKeyId, Value removedValue )
public void nodeDoRemoveProperty( long nodeId, int propertyKeyId )
{
getOrCreateNodeState( nodeId ).removeProperty( propertyKeyId, removedValue );
getOrCreateNodeState( nodeId ).removeProperty( propertyKeyId );
dataChanged();
}

@Override
public void relationshipDoRemoveProperty( long relationshipId, int propertyKeyId, Value removedValue )
public void relationshipDoRemoveProperty( long relationshipId, int propertyKeyId )
{
getOrCreateRelationshipState( relationshipId ).removeProperty( propertyKeyId, removedValue );
getOrCreateRelationshipState( relationshipId ).removeProperty( propertyKeyId );
dataChanged();
}

@Override
public void graphDoRemoveProperty( int propertyKeyId, Value removedValue )
public void graphDoRemoveProperty( int propertyKeyId )
{
getOrCreateGraphState().removeProperty( propertyKeyId, removedValue );
getOrCreateGraphState().removeProperty( propertyKeyId );
dataChanged();
}

Expand Down
6 changes: 3 additions & 3 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Operations.java
View file
Expand Up @@ -483,7 +483,7 @@ public Value nodeRemoveProperty( long node, int propertyKey )
if ( existingValue != NO_VALUE )
{
autoIndexing.nodes().propertyRemoved( this, node, propertyKey );
ktx.txState().nodeDoRemoveProperty( node, propertyKey, existingValue );
ktx.txState().nodeDoRemoveProperty( node, propertyKey );
updater.onPropertyRemove( nodeCursor, propertyCursor, propertyKey, existingValue );
}

Expand Down Expand Up @@ -529,7 +529,7 @@ public Value relationshipRemoveProperty( long relationship, int propertyKey )
if ( existingValue != NO_VALUE )
{
autoIndexing.relationships().propertyRemoved( this, relationship, propertyKey );
ktx.txState().relationshipDoRemoveProperty( relationship, propertyKey, existingValue );
ktx.txState().relationshipDoRemoveProperty( relationship, propertyKey );
}

return existingValue;
Expand Down Expand Up @@ -559,7 +559,7 @@ public Value graphRemoveProperty( int propertyKey )
Value existingValue = readGraphProperty( propertyKey );
if ( existingValue != Values.NO_VALUE )
{
ktx.txState().graphDoRemoveProperty( propertyKey, existingValue );
ktx.txState().graphDoRemoveProperty( propertyKey );
}
return existingValue;
}
Expand Down
4 changes: 2 additions & 2 deletions .../kernel/src/test/java/org/neo4j/kernel/impl/api/state/PropertyContainerStateImplTest.java
View file
Expand Up @@ -42,7 +42,7 @@ public void shouldListAddedProperties()
PropertyContainerStateImpl state = new PropertyContainerStateImpl( 1 );
state.addProperty( 1, Values.of( "Hello" ) );
state.addProperty( 2, Values.of( "Hello" ) );
state.removeProperty( 1, Values.of( "Hello" ) );
state.removeProperty( 1 );

// When
Iterator<StorageProperty> added = state.addedProperties();
Expand Down Expand Up @@ -79,7 +79,7 @@ public void shouldConvertAddRemoveToChange()
PropertyContainerStateImpl state = new PropertyContainerStateImpl( 1 );

// When
state.removeProperty( 4, Values.of( "a value" ) );
state.removeProperty( 4 );
state.addProperty( 4, Values.of( "another value" ) );

// Then
Expand Down
4 changes: 2 additions & 2 deletions ...ty/kernel/src/test/java/org/neo4j/kernel/impl/coreapi/TxStateTransactionDataViewTest.java
View file
Expand Up @@ -225,7 +225,7 @@ public void shouldListRemovedNodeProperties() throws Exception
// Given
int propertyKeyId = 1;
Value prevValue = Values.of( "prevValue" );
state.nodeDoRemoveProperty( 1L, propertyKeyId, prevValue );
state.nodeDoRemoveProperty( 1L, propertyKeyId );
when( ops.propertyKeyGetName( propertyKeyId ) ).thenReturn( "theKey" );
long propertyId = 20L;
when( storeStatement.acquireSingleNodeCursor( 1L ) ).thenReturn(
Expand All @@ -250,7 +250,7 @@ public void shouldListRemovedRelationshipProperties() throws Exception
// Given
int propertyKeyId = 1;
Value prevValue = Values.of( "prevValue" );
state.relationshipDoRemoveProperty( 1L, propertyKeyId, prevValue );
state.relationshipDoRemoveProperty( 1L, propertyKeyId );
when( ops.propertyKeyGetName( propertyKeyId ) ).thenReturn( "theKey" );
long propertyId = 40L;
when( storeStatement.acquireSingleRelationshipCursor( 1 ) )
Expand Down
10 changes: 5 additions & 5 deletions community/kernel/src/test/java/org/neo4j/kernel/impl/store/NeoStoresTest.java
View file
Expand Up @@ -503,7 +503,7 @@ public void testProps1() throws Exception
initializeStores( storeDir, stringMap() );
startTx();
StorageProperty prop2 = nodeAddProperty( nodeId, prop.propertyKeyId(), 5 );
transaction.nodeDoRemoveProperty( nodeId, prop2.propertyKeyId(), prop2.value() );
transaction.nodeDoRemoveProperty( nodeId, prop2.propertyKeyId() );
transaction.nodeDoDelete( nodeId );
commitTx();
ds.stop();
Expand Down Expand Up @@ -1328,7 +1328,7 @@ else if ( data.propertyKeyId() == prop3.propertyKeyId() )
assertEquals( "prop3", MyPropertyKeyToken.getIndexFor(
keyId ).name() );
assertEquals( false, data.value().asObject() );
transaction.relationshipDoRemoveProperty( rel, prop3.propertyKeyId(), prop3.value() );
transaction.relationshipDoRemoveProperty( rel, prop3.propertyKeyId() );
}
else
{
Expand Down Expand Up @@ -1389,7 +1389,7 @@ else if ( data.propertyKeyId() == prop3.propertyKeyId() )
assertEquals( "prop3", MyPropertyKeyToken.getIndexFor(
keyId ).name() );
assertEquals( true, data.value().asObject() );
transaction.relationshipDoRemoveProperty( rel, prop3.propertyKeyId(), prop3.value() );
transaction.relationshipDoRemoveProperty( rel, prop3.propertyKeyId() );
}
else
{
Expand Down Expand Up @@ -1455,7 +1455,7 @@ else if ( data.propertyKeyId() == prop3.propertyKeyId() )
assertEquals( "prop3", MyPropertyKeyToken.getIndexFor(
keyId ).name() );
assertEquals( false, data.value().asObject() );
transaction.nodeDoRemoveProperty( node, prop3.propertyKeyId(), prop3.value() );
transaction.nodeDoRemoveProperty( node, prop3.propertyKeyId() );
}
else
{
Expand Down Expand Up @@ -1501,7 +1501,7 @@ else if ( data.propertyKeyId() == prop3.propertyKeyId() )
assertEquals( "prop3", MyPropertyKeyToken.getIndexFor(
keyId ).name() );
assertEquals( true, data.value().asObject() );
transaction.nodeDoRemoveProperty( node, prop3.propertyKeyId(), prop3.value() );
transaction.nodeDoRemoveProperty( node, prop3.propertyKeyId() );
}
else
{
Expand Down

0 comments on commit 7130925

Please sign in to comment.