Optimizing Collection::sort()

Using a heap was a terrible idea and also painfully slow. This implementation is now almost on part to sorting plain arrays
cakephp · Jan 6, 2015 · 6b9920d · 6b9920d
1 parent 8f9cb3d
commit 6b9920d
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 163 deletions.
diff --git a/src/Collection/CollectionTrait.php b/src/Collection/CollectionTrait.php
@@ -159,8 +159,7 @@ public function extract($matcher)
      */
     public function max($callback, $type = SORT_NUMERIC)
     {
-        $sorted = new SortIterator($this, $callback, SORT_DESC, $type);
-        return $sorted->top();
+        return (new SortIterator($this, $callback, SORT_DESC, $type))->first();
     }
 
     /**
@@ -169,8 +168,7 @@ public function max($callback, $type = SORT_NUMERIC)
      */
     public function min($callback, $type = SORT_NUMERIC)
     {
-        $sorted = new SortIterator($this, $callback, SORT_ASC, $type);
-        return $sorted->top();
+        return (new SortIterator($this, $callback, SORT_ASC, $type))->first();
     }
 
     /**
@@ -179,7 +177,7 @@ public function min($callback, $type = SORT_NUMERIC)
      */
     public function sortBy($callback, $dir = SORT_DESC, $type = SORT_NUMERIC)
     {
-        return new Collection(new SortIterator($this, $callback, $dir, $type));
+        return new SortIterator($this, $callback, $dir, $type);
     }
 
     /**

diff --git a/src/Collection/Iterator/SortIterator.php b/src/Collection/Iterator/SortIterator.php
@@ -14,8 +14,8 @@
  */
 namespace Cake\Collection\Iterator;
 
-use Cake\Collection\ExtractTrait;
-use SplHeap;
+use Cake\Collection\CollectionInterface;
+use Cake\Collection\Collection;
 
 /**
  * An iterator that will return the passed items in order. The order is given by
@@ -37,40 +37,9 @@
  *
  * This iterator does not preserve the keys passed in the original elements.
  */
-class SortIterator extends SplHeap
+class SortIterator extends Collection
 {
 
-    use ExtractTrait;
-
-    /**
-     * Original items passed to this iterator
-     *
-     * @var array|\Traversable
-     */
-    protected $_items;
-
-    /**
-     * The callback used to extract the column or property from the elements
-     *
-     * @var callable
-     */
-    protected $_callback;
-
-    /**
-     * The direction in which the elements should be sorted. The constants
-     * `SORT_ASC` and `SORT_DESC` are the accepted values
-     *
-     * @var string
-     */
-    protected $_dir;
-
-    /**
-     * The type of sort comparison to perform.
-     *
-     * @var string
-     */
-    protected $_type;
-
     /**
      * Wraps this iterator around the passed items so when iterated they are returned
      * in order.
@@ -90,70 +59,22 @@ class SortIterator extends SplHeap
      */
     public function __construct($items, $callback, $dir = SORT_DESC, $type = SORT_NUMERIC)
     {
-        $this->_items = $items;
-        $this->_dir = $dir;
-        $this->_type = $type;
-        $this->_callback = $this->_propertyExtractor($callback);
-    }
-
-    /**
-     * The comparison function used to sort the elements
-     *
-     * @param mixed $a an element in the list
-     * @param mixed $b an element in the list
-     * @return int
-     */
-    public function compare($a, $b)
-    {
-        if ($this->_dir === SORT_ASC) {
-            list($a, $b) = [$b, $a];
-        }
-
-        $callback = $this->_callback;
-        $a = $callback($a);
-        $b = $callback($b);
-
-        if ($this->_type === SORT_NUMERIC) {
-            return $a - $b;
+        if ($items instanceof CollectionInterface) {
+            $items = $items->toList();
         }
 
-        if ($this->_type === SORT_NATURAL) {
-            return strnatcmp($a, $b);
+        $callback = $this->_propertyExtractor($callback);
+        $results = [];
+        foreach ($items as $key => $value) {
+            $results[$key] = $callback($value);
         }
 
-        if ($this->_type === SORT_STRING) {
-            return strcmp($a, $b);
-        }
+        $dir === SORT_DESC ? arsort($results, $type) : asort($results, $type);
 
-        return strcoll($a, $b);
-    }
-
-    /**
-     * Returns the top of the heap. Rewinds the iterator if the heap is empty.
-     *
-     * @return mixed
-     */
-    public function top()
-    {
-        if ($this->isEmpty()) {
-            $this->rewind();
+        foreach (array_keys($results) as $key) {
+            $results[$key] = $items[$key];
         }
-        if ($this->isEmpty()) {
-            return null;
-        }
-        return parent::top();
+        parent::__construct($results);
     }
 
-    /**
-     * SplHeap removes elements upon iteration. Implementing rewind so that
-     * this iterator can be reused, at least at a cost.
-     *
-     * @return void
-     */
-    public function rewind()
-    {
-        foreach ($this->_items as $item) {
-            $this->insert($item);
-        }
-    }
 }
diff --git a/tests/TestCase/Collection/CollectionTest.php b/tests/TestCase/Collection/CollectionTest.php
@@ -306,11 +306,11 @@ public function testSortString()
         $map = $collection->sortBy('a.b.c');
         $this->assertInstanceOf('Cake\Collection\Collection', $map);
         $expected = [
-            2 => ['a' => ['b' => ['c' => 10]]],
-            1 => ['a' => ['b' => ['c' => 6]]],
-            0 => ['a' => ['b' => ['c' => 4]]],
+            ['a' => ['b' => ['c' => 10]]],
+            ['a' => ['b' => ['c' => 6]]],
+            ['a' => ['b' => ['c' => 4]]],
         ];
-        $this->assertEquals($expected, iterator_to_array($map));
+        $this->assertEquals($expected, $map->toList());
     }
 
     /**

diff --git a/tests/TestCase/Collection/Iterator/SortIteratorTest.php b/tests/TestCase/Collection/Iterator/SortIteratorTest.php
@@ -37,12 +37,12 @@ public function testSortNumbersIdentity()
             return $a;
         };
         $sorted = new SortIterator($items, $identity);
-        $expected = array_combine(range(4, 0), range(5, 1));
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $expected = range(5, 1);
+        $this->assertEquals($expected, $sorted->toList());
 
         $sorted = new SortIterator($items, $identity, SORT_ASC);
-        $expected = array_combine(range(4, 0), range(1, 5));
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $expected = range(1, 5);
+        $this->assertEquals($expected, $sorted->toList());
     }
 
     /**
@@ -57,12 +57,12 @@ public function testSortNumbersCustom()
             return $a * -1;
         };
         $sorted = new SortIterator($items, $callback);
-        $expected = array_combine(range(4, 0), [1, 2, 3, 4, 5]);
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $expected = range(1, 5);
+        $this->assertEquals($expected, $sorted->toList());
 
         $sorted = new SortIterator($items, $callback, SORT_ASC);
-        $expected = array_combine(range(4, 0), [5, 4, 3, 2, 1]);
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $expected = range(5, 1);
+        $this->assertEquals($expected, $sorted->toList());
     }
 
     /**
@@ -83,21 +83,21 @@ public function testSortComplexNumeric()
         };
         $sorted = new SortIterator($items, $callback, SORT_DESC, SORT_NUMERIC);
         $expected = [
-            3 => ['foo' => 13, 'bar' => 'a'],
-            2 => ['foo' => 10, 'bar' => 'a'],
-            1 => ['foo' => 2, 'bar' => 'a'],
-            0 => ['foo' => 1, 'bar' => 'a'],
+            ['foo' => 13, 'bar' => 'a'],
+            ['foo' => 10, 'bar' => 'a'],
+            ['foo' => 2, 'bar' => 'a'],
+            ['foo' => 1, 'bar' => 'a'],
         ];
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $this->assertEquals($expected, $sorted->toList());
 
         $sorted = new SortIterator($items, $callback, SORT_ASC, SORT_NUMERIC);
         $expected = [
-            3 => ['foo' => 1, 'bar' => 'a'],
-            2 => ['foo' => 2, 'bar' => 'a'],
-            1 => ['foo' => 10, 'bar' => 'a'],
-            0 => ['foo' => 13, 'bar' => 'a'],
+            ['foo' => 1, 'bar' => 'a'],
+            ['foo' => 2, 'bar' => 'a'],
+            ['foo' => 10, 'bar' => 'a'],
+            ['foo' => 13, 'bar' => 'a'],
         ];
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $this->assertEquals($expected, $sorted->toList());
     }
 
     /**
@@ -118,22 +118,22 @@ public function testSortComplexNatural()
         };
         $sorted = new SortIterator($items, $callback, SORT_DESC, SORT_NATURAL);
         $expected = [
-            3 => ['foo' => 'foo_13', 'bar' => 'a'],
-            2 => ['foo' => 'foo_10', 'bar' => 'a'],
-            1 => ['foo' => 'foo_2', 'bar' => 'a'],
-            0 => ['foo' => 'foo_1', 'bar' => 'a'],
+            ['foo' => 'foo_13', 'bar' => 'a'],
+            ['foo' => 'foo_10', 'bar' => 'a'],
+            ['foo' => 'foo_2', 'bar' => 'a'],
+            ['foo' => 'foo_1', 'bar' => 'a'],
         ];
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $this->assertEquals($expected, $sorted->toList());
 
         $sorted = new SortIterator($items, $callback, SORT_ASC, SORT_NATURAL);
         $expected = [
-            3 => ['foo' => 'foo_1', 'bar' => 'a'],
-            2 => ['foo' => 'foo_2', 'bar' => 'a'],
-            1 => ['foo' => 'foo_10', 'bar' => 'a'],
-            0 => ['foo' => 'foo_13', 'bar' => 'a'],
+            ['foo' => 'foo_1', 'bar' => 'a'],
+            ['foo' => 'foo_2', 'bar' => 'a'],
+            ['foo' => 'foo_10', 'bar' => 'a'],
+            ['foo' => 'foo_13', 'bar' => 'a'],
         ];
-        $this->assertEquals($expected, iterator_to_array($sorted));
-        $this->assertEquals($expected, iterator_to_array($sorted), 'Iterator should rewind');
+        $this->assertEquals($expected, $sorted->toList());
+        $this->assertEquals($expected, $sorted->toList(), 'Iterator should rewind');
     }
 
     /**
@@ -151,22 +151,22 @@ public function testSortComplexNaturalWithPath()
         ]);
         $sorted = new SortIterator($items, 'foo', SORT_DESC, SORT_NATURAL);
         $expected = [
-            3 => ['foo' => 'foo_13', 'bar' => 'a'],
-            2 => ['foo' => 'foo_10', 'bar' => 'a'],
-            1 => ['foo' => 'foo_2', 'bar' => 'a'],
-            0 => ['foo' => 'foo_1', 'bar' => 'a'],
+            ['foo' => 'foo_13', 'bar' => 'a'],
+            ['foo' => 'foo_10', 'bar' => 'a'],
+            ['foo' => 'foo_2', 'bar' => 'a'],
+            ['foo' => 'foo_1', 'bar' => 'a'],
         ];
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $this->assertEquals($expected, $sorted->toList());
 
         $sorted = new SortIterator($items, 'foo', SORT_ASC, SORT_NATURAL);
         $expected = [
-            3 => ['foo' => 'foo_1', 'bar' => 'a'],
-            2 => ['foo' => 'foo_2', 'bar' => 'a'],
-            1 => ['foo' => 'foo_10', 'bar' => 'a'],
-            0 => ['foo' => 'foo_13', 'bar' => 'a'],
+            ['foo' => 'foo_1', 'bar' => 'a'],
+            ['foo' => 'foo_2', 'bar' => 'a'],
+            ['foo' => 'foo_10', 'bar' => 'a'],
+            ['foo' => 'foo_13', 'bar' => 'a'],
         ];
-        $this->assertEquals($expected, iterator_to_array($sorted));
-        $this->assertEquals($expected, iterator_to_array($sorted), 'Iterator should rewind');
+        $this->assertEquals($expected, $sorted->toList());
+        $this->assertEquals($expected, $sorted->toList(), 'Iterator should rewind');
     }
 
     /**
@@ -184,29 +184,12 @@ public function testSortComplexDeepPath()
         ]);
         $sorted = new SortIterator($items, 'foo.bar', SORT_ASC, SORT_NUMERIC);
         $expected = [
-            3 => ['foo' => ['bar' => 1], 'bar' => 'a'],
-            2 => ['foo' => ['bar' => 2], 'bar' => 'a'],
-            1 => ['foo' => ['bar' => 10], 'bar' => 'a'],
-            0 => ['foo' => ['bar' => 12], 'bar' => 'a'],
+            ['foo' => ['bar' => 1], 'bar' => 'a'],
+            ['foo' => ['bar' => 2], 'bar' => 'a'],
+            ['foo' => ['bar' => 10], 'bar' => 'a'],
+            ['foo' => ['bar' => 12], 'bar' => 'a'],
         ];
-        $this->assertEquals($expected, iterator_to_array($sorted));
+        $this->assertEquals($expected, $sorted->toList());
     }
 
-    /**
-     * Tests top
-     *
-     * @return void
-     */
-    public function testTop()
-    {
-        $items = new ArrayObject([3, 5, 1, 2, 4]);
-        $identity = function ($a) {
-            return $a;
-        };
-        $sorted = new SortIterator($items, $identity);
-        $this->assertEquals(5, $sorted->top());
-
-        $sorted = new SortIterator([], $identity);
-        $this->assertNull($sorted->top());
-    }
 }