Skip to content

Commit

Permalink
Fix some review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
henriknyman committed Sep 7, 2017
1 parent f769649 commit 712ab7c
Show file tree
Hide file tree
Showing 8 changed files with 50 additions and 60 deletions.
Expand Up @@ -179,7 +179,8 @@ case class Top1WithTiesPipe(source: Pipe, sortDescription: List[ColumnOrder])
val comparison = lessThan.compare(next, best) val comparison = lessThan.compare(next, best)
if (comparison < 0) { // Found a new best if (comparison < 0) { // Found a new best
best = next best = next
matchingRows = init(next) matchingRows.clear()
matchingRows += next._2
} }


if (comparison == 0) { // Found a tie if (comparison == 0) { // Found a tie
Expand Down
Expand Up @@ -40,7 +40,7 @@ class Top1WithTiesPipeTest extends CypherFunSuite {
sortPipe.createResults(QueryStateHelper.emptyWithValueSerialization).toList should beEquivalentTo(List(Map("x" -> "A"))) sortPipe.createResults(QueryStateHelper.emptyWithValueSerialization).toList should beEquivalentTo(List(Map("x" -> "A")))
} }


test("three ties for the first place are all returned") { test("two ties for the first place are all returned") {
val input = List( val input = List(
Map("x" -> 1, "y" -> 1), Map("x" -> 1, "y" -> 1),
Map("x" -> 1, "y" -> 2), Map("x" -> 1, "y" -> 2),
Expand Down
Expand Up @@ -31,19 +31,15 @@


public class DefaultTopTableTest public class DefaultTopTableTest
{ {
private static Long[] testValues = new Long[] { private static Long[] testValues = new Long[]{7L, 4L, 5L, 0L, 3L, 4L, 8L, 6L, 1L, 9L, 2L};
7L, 4L, 5L, 0L, 3L, 4L, 8L, 6L, 1L, 9L, 2L
};


private static long[] expectedValues = new long[] { private static long[] expectedValues = new long[]{0L, 1L, 2L, 3L, 4L, 4L, 5L, 6L, 7L, 8L, 9L};
0L, 1L, 2L, 3L, 4L, 4L, 5L, 6L, 7L, 8L, 9L
};


@Rule @Rule
public final ExpectedException exception = ExpectedException.none(); public final ExpectedException exception = ExpectedException.none();


@Test @Test
public void shouldHandleAddingMoreValuesThenCapacity() public void shouldHandleAddingMoreValuesThanCapacity()
{ {
DefaultTopTable table = new DefaultTopTable( 7 ); DefaultTopTable table = new DefaultTopTable( 7 );
for ( Long i : testValues ) for ( Long i : testValues )
Expand Down
Expand Up @@ -31,7 +31,7 @@ case class SortSlottedPipe(source: Pipe, orderBy: Seq[ColumnOrder], pipelineInfo
assert(orderBy.nonEmpty) assert(orderBy.nonEmpty)


private val comparator = orderBy private val comparator = orderBy
.map(ExecutionContextOrdering(_).comparator) .map(ExecutionContextOrdering.comparator(_))
.reduceLeft[Comparator[ExecutionContext]]((a, b) => a.thenComparing(b)) .reduceLeft[Comparator[ExecutionContext]]((a, b) => a.thenComparing(b))


override protected def internalCreateResults(input: Iterator[ExecutionContext], state: QueryState): Iterator[ExecutionContext] = { override protected def internalCreateResults(input: Iterator[ExecutionContext], state: QueryState): Iterator[ExecutionContext] = {
Expand All @@ -41,8 +41,8 @@ case class SortSlottedPipe(source: Pipe, orderBy: Seq[ColumnOrder], pipelineInfo
} }
} }


case class ExecutionContextOrdering(order: ColumnOrder) { object ExecutionContextOrdering {
val comparator: scala.Ordering[ExecutionContext] = order.slot match { def comparator(order: ColumnOrder): scala.Ordering[ExecutionContext] = order.slot match {
case LongSlot(offset, _, _, _) => case LongSlot(offset, _, _, _) =>
new scala.Ordering[ExecutionContext] { new scala.Ordering[ExecutionContext] {
override def compare(a: ExecutionContext, b: ExecutionContext): Int = { override def compare(a: ExecutionContext, b: ExecutionContext): Int = {
Expand Down
Expand Up @@ -38,12 +38,8 @@ abstract class TopSlottedPipe(source: Pipe, orderBy: Seq[ColumnOrder])
extends PipeWithSource(source) { extends PipeWithSource(source) {


protected val comparator = orderBy protected val comparator = orderBy
.map(ExecutionContextOrdering(_).comparator) .map(ExecutionContextOrdering.comparator(_))
.reduceLeft[Comparator[ExecutionContext]]((a, b) => a.thenComparing(b)) .reduceLeft[Comparator[ExecutionContext]]((a, b) => a.thenComparing(b))

def binarySearch(array: Array[ExecutionContext], comparator: Comparator[ExecutionContext])(key: ExecutionContext) = {
java.util.Arrays.binarySearch(array.asInstanceOf[Array[ExecutionContext]], key, comparator)
}
} }


case class TopNSlottedPipe(source: Pipe, orderBy: Seq[ColumnOrder], countExpression: Expression) case class TopNSlottedPipe(source: Pipe, orderBy: Seq[ColumnOrder], countExpression: Expression)
Expand Down Expand Up @@ -128,7 +124,8 @@ case class Top1WithTiesSlottedPipe(source: Pipe, orderBy: List[ColumnOrder])
val comparison = comparator.compare(ctx, best) val comparison = comparator.compare(ctx, best)
if (comparison < 0) { // Found a new best if (comparison < 0) { // Found a new best
best = ctx best = ctx
matchingRows = init(ctx) matchingRows.clear()
matchingRows += ctx
} }


if (comparison == 0) { // Found a tie if (comparison == 0) { // Found a tie
Expand Down
Expand Up @@ -32,13 +32,9 @@


public class DefaultComparatorTopTableTest public class DefaultComparatorTopTableTest
{ {
private static Long[] testValues = new Long[] { private static Long[] testValues = new Long[]{7L, 4L, 5L, 0L, 3L, 4L, 8L, 6L, 1L, 9L, 2L};
7L, 4L, 5L, 0L, 3L, 4L, 8L, 6L, 1L, 9L, 2L
};


private static long[] expectedValues = new long[] { private static long[] expectedValues = new long[]{0L, 1L, 2L, 3L, 4L, 4L, 5L, 6L, 7L, 8L, 9L};
0L, 1L, 2L, 3L, 4L, 4L, 5L, 6L, 7L, 8L, 9L
};


private static Comparator<Long> comparator = new Comparator<Long>() private static Comparator<Long> comparator = new Comparator<Long>()
{ {
Expand All @@ -53,9 +49,9 @@ public int compare( Long x, Long y )
public final ExpectedException exception = ExpectedException.none(); public final ExpectedException exception = ExpectedException.none();


@Test @Test
public void shouldHandleAddingMoreValuesThenCapacity() public void shouldHandleAddingMoreValuesThanCapacity()
{ {
DefaultComparatorTopTable table = new DefaultComparatorTopTable( comparator,7 ); DefaultComparatorTopTable table = new DefaultComparatorTopTable( comparator, 7 );
for ( Long i : testValues ) for ( Long i : testValues )
{ {
table.add( i ); table.add( i );
Expand All @@ -77,7 +73,7 @@ public void shouldHandleAddingMoreValuesThenCapacity()
@Test @Test
public void shouldHandleWhenNotCompletelyFilledToCapacity() public void shouldHandleWhenNotCompletelyFilledToCapacity()
{ {
DefaultComparatorTopTable table = new DefaultComparatorTopTable( comparator,20 ); DefaultComparatorTopTable table = new DefaultComparatorTopTable( comparator, 20 );
for ( Long i : testValues ) for ( Long i : testValues )
{ {
table.add( i ); table.add( i );
Expand Down Expand Up @@ -112,20 +108,20 @@ public void shouldHandleWhenEmpty()
public void shouldThrowOnInitializeToZeroCapacity() public void shouldThrowOnInitializeToZeroCapacity()
{ {
exception.expect( IllegalArgumentException.class ); exception.expect( IllegalArgumentException.class );
new DefaultComparatorTopTable( comparator,0 ); new DefaultComparatorTopTable( comparator, 0 );
} }


@Test @Test
public void shouldThrowOnInitializeToNegativeCapacity() public void shouldThrowOnInitializeToNegativeCapacity()
{ {
exception.expect( IllegalArgumentException.class ); exception.expect( IllegalArgumentException.class );
new DefaultComparatorTopTable( comparator,-1 ); new DefaultComparatorTopTable( comparator, -1 );
} }


@Test @Test
public void shouldThrowOnSortNotCalledBeforeIterator() public void shouldThrowOnSortNotCalledBeforeIterator()
{ {
DefaultComparatorTopTable table = new DefaultComparatorTopTable( comparator,5 ); DefaultComparatorTopTable table = new DefaultComparatorTopTable( comparator, 5 );
for ( Long i : testValues ) for ( Long i : testValues )
{ {
table.add( i ); table.add( i );
Expand Down
Expand Up @@ -43,7 +43,7 @@ class Top1WithTiesSlottedPipeTest extends CypherFunSuite {
sortPipe.createResults(QueryStateHelper.emptyWithValueSerialization).toList should beEquivalentTo(List(Map("x" -> "A"))) sortPipe.createResults(QueryStateHelper.emptyWithValueSerialization).toList should beEquivalentTo(List(Map("x" -> "A")))
} }


test("three ties for the first place are all returned") { test("two ties for the first place are all returned") {
val input = List( val input = List(
Map("x" -> 1, "y" -> 1), Map("x" -> 1, "y" -> 1),
Map("x" -> 1, "y" -> 2), Map("x" -> 1, "y" -> 2),
Expand Down
Expand Up @@ -32,158 +32,158 @@ import scala.util.Random


class TopSlottedPipeTest extends CypherFunSuite { class TopSlottedPipeTest extends CypherFunSuite {


private sealed trait _ColumnOrder private sealed trait TestColumnOrder
private case object _Ascending extends _ColumnOrder private case object AscendingOrder extends TestColumnOrder
private case object _Descending extends _ColumnOrder private case object DescendingOrder extends TestColumnOrder


test("returning top 10 from 5 possible should return all") { test("returning top 10 from 5 possible should return all") {
val input = randomlyShuffledIntDataFromZeroUntil(5) val input = randomlyShuffledIntDataFromZeroUntil(5)
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Ascending, limit = 10 input, orderBy = AscendingOrder, limit = 10
) )
result should equal(list(0, 1, 2, 3, 4)) result should equal(list(0, 1, 2, 3, 4))
} }


test("returning top 10 descending from 3 possible should return all") { test("returning top 10 descending from 3 possible should return all") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
randomlyShuffledIntDataFromZeroUntil(3), orderBy = _Descending, limit = 10 randomlyShuffledIntDataFromZeroUntil(3), orderBy = DescendingOrder, limit = 10
) )
result should equal(list(2, 1, 0)) result should equal(list(2, 1, 0))
} }


test("returning top 5 from 20 possible should return 5 with lowest value") { test("returning top 5 from 20 possible should return 5 with lowest value") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
randomlyShuffledIntDataFromZeroUntil(20), orderBy = _Ascending, limit = 5 randomlyShuffledIntDataFromZeroUntil(20), orderBy = AscendingOrder, limit = 5
) )
result should equal(list(0, 1, 2, 3, 4)) result should equal(list(0, 1, 2, 3, 4))
} }


test("returning top 3 descending from 10 possible values should return three highest values") { test("returning top 3 descending from 10 possible values should return three highest values") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
randomlyShuffledIntDataFromZeroUntil(10), orderBy = _Descending, limit = 3 randomlyShuffledIntDataFromZeroUntil(10), orderBy = DescendingOrder, limit = 3
) )
result should equal(list(9, 8, 7)) result should equal(list(9, 8, 7))
} }


test("returning top 5 from a reversed pipe should work correctly") { test("returning top 5 from a reversed pipe should work correctly") {
val input = (0 until 100).reverse val input = (0 until 100).reverse
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Ascending, limit = 5 input, orderBy = AscendingOrder, limit = 5
) )
result should equal(list(0, 1, 2, 3, 4)) result should equal(list(0, 1, 2, 3, 4))
} }


test("duplicates should be sorted correctly") { test("duplicates should be sorted correctly") {
val input = ((0 until 5) ++ (0 until 5)).reverse val input = ((0 until 5) ++ (0 until 5)).reverse
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Descending, limit = 5 input, orderBy = DescendingOrder, limit = 5
) )
result should equal(list(4, 4, 3, 3, 2)) result should equal(list(4, 4, 3, 3, 2))
} }


test("duplicates should be sorted correctly for small lists") { test("duplicates should be sorted correctly for small lists") {
val input = List(0, 1, 1) val input = List(0, 1, 1)
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Descending, limit = 2 input, orderBy = DescendingOrder, limit = 2
) )
result should equal(list(1,1)) result should equal(list(1,1))
} }


test("should handle empty input") { test("should handle empty input") {
val input = Seq.empty val input = Seq.empty
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Ascending, limit = 5 input, orderBy = AscendingOrder, limit = 5
) )
result should equal(List.empty) result should equal(List.empty)
} }


test("should handle null input") { test("should handle null input") {
val input = Seq(10, null) val input = Seq(10, null)
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Ascending, limit = 5 input, orderBy = AscendingOrder, limit = 5
) )
result should equal(list(10, null)) result should equal(list(10, null))
} }


test("returning top 1 from 5 possible should return lowest") { test("returning top 1 from 5 possible should return lowest") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
randomlyShuffledIntDataFromZeroUntil(5), orderBy = _Ascending, limit = 1 randomlyShuffledIntDataFromZeroUntil(5), orderBy = AscendingOrder, limit = 1
) )
result should equal(list(0)) result should equal(list(0))
} }


test("returning top 1 descending from 3 possible should return all") { test("returning top 1 descending from 3 possible should return all") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
randomlyShuffledIntDataFromZeroUntil(3), orderBy = _Descending, limit = 1 randomlyShuffledIntDataFromZeroUntil(3), orderBy = DescendingOrder, limit = 1
) )
result should equal(list(2)) result should equal(list(2))
} }


test("returning top 1 from 20 possible should return 5 with lowest value") { test("returning top 1 from 20 possible should return 5 with lowest value") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
randomlyShuffledIntDataFromZeroUntil(20), orderBy = _Ascending, limit = 1 randomlyShuffledIntDataFromZeroUntil(20), orderBy = AscendingOrder, limit = 1
) )
result should equal(list(0)) result should equal(list(0))
} }


test("returning top 1 descending from 10 possible values should return three highest values") { test("returning top 1 descending from 10 possible values should return three highest values") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
randomlyShuffledIntDataFromZeroUntil(10), orderBy = _Descending, limit = 1 randomlyShuffledIntDataFromZeroUntil(10), orderBy = DescendingOrder, limit = 1
) )
result should equal(list(9)) result should equal(list(9))
} }


test("returning top 1 from a reversed pipe should work correctly") { test("returning top 1 from a reversed pipe should work correctly") {
val input = (0 until 100).reverse val input = (0 until 100).reverse
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Ascending, limit = 1 input, orderBy = AscendingOrder, limit = 1
) )
result should equal(list(0)) result should equal(list(0))
} }


test("duplicates should be sorted correctly with top 1") { test("duplicates should be sorted correctly with top 1") {
val input = ((0 until 5) ++ (0 until 5)).reverse val input = ((0 until 5) ++ (0 until 5)).reverse
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Descending, limit = 1 input, orderBy = DescendingOrder, limit = 1
) )
result should equal(list(4)) result should equal(list(4))
} }


test("duplicates should be sorted correctly for small lists with top 1") { test("duplicates should be sorted correctly for small lists with top 1") {
val input = Seq(0, 1, 1) val input = Seq(0, 1, 1)
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Descending, limit = 1 input, orderBy = DescendingOrder, limit = 1
) )
result should equal(list(1)) result should equal(list(1))
} }


test("top 1 should handle empty input with") { test("top 1 should handle empty input with") {
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
Seq.empty, orderBy = _Descending, limit = 1 Seq.empty, orderBy = DescendingOrder, limit = 1
) )
result should equal(List.empty) result should equal(List.empty)
} }


test("top 1 should handle null input") { test("top 1 should handle null input") {
val input = Seq(10, null) val input = Seq(10, null)
val result = singleColumnTopWithInput( val result = singleColumnTopWithInput(
input, orderBy = _Ascending, limit = 1 input, orderBy = AscendingOrder, limit = 1
) )
result should equal(list(10)) result should equal(list(10))
} }


test("top 5 from multi column values") { test("top 5 from multi column values") {
val input = List((2, 0), (1, 2), (0, 4), (1, 1), (0, 2), (1, 2), (0, 5), (2, 1)) val input = List((2, 0), (1, 2), (0, 4), (1, 1), (0, 2), (1, 2), (0, 5), (2, 1))
val result = twoColumnTopWithInput( val result = twoColumnTopWithInput(
input, orderBy = (_Ascending, _Descending), limit = 5 input, orderBy = (AscendingOrder, DescendingOrder), limit = 5
) )
result should equal(list((0, 5), (0, 4), (0, 2), (1, 2), (1, 2))) result should equal(list((0, 5), (0, 4), (0, 2), (1, 2), (1, 2)))
} }


test("top 1 from multi column values") { test("top 1 from multi column values") {
val input = List((2, 0), (1, 2), (0, 4), (1, 1), (0, 2), (1, 2), (0, 5), (2, 1)) val input = List((2, 0), (1, 2), (0, 4), (1, 1), (0, 2), (1, 2), (0, 5), (2, 1))
val result = twoColumnTopWithInput( val result = twoColumnTopWithInput(
input, orderBy = (_Ascending, _Descending), limit = 1 input, orderBy = (AscendingOrder, DescendingOrder), limit = 1
) )
result should equal(list((0, 5))) result should equal(list((0, 5)))
} }
Expand All @@ -199,7 +199,7 @@ class TopSlottedPipeTest extends CypherFunSuite {
data data
} }


private def singleColumnTopWithInput(data: Traversable[Any], orderBy: _ColumnOrder, limit: Int) = { private def singleColumnTopWithInput(data: Traversable[Any], orderBy: TestColumnOrder, limit: Int) = {
val pipeline = PipelineInformation.empty val pipeline = PipelineInformation.empty
.newReference("a", nullable = true, CTAny) .newReference("a", nullable = true, CTAny)


Expand All @@ -208,8 +208,8 @@ class TopSlottedPipeTest extends CypherFunSuite {
val source = FakeSlottedPipe(data.map(v => Map("a" -> v)).toIterator, pipeline) val source = FakeSlottedPipe(data.map(v => Map("a" -> v)).toIterator, pipeline)


val topOrderBy = orderBy match { val topOrderBy = orderBy match {
case `_Ascending` => List(Ascending(slot)) case AscendingOrder => List(Ascending(slot))
case `_Descending` => List(Descending(slot)) case DescendingOrder => List(Descending(slot))
} }


val topPipe = val topPipe =
Expand All @@ -230,7 +230,7 @@ class TopSlottedPipeTest extends CypherFunSuite {
}.toList }.toList
} }


private def twoColumnTopWithInput(data: Traversable[(Any, Any)], orderBy: (_ColumnOrder, _ColumnOrder), limit: Int) = { private def twoColumnTopWithInput(data: Traversable[(Any, Any)], orderBy: (TestColumnOrder, TestColumnOrder), limit: Int) = {
val pipeline = PipelineInformation.empty val pipeline = PipelineInformation.empty
.newReference("a", nullable = true, CTAny) .newReference("a", nullable = true, CTAny)
.newReference("b", nullable = true, CTAny) .newReference("b", nullable = true, CTAny)
Expand All @@ -241,8 +241,8 @@ class TopSlottedPipeTest extends CypherFunSuite {
val source = FakeSlottedPipe(data.map { case (v1, v2) => Map("a" -> v1, "b" -> v2) }.toIterator, pipeline) val source = FakeSlottedPipe(data.map { case (v1, v2) => Map("a" -> v1, "b" -> v2) }.toIterator, pipeline)


val topOrderBy = List((orderBy._1, slot1), (orderBy._2, slot2)).map { val topOrderBy = List((orderBy._1, slot1), (orderBy._2, slot2)).map {
case (`_Ascending`, slot) => Ascending(slot) case (AscendingOrder, slot) => Ascending(slot)
case (`_Descending`, slot) => Descending(slot) case (DescendingOrder, slot) => Descending(slot)
} }


val topPipe = val topPipe =
Expand Down

0 comments on commit 712ab7c

Please sign in to comment.