From cb986a99300092343682ed9cfcfa22da16dd2920 Mon Sep 17 00:00:00 2001 From: Anton Persson Date: Tue, 22 Nov 2016 15:51:15 +0100 Subject: [PATCH] Split insert in IndexWriter into merge and put Taking inspiration from Map#merge. Removed ValueAmender (now ValueMerger) that could insert non unique keys, because this is not supported by the tree. --- .../java/org/neo4j/index/IndexWriter.java | 38 ++++++------- .../{ValueAmender.java => ValueMerger.java} | 14 ++--- .../{ValueAmenders.java => ValueMergers.java} | 26 ++++----- .../java/org/neo4j/index/gbptree/GBPTree.java | 13 +++-- .../index/gbptree/InternalTreeLogic.java | 35 ++++++------ .../org/neo4j/index/gbptree/GBPTreeTest.java | 12 ++--- .../index/gbptree/InternalTreeLogicTest.java | 53 +++++++++---------- 7 files changed, 95 insertions(+), 96 deletions(-) rename community/index/src/main/java/org/neo4j/index/{ValueAmender.java => ValueMerger.java} (66%) rename community/index/src/main/java/org/neo4j/index/{ValueAmenders.java => ValueMergers.java} (61%) diff --git a/community/index/src/main/java/org/neo4j/index/IndexWriter.java b/community/index/src/main/java/org/neo4j/index/IndexWriter.java index ec2bc999dcdaa..f9be5f8676ebc 100644 --- a/community/index/src/main/java/org/neo4j/index/IndexWriter.java +++ b/community/index/src/main/java/org/neo4j/index/IndexWriter.java @@ -22,43 +22,39 @@ import java.io.Closeable; import java.io.IOException; -import static org.neo4j.index.ValueAmenders.overwrite; - /** - * Able to {@link #insert(Object, Object, ValueAmender)} and {@link #remove(Object)} key/value pairs + * Able to {@link #merge(Object, Object, ValueMerger)} and {@link #remove(Object)} key/value pairs * into an {@link Index}. After all modifications have taken place the writer must be {@link #close() closed}, * typically using try-with-resource clause. * - * @param type of keys to insert/remove - * @param type of values to insert/removed + * @param type of keys + * @param type of values */ public interface IndexWriter extends Closeable { /** - * Defaults to {@link ValueAmenders#overwrite() overwriting values} for existing key. + * Associate given {@code key} with given {@code value}. + * Any existing {@code value} associated with {@code key} will be overwritten. * - * @param key key to insert. - * @param value value to insert for the {@code key}. + * @param key key to associate with value + * @param value value to associate with key * @throws IOException on index access error. - * - * @see #insert(Object, Object, ValueAmender) */ - default void insert( KEY key, VALUE value ) throws IOException - { - insert( key, value, overwrite() ); - } + void put( KEY key, VALUE value ) throws IOException; /** - * Inserts a key/value pair. In the event where {@code key} already exists the {@link ValueAmender} - * gets consulted, which can choose e.g. to insert a new key/value pair, overwrite or somehow modify - * the existing value. + * If the {@code key} doesn't already exist in the index the {@code key} will be added and the {@code value} + * associated with it. If the {@code key} already exists then its existing {@code value} will be merged with + * the given {@code value}, using the {@link ValueMerger}. If the {@link ValueMerger} returns a non-null + * value that value will be associated with the {@code key}, otherwise (if it returns {@code null}) nothing will + * be written. * - * @param key key to insert. - * @param value value to insert for the {@code key}. - * @param amender {@link ValueAmender} to consult if key already exists. + * @param key key for which to merge values. + * @param value value to merge with currently associated value for the {@code key}. + * @param valueMerger {@link ValueMerger} to consult if key already exists. * @throws IOException on index access error. */ - void insert( KEY key, VALUE value, ValueAmender amender ) throws IOException; + void merge( KEY key, VALUE value, ValueMerger valueMerger ) throws IOException; /** * Removes a key, returning it's associated value, if found. diff --git a/community/index/src/main/java/org/neo4j/index/ValueAmender.java b/community/index/src/main/java/org/neo4j/index/ValueMerger.java similarity index 66% rename from community/index/src/main/java/org/neo4j/index/ValueAmender.java rename to community/index/src/main/java/org/neo4j/index/ValueMerger.java index 9b85ef7ce8771..f5889f2177f9b 100644 --- a/community/index/src/main/java/org/neo4j/index/ValueAmender.java +++ b/community/index/src/main/java/org/neo4j/index/ValueMerger.java @@ -21,19 +21,19 @@ /** * Decides what to do when inserting key which already exists in index. Different implementations of - * {@link ValueAmender} can result in unique/non-unique indexes for example. + * {@link ValueMerger} can result in unique/non-unique indexes for example. * - * @param type of values to amend. + * @param type of values to merge. */ -public interface ValueAmender +public interface ValueMerger { /** - * Amends an existing value with a new value, returning potentially a combination of the two, or {@code null} - * if no amend was done effectively meaning that a new value should be inserted for that same key. + * Merge an existing value with a new value, returning potentially a combination of the two, or {@code null} + * if no merge was done effectively meaning that nothing should be written. * * @param value existing value * @param withValue new value - * @return {@code value}, now amended with {@code withValue}, or {@code null} if no amend was done. + * @return {@code value}, now merged with {@code withValue}, or {@code null} if no merge was done. */ - VALUE amend( VALUE value, VALUE withValue ); + VALUE merge( VALUE value, VALUE withValue ); } diff --git a/community/index/src/main/java/org/neo4j/index/ValueAmenders.java b/community/index/src/main/java/org/neo4j/index/ValueMergers.java similarity index 61% rename from community/index/src/main/java/org/neo4j/index/ValueAmenders.java rename to community/index/src/main/java/org/neo4j/index/ValueMergers.java index 4c04adc23e0fc..f827787256ebb 100644 --- a/community/index/src/main/java/org/neo4j/index/ValueAmenders.java +++ b/community/index/src/main/java/org/neo4j/index/ValueMergers.java @@ -20,47 +20,47 @@ package org.neo4j.index; /** - * Common {@link ValueAmender} implementations. + * Common {@link ValueMerger} implementations. */ -public class ValueAmenders +public class ValueMergers { @SuppressWarnings( "rawtypes" ) - private static final ValueAmender OVERWRITE = new ValueAmender() + private static final ValueMerger OVERWRITE = new ValueMerger() { @Override - public Object amend( Object value, Object withValue ) + public Object merge( Object value, Object withValue ) { return withValue; } }; @SuppressWarnings( "rawtypes" ) - private static final ValueAmender INSERT_NEW = new ValueAmender() + private static final ValueMerger KEEP_EXISTING = new ValueMerger() { @Override - public Object amend( Object value, Object withValue ) + public Object merge( Object value, Object withValue ) { return null; } }; /** - * @return {@link ValueAmender} which overwrites value for existing key when inserting. - * This makes an index have unique keys. + * @return {@link ValueMerger} which overwrites value for existing key when inserting. + * This merger guarantees unique keys in index. */ @SuppressWarnings( "unchecked" ) - public static ValueAmender overwrite() + public static ValueMerger overwrite() { return OVERWRITE; } /** - * @return {@link ValueAmender} which inserts new key/value even for existing keys. - * This makes an index have non-unique keys. + * @return {@link ValueMerger} which keeps existing key/value otherwise adds new key/value pair. + * This merger guarantees unique keys in index. */ @SuppressWarnings( "unchecked" ) - public static ValueAmender insertNew() + public static ValueMerger keepExisting() { - return INSERT_NEW; + return KEEP_EXISTING; } } diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java b/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java index f1bd5470332a0..7ea79844baae2 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java @@ -29,7 +29,8 @@ import org.neo4j.index.Hit; import org.neo4j.index.Index; import org.neo4j.index.IndexWriter; -import org.neo4j.index.ValueAmender; +import org.neo4j.index.ValueMerger; +import org.neo4j.index.ValueMergers; import org.neo4j.io.pagecache.PageCache; import org.neo4j.io.pagecache.PageCursor; import org.neo4j.io.pagecache.PagedFile; @@ -420,11 +421,17 @@ SingleIndexWriter take( long rootId, IndexWriter.Options options ) throws IOExce } @Override - public void insert( KEY key, VALUE value, ValueAmender amender ) throws IOException + public void put( KEY key, VALUE value ) throws IOException + { + merge( key, value, ValueMergers.overwrite() ); + } + + @Override + public void merge( KEY key, VALUE value, ValueMerger valueMerger ) throws IOException { cursor.next( rootId ); - SplitResult split = treeLogic.insert( cursor, key, value, amender, options, + SplitResult split = treeLogic.insert( cursor, key, value, valueMerger, options, stableGeneration, unstableGeneration ); if ( cursor.checkAndClearBoundsFlag() ) diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java b/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java index 7c15619e091f4..71627d7fd0c8d 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java @@ -22,7 +22,7 @@ import java.io.IOException; import org.neo4j.index.IndexWriter; -import org.neo4j.index.ValueAmender; +import org.neo4j.index.ValueMerger; import org.neo4j.io.pagecache.PageCursor; import static java.lang.Integer.max; @@ -108,19 +108,19 @@ class InternalTreeLogic * @param cursor {@link org.neo4j.io.pagecache.PageCursor} pinned to page where insertion is to be done. * @param key key to be inserted * @param value value to be associated with key - * @param amender {@link ValueAmender} for deciding what to do with existing keys + * @param valueMerger {@link ValueMerger} for deciding what to do with existing keys * @param options options for this insert * @param stableGeneration stable generation, i.e. generations <= this generation are considered stable. * @param unstableGeneration unstable generation, i.e. generation which is under development right now. * @return {@link SplitResult} from insert to be used caller. * @throws IOException on cursor failure */ - public SplitResult insert( PageCursor cursor, KEY key, VALUE value, ValueAmender amender, + public SplitResult insert( PageCursor cursor, KEY key, VALUE value, ValueMerger valueMerger, IndexWriter.Options options, int stableGeneration, int unstableGeneration ) throws IOException { if ( bTreeNode.isLeaf( cursor ) ) { - return insertInLeaf( cursor, key, value, amender, options, stableGeneration, unstableGeneration ); + return insertInLeaf( cursor, key, value, valueMerger, options, stableGeneration, unstableGeneration ); } int keyCount = bTreeNode.keyCount( cursor ); @@ -137,7 +137,8 @@ public SplitResult insert( PageCursor cursor, KEY key, VALUE value, ValueAm cursor.next( childId ); - SplitResult split = insert( cursor, key, value, amender, options, stableGeneration, unstableGeneration ); + SplitResult split = insert( cursor, key, value, valueMerger, options, stableGeneration, + unstableGeneration ); cursor.next( currentId ); @@ -293,12 +294,12 @@ private static int middle( int keyCountAfterInsert, float splitLeftChildSize ) * insertion. * @param key key to be inserted * @param value value to be associated with key - * @param amender {@link ValueAmender} for deciding what to do with existing keys + * @param valueMerger {@link ValueMerger} for deciding what to do with existing keys * @param options options for this insert * @return {@link SplitResult} from insert to be used caller. * @throws IOException on cursor failure */ - private SplitResult insertInLeaf( PageCursor cursor, KEY key, VALUE value, ValueAmender amender, + private SplitResult insertInLeaf( PageCursor cursor, KEY key, VALUE value, ValueMerger valueMerger, IndexWriter.Options options, int stableGeneration, int unstableGeneration ) throws IOException { int keyCount = bTreeNode.keyCount( cursor ); @@ -306,16 +307,15 @@ private SplitResult insertInLeaf( PageCursor cursor, KEY key, VALUE value, int pos = positionOf( search ); if ( isHit( search ) ) { - // this key already exists, what shall we do? ask the amender + // this key already exists, what shall we do? ask the valueMerger bTreeNode.valueAt( cursor, readValue, pos ); - VALUE amendedValue = amender.amend( readValue, value ); - if ( amendedValue != null ) + VALUE mergedValue = valueMerger.merge( readValue, value ); + if ( mergedValue != null ) { - // simple, just write the amended value right in there - bTreeNode.setValueAt( cursor, amendedValue, pos ); - return null; // No split has occurred + // simple, just write the merged value right in there + bTreeNode.setValueAt( cursor, mergedValue, pos ); } - // else fall-through to normal insert + return null; // No split has occurred } if ( keyCount < bTreeNode.leafMaxKeyCount() ) @@ -329,7 +329,7 @@ private SplitResult insertInLeaf( PageCursor cursor, KEY key, VALUE value, } // Overflow, split leaf - return splitLeaf( cursor, key, value, amender, keyCount, options, stableGeneration, unstableGeneration ); + return splitLeaf( cursor, key, value, keyCount, options, stableGeneration, unstableGeneration ); } /** @@ -338,14 +338,13 @@ private SplitResult insertInLeaf( PageCursor cursor, KEY key, VALUE value, * @param cursor cursor pointing into full (left) leaf that should be split in two. * @param newKey key to be inserted * @param newValue value to be inserted (in association with key) - * @param amender {@link ValueAmender} for deciding what to do with existing keys * @param keyCount number of keys in this leaf (it was already read anyway) * @param options options for this insert * @return {@link SplitResult} with necessary information to inform parent * @throws IOException if cursor.next( newRight ) fails */ - private SplitResult splitLeaf( PageCursor cursor, KEY newKey, VALUE newValue, ValueAmender amender, - int keyCount, IndexWriter.Options options, int stableGeneration, int unstableGeneration ) throws IOException + private SplitResult splitLeaf( PageCursor cursor, KEY newKey, VALUE newValue, int keyCount, + IndexWriter.Options options, int stableGeneration, int unstableGeneration ) throws IOException { // To avoid moving cursor between pages we do all operations on left node first. // Save data that needs transferring and then add it to right node. diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java index 0682dac9dfc93..cc59c61a31a0b 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java @@ -317,7 +317,7 @@ public void shouldSeeSimpleInsertions() throws Exception { for ( int i = 0; i < count; i++ ) { - writer.insert( new MutableLong( i ), new MutableLong( i ) ); + writer.put( new MutableLong( i ), new MutableLong( i ) ); } } @@ -353,7 +353,7 @@ public void shouldStayCorrectAfterRandomModifications() throws Exception { for ( Map.Entry entry : data.entrySet() ) { - writer.insert( entry.getKey(), entry.getValue() ); + writer.put( entry.getKey(), entry.getValue() ); } } @@ -421,7 +421,7 @@ public void shouldSplitCorrectly() throws Exception } while ( !seen.add( key.longValue() ) ); MutableLong value = new MutableLong( i ); - writer.insert( key, value ); + writer.put( key, value ); seen.add( key.longValue() ); } } @@ -545,7 +545,7 @@ public void shouldReadCorrectlyWhenConcurrentlyInserting() throws Throwable for ( int i = 0; i < groupCount; i++, inserted++ ) { MutableLong thing = new MutableLong( inserted ); - writer.insert( thing, thing ); + writer.put( thing, thing ); highestId.set( inserted ); } // Sleep a little in between update groups (transactions, sort of) @@ -586,10 +586,10 @@ private void randomlyModifyIndex( Index index, assertEquals( "For " + key, value, removedValue ); } else - { // insert + { // put MutableLong key = randomKey( random ); MutableLong value = randomKey( random ); - writer.insert( key, value ); + writer.put( key, value ); data.put( key, value ); } } diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/InternalTreeLogicTest.java b/community/index/src/test/java/org/neo4j/index/gbptree/InternalTreeLogicTest.java index 29fadc9cb670e..39682b805ffd6 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/InternalTreeLogicTest.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/InternalTreeLogicTest.java @@ -26,27 +26,22 @@ import java.io.IOException; -import org.neo4j.index.ValueAmender; -import org.neo4j.index.ValueAmenders; +import org.neo4j.index.ValueMerger; +import org.neo4j.index.ValueMergers; import org.neo4j.test.rule.RandomRule; -import static org.hamcrest.CoreMatchers.allOf; -import static org.hamcrest.CoreMatchers.anyOf; import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; - import static org.neo4j.index.IndexWriter.Options.DEFAULTS; -import static org.neo4j.index.ValueAmenders.insertNew; -import static org.neo4j.index.ValueAmenders.overwrite; +import static org.neo4j.index.ValueMergers.overwrite; public class InternalTreeLogicTest { - private static final ValueAmender ADDER = (base,add) -> { + private static final ValueMerger ADDER = (base,add) -> { base.add( add.longValue() ); return base; }; @@ -148,13 +143,13 @@ public void modifierMustSplitWhenInsertingMiddleOfFullLeaf() throws Exception // given for ( int i = 0; i < maxKeyCount; i++ ) { - long key = i % 2 == 0 ? i / 2 : maxKeyCount - i / 2; + long key = i % 2 == 0 ? i : maxKeyCount * 2 - i; insert( key, key ); } // then - long middle = maxKeyCount / 2; - assertNotNull( insert( middle, middle, insertNew() ) ); + long middle = maxKeyCount; + assertNotNull( insert( middle, middle ) ); } @Test @@ -482,10 +477,10 @@ public void modifierMustProduceConsistentTreeWithRandomInserts() throws Exceptio consistencyChecker.check( cursor ); } - /* TEST AMENDER */ + /* TEST VALUE MERGER */ @Test - public void modifierMustOverwriteWithOverwriteAmender() throws Exception + public void modifierMustOverwriteWithOverwriteMerger() throws Exception { // given long key = random.nextLong(); @@ -494,7 +489,7 @@ public void modifierMustOverwriteWithOverwriteAmender() throws Exception // when long secondValue = random.nextLong(); - insert( key, secondValue, ValueAmenders.overwrite() ); + insert( key, secondValue, ValueMergers.overwrite() ); // then assertThat( node.keyCount( cursor ), is( 1 ) ); @@ -502,26 +497,28 @@ public void modifierMustOverwriteWithOverwriteAmender() throws Exception } @Test - public void modifierMustInsertNewWithInsertNewAmender() throws Exception + public void modifierMustKeepExistingWithKeepExistingMerger() throws Exception { // given long key = random.nextLong(); long firstValue = random.nextLong(); - insert( key, firstValue ); + insert( key, firstValue, ValueMergers.keepExisting() ); + assertThat( node.keyCount( cursor ), is( 1 ) ); + Long actual = valueAt( 0 ); + assertThat( actual, is( firstValue ) ); // when long secondValue = random.nextLong(); - insert( key, secondValue, ValueAmenders.insertNew() ); + insert( key, secondValue, ValueMergers.keepExisting() ); // then - assertThat( node.keyCount( cursor ), is( 2 ) ); - Long actualFirst = valueAt( 0 ); - assertThat( actualFirst, anyOf( is( firstValue ), is( secondValue ) ) ); - assertThat( valueAt( 1 ), allOf( not( is( actualFirst ) ), anyOf( is( firstValue ), is( secondValue ) ) ) ); + assertThat( node.keyCount( cursor ), is( 1 ) ); + actual = valueAt( 0 ); + assertThat( actual, is( firstValue ) ); } @Test - public void shouldAmendValueInRootLeaf() throws Exception + public void shouldMergeValueInRootLeaf() throws Exception { // GIVEN long key = 10; @@ -542,7 +539,7 @@ public void shouldAmendValueInRootLeaf() throws Exception } @Test - public void shouldAmendValueInLeafLeftOfParentKey() throws Exception + public void shouldMergeValueInLeafLeftOfParentKey() throws Exception { // GIVEN SplitResult split = null; @@ -569,7 +566,7 @@ public void shouldAmendValueInLeafLeftOfParentKey() throws Exception } @Test - public void shouldAmendValueInLeafAtParentKey() throws Exception + public void shouldMergeValueInLeafAtParentKey() throws Exception { // GIVEN SplitResult split = null; @@ -596,7 +593,7 @@ public void shouldAmendValueInLeafAtParentKey() throws Exception } @Test - public void shouldAmendValueInLeafBetweenTwoParentKeys() throws Exception + public void shouldMergeValueInLeafBetweenTwoParentKeys() throws Exception { // GIVEN long rootId = -1; @@ -681,12 +678,12 @@ private SplitResult insert( long key, long value ) throws IOExcepti return insert( key, value, overwrite() ); } - private SplitResult insert( long key, long value, ValueAmender amender ) + private SplitResult insert( long key, long value, ValueMerger valueMerger ) throws IOException { insertKey.setValue( key ); insertValue.setValue( value ); - return treeLogic.insert( cursor, insertKey, insertValue, amender, DEFAULTS, + return treeLogic.insert( cursor, insertKey, insertValue, valueMerger, DEFAULTS, STABLE_GENERATION, UNSTABLE_GENERATION ); }