Skip to content

Commit

Permalink
Layout bug, and increase index value types test coverage
Browse files Browse the repository at this point in the history
  • Loading branch information
@OliviaYtterbrink @Lojjs
OliviaYtterbrink authored and Lojjs committed Mar 5, 2018
1 parent 1e705c8 commit 1cec3f6
Show file tree
Hide file tree
Showing 7 changed files with 151 additions and 62 deletions.
20 changes: 10 additions & 10 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/DateLayout.java
View file
Expand Up @@ -58,7 +58,15 @@ public int minorVersion()
public int compare( DateSchemaKey o1, DateSchemaKey o2 )
{
int comparison = o1.compareValueTo( o2 );
return comparison != 0 ? comparison : Long.compare( o1.getEntityId(), o2.getEntityId() );
if ( comparison == 0 )
{
// This is a special case where we need also compare entityId to support inclusive/exclusive
if ( o1.getCompareId() || o2.getCompareId() )
{
return Long.compare( o1.getEntityId(), o2.getEntityId() );
}
}
return comparison;
}
};

Expand Down Expand Up @@ -87,15 +95,7 @@ public int minorVersion()
public int compare( DateSchemaKey o1, DateSchemaKey o2 )
{
int comparison = o1.compareValueTo( o2 );
if ( comparison == 0 )
{
// This is a special case where we need also compare entityId to support inclusive/exclusive
if ( o1.getCompareId() || o2.getCompareId() )
{
return Long.compare( o1.getEntityId(), o2.getEntityId() );
}
}
return comparison;
return comparison != 0 ? comparison : Long.compare( o1.getEntityId(), o2.getEntityId() );
}
};

Expand Down
62 changes: 52 additions & 10 deletions .../kernel/src/test/java/org/neo4j/kernel/api/index/IndexProviderCompatibilityTestSuite.java
View file
Expand Up @@ -57,13 +57,9 @@ public abstract class IndexProviderCompatibilityTestSuite
{
protected abstract SchemaIndexProvider createIndexProvider( PageCache pageCache, FileSystemAbstraction fs, File graphDbDir );

public abstract Iterable<Value> getSupportedValues();
public abstract boolean supportsSpatial();

protected List<Value> commonValues = Arrays.asList( Values.of( "string1" ), Values.of( 42 ) );
protected List<Value> temporalValues = Arrays.asList( DateValue.epochDate( 2 ), DateValue.epochDate( 5 ) );
protected List<Value> spatialValues = Arrays.asList(
Values.pointValue( CoordinateReferenceSystem.Cartesian, 0, 0 ),
Values.pointValue( CoordinateReferenceSystem.WGS84, 12.78, 56.7 ) );
public abstract boolean supportsTemporal();

public abstract static class Compatibility
{
Expand All @@ -75,7 +71,8 @@ public abstract static class Compatibility
protected SchemaIndexProvider indexProvider;
protected IndexDescriptor descriptor;
final IndexProviderCompatibilityTestSuite testSuite;
final List<NodeAndValue> allValues;
final List<NodeAndValue> valueSet1;
final List<NodeAndValue> valueSet2;

@Before
public void setup()
Expand All @@ -90,7 +87,22 @@ public Compatibility( IndexProviderCompatibilityTestSuite testSuite, IndexDescri
{
this.testSuite = testSuite;
this.descriptor = descriptor;
this.allValues = allValues( testSuite.getSupportedValues() );
this.valueSet1 = allValues(
testSuite.supportsSpatial(),
testSuite.supportsTemporal(),
Arrays.asList( Values.of( "string1" ), Values.of( 42 ) ),
Arrays.asList( DateValue.epochDate( 2 ), DateValue.epochDate( 5 ) ),
Arrays.asList( Values.pointValue( CoordinateReferenceSystem.Cartesian, 0, 0 ),
Values.pointValue( CoordinateReferenceSystem.WGS84, 12.78, 56.7 ) ) );

this.valueSet2 = allValues(
testSuite.supportsSpatial(),
testSuite.supportsTemporal(),
Arrays.asList( Values.of( "string2" ), Values.of( 1337 ) ),
Arrays.asList( DateValue.epochDate( 42 ), DateValue.epochDate( 1337 ) ),
Arrays.asList( Values.pointValue( CoordinateReferenceSystem.Cartesian, 10, 10 ),
Values.pointValue( CoordinateReferenceSystem.WGS84, 87.21, 7.65 ) ) );

pageCacheAndDependenciesRule = new PageCacheAndDependenciesRule( DefaultFileSystemRule::new, testSuite.getClass() );
}

Expand Down Expand Up @@ -119,14 +131,44 @@ protected void withPopulator( IndexPopulator populator, ThrowingConsumer<IndexPo
}
}

private static List<NodeAndValue> allValues( Iterable<Value> supportedValues )
List<IndexEntryUpdate<?>> updates( List<NodeAndValue> values )
{
return updates( values, 0 );
}

List<IndexEntryUpdate<?>> updates( List<NodeAndValue> values, long nodeIdOffset )
{
List<IndexEntryUpdate<?>> updates = new ArrayList<>();
values.forEach( entry -> updates.add( IndexEntryUpdate.add( nodeIdOffset + entry.nodeId, descriptor.schema(), entry.value ) ) );
return updates;
}

private static List<NodeAndValue> allValues( boolean supportsSpatial,
boolean supportsTemporal,
List<Value> common,
List<Value> temporal,
List<Value> spatial )
{
long nodeIds = 0;
List<NodeAndValue> result = new ArrayList<>();
for ( Value value : supportedValues )
for ( Value value : common )
{
result.add( new NodeAndValue( nodeIds++, value ) );
}
if ( supportsSpatial )
{
for ( Value value : spatial )
{
result.add( new NodeAndValue( nodeIds++, value ) );
}
}
if ( supportsTemporal )
{
for ( Value value : temporal )
{
result.add( new NodeAndValue( nodeIds++, value ) );
}
}
return result;
}

Expand Down
6 changes: 2 additions & 4 deletions ...ity/kernel/src/test/java/org/neo4j/kernel/api/index/SimpleIndexAccessorCompatibility.java
View file
Expand Up @@ -22,7 +22,6 @@
import org.junit.Ignore;
import org.junit.Test;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

Expand Down Expand Up @@ -127,13 +126,12 @@ public void testIndexSeekByPrefixOnNonStrings() throws Exception
public void shouldUpdateWithAllValues() throws Exception
{
// GIVEN
List<IndexEntryUpdate<?>> updates = new ArrayList<>();
allValues.forEach( entry -> updates.add( IndexEntryUpdate.add( entry.nodeId, descriptor.schema(), entry.value ) ) );
List<IndexEntryUpdate<?>> updates = updates( valueSet1 );
updateAndCommit( updates );

// then
int propertyKeyId = descriptor.schema().getPropertyId();
for ( NodeAndValue entry : allValues )
for ( NodeAndValue entry : valueSet1 )
{
List<Long> result = query( IndexQuery.exact( propertyKeyId, entry.value ) );
assertThat( result, equalTo( Collections.singletonList( entry.nodeId ) ) );
Expand Down
90 changes: 63 additions & 27 deletions ...ty/kernel/src/test/java/org/neo4j/kernel/api/index/SimpleIndexPopulatorCompatibility.java
View file
Expand Up @@ -23,12 +23,12 @@
import org.junit.Test;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.neo4j.collection.primitive.PrimitiveLongCollections;
import org.neo4j.collection.primitive.PrimitiveLongIterator;
import org.neo4j.helpers.collection.Iterables;
import org.neo4j.internal.kernel.api.IndexOrder;
import org.neo4j.internal.kernel.api.IndexQuery;
import org.neo4j.internal.kernel.api.schema.LabelSchemaDescriptor;
Expand All @@ -37,6 +37,7 @@
import org.neo4j.kernel.api.schema.index.IndexDescriptor;
import org.neo4j.kernel.api.schema.index.IndexDescriptorFactory;
import org.neo4j.kernel.configuration.Config;
import org.neo4j.kernel.impl.api.index.IndexUpdateMode;
import org.neo4j.kernel.impl.api.index.sampling.IndexSamplingConfig;
import org.neo4j.kernel.impl.index.schema.NodeValueIterator;
import org.neo4j.storageengine.api.schema.IndexReader;
Expand Down Expand Up @@ -68,7 +69,7 @@ public SimpleIndexPopulatorCompatibility(
super( testSuite, descriptor );
}

private final IndexSamplingConfig indexSamplingConfig = new IndexSamplingConfig( Config.defaults() );
final IndexSamplingConfig indexSamplingConfig = new IndexSamplingConfig( Config.defaults() );

@Test
public void shouldStorePopulationFailedForRetrievalFromProviderLater() throws Exception
Expand Down Expand Up @@ -146,7 +147,7 @@ public void shouldApplyUpdatesIdempotently() throws Exception
p.close( true );
} );

// then
// THEN
try ( IndexAccessor accessor = indexProvider.getOnlineAccessor( 17, descriptor, indexSamplingConfig ) )
{
try ( IndexReader reader = accessor.newReader() )
Expand All @@ -166,17 +167,12 @@ public void shouldPopulateWithAllValues() throws Exception
withPopulator( indexProvider.getPopulator( 17, descriptor, indexSamplingConfig ), p ->
{
p.create();

List<IndexEntryUpdate<LabelSchemaDescriptor>> updates = new ArrayList<>();
allValues.forEach( entry -> updates.add( IndexEntryUpdate.add( entry.nodeId, descriptor.schema(), entry.value ) ) );

p.add( updates );

p.add( updates( valueSet1 ) );
p.close( true );
} );

// then
assertHasAllValues();
// THEN
assertHasAllValues( valueSet1 );
}

@Test
Expand All @@ -187,9 +183,9 @@ public void shouldUpdateWithAllValuesDuringPopulation() throws Exception
{
p.create();

try ( IndexUpdater updater = p.newPopulatingUpdater( this::allValueLookup ) )
try ( IndexUpdater updater = p.newPopulatingUpdater( this::valueSet1Lookup ) )
{
for ( NodeAndValue entry : allValues )
for ( NodeAndValue entry : valueSet1 )
{
updater.process( IndexEntryUpdate.add( entry.nodeId, descriptor.schema(), entry.value ) );
}
Expand All @@ -198,13 +194,51 @@ public void shouldUpdateWithAllValuesDuringPopulation() throws Exception
p.close( true );
} );

// then
assertHasAllValues();
// THEN
assertHasAllValues( valueSet1 );
}

@Test
public void shouldPopulateAndUpdate() throws Exception
{
// GIVEN
withPopulator( indexProvider.getPopulator( 17, descriptor, indexSamplingConfig ), p ->
{
p.create();
p.add( updates( valueSet1 ) );
p.close( true );
} );

try ( IndexAccessor accessor = indexProvider.getOnlineAccessor( 17, descriptor, indexSamplingConfig ) )
{
// WHEN
try ( IndexUpdater updater = accessor.newUpdater( IndexUpdateMode.ONLINE ) )
{
List<IndexEntryUpdate<?>> updates = updates( valueSet2 );
for ( IndexEntryUpdate<?> update : updates )
{
updater.process( update );
}
}

// THEN
try ( IndexReader reader = accessor.newReader() )
{
int propertyKeyId = descriptor.schema().getPropertyId();
for ( NodeAndValue entry : Iterables.concat( valueSet1, valueSet2 ) )
{
NodeValueIterator nodes = new NodeValueIterator();
reader.query( nodes, IndexOrder.NONE, IndexQuery.exact( propertyKeyId, entry.value ) );
assertEquals( entry.nodeId, single( nodes, NO_SUCH_NODE ) );
}
}
accessor.close();
}
}

private Value allValueLookup( long nodeId, int propertyId )
private Value valueSet1Lookup( long nodeId, int propertyId )
{
for ( NodeAndValue x : allValues )
for ( NodeAndValue x : valueSet1 )
{
if ( x.nodeId == nodeId )
{
Expand All @@ -214,14 +248,14 @@ private Value allValueLookup( long nodeId, int propertyId )
return Values.NO_VALUE;
}

private void assertHasAllValues() throws IOException, IndexNotApplicableKernelException
private void assertHasAllValues( List<NodeAndValue> values ) throws IOException, IndexNotApplicableKernelException
{
try ( IndexAccessor accessor = indexProvider.getOnlineAccessor( 17, descriptor, indexSamplingConfig ) )
{
try ( IndexReader reader = accessor.newReader() )
{
int propertyKeyId = descriptor.schema().getPropertyId();
for ( NodeAndValue entry : allValues )
for ( NodeAndValue entry : values )
{
NodeValueIterator nodes = new NodeValueIterator();
reader.query( nodes, IndexOrder.NONE, IndexQuery.exact( propertyKeyId, entry.value ) );
Expand All @@ -244,14 +278,12 @@ public General( IndexProviderCompatibilityTestSuite testSuite )
public void shouldProvidePopulatorThatAcceptsDuplicateEntries() throws Exception
{
// when
IndexSamplingConfig indexSamplingConfig = new IndexSamplingConfig( Config.defaults() );
Value value = Values.of( "value1" );
long offset = valueSet1.size();
withPopulator( indexProvider.getPopulator( 17, descriptor, indexSamplingConfig ), p ->
{
p.create();
p.add( Arrays.asList(
IndexEntryUpdate.add( 1, descriptor.schema(), value ),
IndexEntryUpdate.add( 2, descriptor.schema(), value ) ) );
p.add( updates( valueSet1, 0 ) );
p.add( updates( valueSet1, offset ) );
p.close( true );
} );

Expand All @@ -260,8 +292,13 @@ public void shouldProvidePopulatorThatAcceptsDuplicateEntries() throws Exception
{
try ( IndexReader reader = accessor.newReader() )
{
PrimitiveLongIterator nodes = reader.query( IndexQuery.exact( 1, value ) );
assertEquals( asSet( 1L, 2L ), PrimitiveLongCollections.toSet( nodes ) );
int propertyKeyId = descriptor.schema().getPropertyId();
for ( NodeAndValue entry : valueSet1 )
{
NodeValueIterator nodes = new NodeValueIterator();
reader.query( nodes, IndexOrder.NONE, IndexQuery.exact( propertyKeyId, entry.value ) );
assertEquals( asSet( entry.nodeId, entry.nodeId + offset ), PrimitiveLongCollections.toSet( nodes ) );
}
}
accessor.close();
}
Expand All @@ -287,7 +324,6 @@ public void shouldProvidePopulatorThatEnforcesUniqueConstraints() throws Excepti
int nodeId1 = 1;
int nodeId2 = 2;

IndexSamplingConfig indexSamplingConfig = new IndexSamplingConfig( Config.defaults() );
withPopulator( indexProvider.getPopulator( 17, descriptor, indexSamplingConfig ), p ->
{
p.create();
Expand Down
12 changes: 8 additions & 4 deletions ...nel/src/test/java/org/neo4j/kernel/impl/api/index/inmemory/InMemoryIndexProviderTest.java
View file
Expand Up @@ -21,12 +21,10 @@

import java.io.File;

import org.neo4j.helpers.collection.Iterables;
import org.neo4j.io.fs.FileSystemAbstraction;
import org.neo4j.io.pagecache.PageCache;
import org.neo4j.kernel.api.index.IndexProviderCompatibilityTestSuite;
import org.neo4j.kernel.api.index.SchemaIndexProvider;
import org.neo4j.values.storable.Value;

public class InMemoryIndexProviderTest extends IndexProviderCompatibilityTestSuite
{
Expand All @@ -37,8 +35,14 @@ protected SchemaIndexProvider createIndexProvider( PageCache pageCache, FileSyst
}

@Override
public Iterable<Value> getSupportedValues()
public boolean supportsSpatial()
{
return Iterables.concat( commonValues, spatialValues, temporalValues );
return true;
}

@Override
public boolean supportsTemporal()
{
return true;
}
}

0 comments on commit 1cec3f6

Please sign in to comment.