Skip to content

Commit

Permalink
Revert "Do not store (unused) removed property values in tx state"
Browse files Browse the repository at this point in the history 
This reverts commit 7130925.
  • Loading branch information
Andrei Koval committed Mar 20, 2018
1 parent 7130925 commit 90072aa
Show file tree
Hide file tree
Showing 8 changed files with 53 additions and 41 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 );
void nodeDoRemoveProperty( long nodeId, int propertyKeyId, Value removedValue );

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

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

void nodeDoAddLabel( int labelId, long nodeId );

Expand Down
7 changes: 4 additions & 3 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 );
state.txState().nodeDoRemoveProperty( node.id(), propertyKeyId, existingValue );

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

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

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

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

import static java.util.Collections.emptyIterator;

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

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

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

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

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

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

void addProperty( int propertyKeyId, Value value )
{
if ( removedProperties != null && removedProperties.remove( propertyKeyId ) != null )
if ( removedProperties != 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;
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;
}
}
if ( addedProperties == null )
{
Expand All @@ -113,17 +121,20 @@ void addProperty( int propertyKeyId, Value value )
addedProperties.put( propertyKeyId, value );
}

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

private Iterator<StorageProperty> toPropertyIterator( VersionedHashMap<Integer,Value> propertyMap )
{
return propertyMap == null ? emptyIterator() :
Iterators.map(
return propertyMap == null ? Collections.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 )
public void nodeDoRemoveProperty( long nodeId, int propertyKeyId, Value removedValue )
{
getOrCreateNodeState( nodeId ).removeProperty( propertyKeyId );
getOrCreateNodeState( nodeId ).removeProperty( propertyKeyId, removedValue );
dataChanged();
}

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

@Override
public void graphDoRemoveProperty( int propertyKeyId )
public void graphDoRemoveProperty( int propertyKeyId, Value removedValue )
{
getOrCreateGraphState().removeProperty( propertyKeyId );
getOrCreateGraphState().removeProperty( propertyKeyId, removedValue );
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 );
ktx.txState().nodeDoRemoveProperty( node, propertyKey, existingValue );
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 );
ktx.txState().relationshipDoRemoveProperty( relationship, propertyKey, existingValue );
}

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 );
ktx.txState().graphDoRemoveProperty( propertyKey, existingValue );
}
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 );
state.removeProperty( 1, Values.of( "Hello" ) );

// 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 );
state.removeProperty( 4, Values.of( "a value" ) );
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 );
state.nodeDoRemoveProperty( 1L, propertyKeyId, prevValue );
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 );
state.relationshipDoRemoveProperty( 1L, propertyKeyId, prevValue );
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() );
transaction.nodeDoRemoveProperty( nodeId, prop2.propertyKeyId(), prop2.value() );
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() );
transaction.relationshipDoRemoveProperty( rel, prop3.propertyKeyId(), prop3.value() );
}
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() );
transaction.relationshipDoRemoveProperty( rel, prop3.propertyKeyId(), prop3.value() );
}
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() );
transaction.nodeDoRemoveProperty( node, prop3.propertyKeyId(), prop3.value() );
}
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() );
transaction.nodeDoRemoveProperty( node, prop3.propertyKeyId(), prop3.value() );
}
else
{
Expand Down

0 comments on commit 90072aa

Please sign in to comment.