Skip to content

Commit

Permalink
Fixes spatial inequalities for non-indexed and txstate cases
Browse files Browse the repository at this point in the history
  • Loading branch information
@craigtaverner
craigtaverner committed Aug 24, 2018
1 parent 9704303 commit 0ccdd2c
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 49 deletions.
6 changes: 4 additions & 2 deletions community/cypher/runtime-util/src/main/java/org/neo4j/cypher/operations/CypherBoolean.java
View file
Expand Up @@ -233,7 +233,8 @@ else if ( lhs instanceof BooleanValue && rhs instanceof BooleanValue )
} }
else if ( lhs instanceof PointValue && rhs instanceof PointValue ) else if ( lhs instanceof PointValue && rhs instanceof PointValue )
{ {
return lessThanOrEqual( AnyValues.TERNARY_COMPARATOR.ternaryCompare( lhs, rhs ) ); // The ternary comparator cannot handle the '=' part of the >= and <= cases, so we need to switch to the within function
return ((PointValue) lhs).withinRange( null, false, (PointValue) rhs, true ) ? TRUE : FALSE;
} }
else if ( lhs instanceof DateValue && rhs instanceof DateValue ) else if ( lhs instanceof DateValue && rhs instanceof DateValue )
{ {
Expand Down Expand Up @@ -329,7 +330,8 @@ else if ( lhs instanceof BooleanValue && rhs instanceof BooleanValue )
} }
else if ( lhs instanceof PointValue && rhs instanceof PointValue ) else if ( lhs instanceof PointValue && rhs instanceof PointValue )
{ {
return greaterThanOrEqual( AnyValues.TERNARY_COMPARATOR.ternaryCompare( lhs, rhs ) ); // The ternary comparator cannot handle the '=' part of the >= and <= cases, so we need to switch to the within function
return ((PointValue) lhs).withinRange( (PointValue) rhs, true, null, false ) ? TRUE : FALSE;
} }
else if ( lhs instanceof DateValue && rhs instanceof DateValue ) else if ( lhs instanceof DateValue && rhs instanceof DateValue )
{ {
Expand Down
17 changes: 17 additions & 0 deletions community/kernel-api/src/main/java/org/neo4j/internal/kernel/api/IndexQuery.java
View file
Expand Up @@ -393,6 +393,14 @@ public boolean toInclusive()
{ {
return toInclusive; return toInclusive;
} }

/**
* @return true if the order defined for this type can also be relied on for bounds comparisons.
*/
public boolean isRegularOrder()
{
return true;
}
} }


public static final class GeometryRangePredicate extends RangePredicate<PointValue> public static final class GeometryRangePredicate extends RangePredicate<PointValue>
Expand Down Expand Up @@ -437,6 +445,15 @@ public PointValue to()
{ {
return to; return to;
} }

/**
* The order defined for spatial types cannot be used for bounds comparisons.
* @return false
*/
public boolean isRegularOrder()
{
return false;
}
} }


public static final class NumberRangePredicate extends RangePredicate<NumberValue> public static final class NumberRangePredicate extends RangePredicate<NumberValue>
Expand Down
9 changes: 2 additions & 7 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultNodeValueIndexCursor.java
View file
Expand Up @@ -287,18 +287,13 @@ private void rangeQuery( IndexDescriptor descriptor, IndexQuery.RangePredicate<?


if ( needsValues ) if ( needsValues )
{ {
ReadableDiffSets<NodeWithPropertyValues> changes = ReadableDiffSets<NodeWithPropertyValues> changes = TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( txState, descriptor, predicate );
TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( txState, descriptor, valueGroup, predicate.fromValue(),
predicate.fromInclusive(), predicate.toValue(), predicate.toInclusive() );
addedWithValues = changes.getAdded().iterator(); addedWithValues = changes.getAdded().iterator();
removed = removed( txState, changes.getRemoved() ); removed = removed( txState, changes.getRemoved() );
} }
else else
{ {
LongDiffSets changes = TxStateIndexChanges.indexUpdatesForRangeSeek( txState, LongDiffSets changes = TxStateIndexChanges.indexUpdatesForRangeSeek( txState, descriptor, predicate );
descriptor, valueGroup,
predicate.fromValue(), predicate.fromInclusive(),
predicate.toValue(), predicate.toInclusive() );
added = changes.augment( ImmutableEmptyLongIterator.INSTANCE ); added = changes.augment( ImmutableEmptyLongIterator.INSTANCE );
removed = removed( txState, changes ); removed = removed( txState, changes );
} }
Expand Down
61 changes: 30 additions & 31 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/TxStateIndexChanges.java
View file
Expand Up @@ -21,7 +21,6 @@


import org.eclipse.collections.impl.UnmodifiableMap; import org.eclipse.collections.impl.UnmodifiableMap;


import java.util.Collection;
import java.util.Map; import java.util.Map;
import java.util.NavigableMap; import java.util.NavigableMap;


Expand All @@ -34,7 +33,6 @@
import org.neo4j.storageengine.api.txstate.ReadableTransactionState; import org.neo4j.storageengine.api.txstate.ReadableTransactionState;
import org.neo4j.values.storable.TextValue; import org.neo4j.values.storable.TextValue;
import org.neo4j.values.storable.Value; import org.neo4j.values.storable.Value;
import org.neo4j.values.storable.ValueGroup;
import org.neo4j.values.storable.ValueTuple; import org.neo4j.values.storable.ValueTuple;
import org.neo4j.values.storable.Values; import org.neo4j.values.storable.Values;


Expand Down Expand Up @@ -144,9 +142,10 @@ static LongDiffSets indexUpdatesForSeek( ReadableTransactionState txState, Index
return LongDiffSets.EMPTY; return LongDiffSets.EMPTY;
} }


static LongDiffSets indexUpdatesForRangeSeek( ReadableTransactionState txState, IndexDescriptor descriptor, ValueGroup valueGroup, Value lower, static LongDiffSets indexUpdatesForRangeSeek( ReadableTransactionState txState, IndexDescriptor descriptor, IndexQuery.RangePredicate<?> predicate )
boolean includeLower, Value upper, boolean includeUpper )
{ {
Value lower = predicate.fromValue();
Value upper = predicate.toValue();
if ( lower == null || upper == null ) if ( lower == null || upper == null )
{ {
throw new IllegalStateException( "Use Values.NO_VALUE to encode the lack of a bound" ); throw new IllegalStateException( "Use Values.NO_VALUE to encode the lack of a bound" );
Expand All @@ -166,46 +165,48 @@ static LongDiffSets indexUpdatesForRangeSeek( ReadableTransactionState txState,


if ( lower == NO_VALUE ) if ( lower == NO_VALUE )
{ {
selectedLower = ValueTuple.of( Values.minValue( valueGroup, upper ) ); selectedLower = ValueTuple.of( Values.minValue( predicate.valueGroup(), upper ) );
selectedIncludeLower = true; selectedIncludeLower = true;
} }
else else
{ {
selectedLower = ValueTuple.of( lower ); selectedLower = ValueTuple.of( lower );
selectedIncludeLower = includeLower; selectedIncludeLower = predicate.fromInclusive();
} }


if ( upper == NO_VALUE ) if ( upper == NO_VALUE )
{ {
selectedUpper = ValueTuple.of( Values.maxValue( valueGroup, lower ) ); selectedUpper = ValueTuple.of( Values.maxValue( predicate.valueGroup(), lower ) );
selectedIncludeUpper = false; selectedIncludeUpper = false;
} }
else else
{ {
selectedUpper = ValueTuple.of( upper ); selectedUpper = ValueTuple.of( upper );
selectedIncludeUpper = includeUpper; selectedIncludeUpper = predicate.toInclusive();
} }


return indexUpdatesForRangeSeek( sortedUpdates, selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper );
}

private static LongDiffSets indexUpdatesForRangeSeek( NavigableMap<ValueTuple,? extends LongDiffSets> sortedUpdates, ValueTuple lower, boolean includeLower,
ValueTuple upper, boolean includeUpper )
{
MutableLongDiffSetsImpl diffs = new MutableLongDiffSetsImpl(); MutableLongDiffSetsImpl diffs = new MutableLongDiffSetsImpl();


Collection<? extends LongDiffSets> inRange = sortedUpdates.subMap( lower, includeLower, upper, includeUpper ).values(); Map<ValueTuple,? extends LongDiffSets> inRange = sortedUpdates.subMap( selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper );
for ( LongDiffSets diffForSpecificValue : inRange ) for ( Map.Entry<ValueTuple,? extends LongDiffSets> entry : inRange.entrySet() )
{ {
diffs.addAll( diffForSpecificValue.getAdded() ); ValueTuple values = entry.getKey();
diffs.removeAll( diffForSpecificValue.getRemoved() ); LongDiffSets diffForSpecificValue = entry.getValue();
// The TreeMap cannot perfectly order multi-dimensional types (spatial) and need additional filtering out false positives
if ( predicate.isRegularOrder() || predicate.acceptsValue( values.getOnlyValue() ) )
{
diffs.addAll( diffForSpecificValue.getAdded() );
diffs.removeAll( diffForSpecificValue.getRemoved() );
}
} }
return diffs; return diffs;
} }


static ReadableDiffSets<NodeWithPropertyValues> indexUpdatesWithValuesForRangeSeek( ReadableTransactionState txState, IndexDescriptor descriptor, static ReadableDiffSets<NodeWithPropertyValues> indexUpdatesWithValuesForRangeSeek( ReadableTransactionState txState, IndexDescriptor descriptor,
ValueGroup valueGroup, Value lower, boolean includeLower, Value upper, boolean includeUpper ) IndexQuery.RangePredicate<?> predicate )
{ {
Value lower = predicate.fromValue();
Value upper = predicate.toValue();
if ( lower == null || upper == null ) if ( lower == null || upper == null )
{ {
throw new IllegalStateException( "Use Values.NO_VALUE to encode the lack of a bound" ); throw new IllegalStateException( "Use Values.NO_VALUE to encode the lack of a bound" );
Expand All @@ -225,32 +226,26 @@ static ReadableDiffSets<NodeWithPropertyValues> indexUpdatesWithValuesForRangeSe


if ( lower == NO_VALUE ) if ( lower == NO_VALUE )
{ {
selectedLower = ValueTuple.of( Values.minValue( valueGroup, upper ) ); selectedLower = ValueTuple.of( Values.minValue( predicate.valueGroup(), upper ) );
selectedIncludeLower = true; selectedIncludeLower = true;
} }
else else
{ {
selectedLower = ValueTuple.of( lower ); selectedLower = ValueTuple.of( lower );
selectedIncludeLower = includeLower; selectedIncludeLower = predicate.fromInclusive();
} }


if ( upper == NO_VALUE ) if ( upper == NO_VALUE )
{ {
selectedUpper = ValueTuple.of( Values.maxValue( valueGroup, lower ) ); selectedUpper = ValueTuple.of( Values.maxValue( predicate.valueGroup(), lower ) );
selectedIncludeUpper = false; selectedIncludeUpper = false;
} }
else else
{ {
selectedUpper = ValueTuple.of( upper ); selectedUpper = ValueTuple.of( upper );
selectedIncludeUpper = includeUpper; selectedIncludeUpper = predicate.toInclusive();
} }


return indexUpdatesWithValuesForRangeSeek( sortedUpdates, selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper );
}

private static ReadableDiffSets<NodeWithPropertyValues> indexUpdatesWithValuesForRangeSeek( NavigableMap<ValueTuple,? extends LongDiffSets> sortedUpdates,
ValueTuple selectedLower, boolean selectedIncludeLower, ValueTuple selectedUpper, boolean selectedIncludeUpper )
{
DiffSets<NodeWithPropertyValues> diffs = new DiffSets<>(); DiffSets<NodeWithPropertyValues> diffs = new DiffSets<>();


Map<ValueTuple,? extends LongDiffSets> inRange = sortedUpdates.subMap( selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper ); Map<ValueTuple,? extends LongDiffSets> inRange = sortedUpdates.subMap( selectedLower, selectedIncludeLower, selectedUpper, selectedIncludeUpper );
Expand All @@ -260,8 +255,12 @@ private static ReadableDiffSets<NodeWithPropertyValues> indexUpdatesWithValuesFo
Value[] valuesArray = values.getValues(); Value[] valuesArray = values.getValues();
LongDiffSets diffForSpecificValue = entry.getValue(); LongDiffSets diffForSpecificValue = entry.getValue();


diffForSpecificValue.getAdded().each( nodeId -> diffs.add( new NodeWithPropertyValues( nodeId, valuesArray ) ) ); // The TreeMap cannot perfectly order multi-dimensional types (spatial) and need additional filtering out false positives
diffForSpecificValue.getRemoved().each( nodeId -> diffs.remove( new NodeWithPropertyValues( nodeId, valuesArray ) ) ); if ( predicate.isRegularOrder() || predicate.acceptsValue( values.getOnlyValue() ) )
{
diffForSpecificValue.getAdded().each( nodeId -> diffs.add( new NodeWithPropertyValues( nodeId, valuesArray ) ) );
diffForSpecificValue.getRemoved().each( nodeId -> diffs.remove( new NodeWithPropertyValues( nodeId, valuesArray ) ) );
}
} }
return diffs; return diffs;
} }
Expand Down
5 changes: 2 additions & 3 deletions community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/TxStateIndexChangesTest.java
View file
Expand Up @@ -217,10 +217,9 @@ private DynamicTest rangeTest( ReadableTransactionState state, Value lo, boolean
{ {
return DynamicTest.dynamicTest( String.format( "range seek: lo=%s (incl: %s), hi=%s (incl: %s)", lo, includeLo, hi, includeHi ), () -> return DynamicTest.dynamicTest( String.format( "range seek: lo=%s (incl: %s), hi=%s (incl: %s)", lo, includeLo, hi, includeHi ), () ->
{ {
final LongDiffSets diffSets = final LongDiffSets diffSets = TxStateIndexChanges.indexUpdatesForRangeSeek( state, index, IndexQuery.range( -1, lo, includeLo, hi, includeHi ) );
TxStateIndexChanges.indexUpdatesForRangeSeek( state, index, ValueGroup.NUMBER, lo, includeLo, hi, includeHi );
final ReadableDiffSets<NodeWithPropertyValues> diffSets2 = final ReadableDiffSets<NodeWithPropertyValues> diffSets2 =
TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( state, index, ValueGroup.NUMBER, lo, includeLo, hi, includeHi ); TxStateIndexChanges.indexUpdatesWithValuesForRangeSeek( state, index, IndexQuery.range( -1, lo, includeLo, hi, includeHi ) );


final LongSet expectedNodeIds = LongSets.immutable.ofAll( Arrays.stream( expected ).mapToLong( NodeWithPropertyValues::getNodeId ) ); final LongSet expectedNodeIds = LongSets.immutable.ofAll( Arrays.stream( expected ).mapToLong( NodeWithPropertyValues::getNodeId ) );
assertEquals( expectedNodeIds, diffSets.getAdded() ); assertEquals( expectedNodeIds, diffSets.getAdded() );
Expand Down
11 changes: 8 additions & 3 deletions community/values/src/main/java/org/neo4j/values/storable/PointValue.java
View file
Expand Up @@ -171,9 +171,14 @@ Comparison unsafeTernaryCompareTo( Value otherValue )
for ( int i = 0; i < coordinate.length; i++ ) for ( int i = 0; i < coordinate.length; i++ )
{ {
int cmpVal = Double.compare( this.coordinate[i], other.coordinate[i] ); int cmpVal = Double.compare( this.coordinate[i], other.coordinate[i] );
if ( cmpVal != 0 && cmpVal != result ) if ( cmpVal == 0 && result != 0 )
{ {
if ( (cmpVal < 0 && result > 0) || (cmpVal > 0 && result < 0) ) // Equal on one dimension, but not others, is not defined
return Comparison.UNDEFINED;
}
if ( cmpVal != result )
{
if ( (cmpVal < 0 && result > 0) || (cmpVal > 0 && result < 0) || (i > 0 && result == 0) )
{ {
return Comparison.UNDEFINED; return Comparison.UNDEFINED;
} }
Expand Down Expand Up @@ -278,7 +283,7 @@ public boolean withinRange( PointValue lower, boolean includeLower, PointValue u
{ {
if ( lower == null && upper == null ) if ( lower == null && upper == null )
{ {
return false; return true;
} }
if ( (lower != null) && (this.crs.getCode() != lower.crs.getCode() || this.coordinate.length != lower.coordinate.length) ) if ( (lower != null) && (this.crs.getCode() != lower.crs.getCode() || this.coordinate.length != lower.coordinate.length) )
{ {
Expand Down
47 changes: 47 additions & 0 deletions community/values/src/test/java/org/neo4j/values/storable/PointTest.java
View file
Expand Up @@ -22,9 +22,11 @@
import org.hamcrest.CoreMatchers; import org.hamcrest.CoreMatchers;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;


import org.neo4j.values.Comparison;
import org.neo4j.values.utils.InvalidValuesArgumentException; import org.neo4j.values.utils.InvalidValuesArgumentException;


import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertThrows;
Expand Down Expand Up @@ -83,6 +85,51 @@ void shouldHaveValueGroup()
assertNotNull( pointValue( WGS84, 1, 2 ).valueGroup() ); assertNotNull( pointValue( WGS84, 1, 2 ).valueGroup() );
} }


//-------------------------------------------------------------
// Comparison tests

@Test
public void shouldCompareTwoPoints()
{
assertThat( "Two identical points should be equal", pointValue( Cartesian, 1, 2 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 0 ) );
assertThat( "Different CRS should compare CRS codes", pointValue( Cartesian, 1, 2 ).compareTo( pointValue( WGS84, 1, 2 ) ), equalTo( 1 ) );
assertThat( "Point greater on both dimensions is greater", pointValue( Cartesian, 2, 3 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) );
assertThat( "Point greater on first dimensions is greater", pointValue( Cartesian, 2, 2 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) );
assertThat( "Point greater on second dimensions is greater", pointValue( Cartesian, 1, 3 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) );
assertThat( "Point smaller on both dimensions is smaller", pointValue( Cartesian, 0, 1 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) );
assertThat( "Point smaller on first dimensions is smaller", pointValue( Cartesian, 0, 2 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) );
assertThat( "Point smaller on second dimensions is smaller", pointValue( Cartesian, 1, 1 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) );
assertThat( "Point greater on first and smaller on second dimensions is greater",
pointValue( Cartesian, 2, 1 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( 1 ) );
assertThat( "Point smaller on first and greater on second dimensions is smaller",
pointValue( Cartesian, 0, 3 ).compareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( -1 ) );
}

@Test
public void shouldTernaryCompareTwoPoints()
{
assertThat( "Two identical points should be equal", pointValue( Cartesian, 1, 2 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ),
equalTo( Comparison.EQUAL ) );
assertThat( "Different CRS should be incomparable", pointValue( Cartesian, 1, 2 ).unsafeTernaryCompareTo( pointValue( WGS84, 1, 2 ) ),
equalTo( Comparison.UNDEFINED ) );
assertThat( "Point greater on both dimensions is greater", pointValue( Cartesian, 2, 3 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ),
equalTo( Comparison.GREATER_THAN ) );
assertThat( "Point greater on first dimensions is UNDEFINED", pointValue( Cartesian, 2, 2 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ),
equalTo( Comparison.UNDEFINED ) );
assertThat( "Point greater on second dimensions is UNDEFINED", pointValue( Cartesian, 1, 3 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ),
equalTo( Comparison.UNDEFINED ) );
assertThat( "Point smaller on both dimensions is smaller", pointValue( Cartesian, 0, 1 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ),
equalTo( Comparison.SMALLER_THAN ) );
assertThat( "Point smaller on first dimensions is UNDEFINED", pointValue( Cartesian, 0, 2 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ),
equalTo( Comparison.UNDEFINED ) );
assertThat( "Point smaller on second dimensions is UNDEFINED", pointValue( Cartesian, 1, 1 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ),
equalTo( Comparison.UNDEFINED ) );
assertThat( "Point greater on first and smaller on second dimensions is UNDEFINED",
pointValue( Cartesian, 2, 1 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( Comparison.UNDEFINED ) );
assertThat( "Point smaller on first and greater on second dimensions is UNDEFINED",
pointValue( Cartesian, 0, 3 ).unsafeTernaryCompareTo( pointValue( Cartesian, 1, 2 ) ), equalTo( Comparison.UNDEFINED ) );
}

//------------------------------------------------------------- //-------------------------------------------------------------
// Parser tests // Parser tests


Expand Down

0 comments on commit 0ccdd2c

Please sign in to comment.