From 325c61f72cdbf682abc5f42f6df415ecdc1e6f7e Mon Sep 17 00:00:00 2001 From: vlakoff Date: Sun, 30 Oct 2016 08:53:35 +0100 Subject: [PATCH 1/5] Avoid extraneous database query when last chunk is partial --- src/Illuminate/Database/Eloquent/Builder.php | 8 +++++++- src/Illuminate/Database/Query/Builder.php | 8 +++++++- tests/Database/DatabaseEloquentBuilderTest.php | 3 +-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Builder.php b/src/Illuminate/Database/Eloquent/Builder.php index cf269b446d5d..0bb2314c21ed 100755 --- a/src/Illuminate/Database/Eloquent/Builder.php +++ b/src/Illuminate/Database/Eloquent/Builder.php @@ -372,8 +372,9 @@ public function cursor() public function chunk($count, callable $callback) { $results = $this->forPage($page = 1, $count)->get(); + $countResults = $results->count(); - while (! $results->isEmpty()) { + while ($countResults > 0) { // On each chunk result set, we will pass them to the callback and then let the // developer take care of everything within the callback, which allows us to // keep the memory low for spinning through large result sets for working. @@ -381,9 +382,14 @@ public function chunk($count, callable $callback) return false; } + if ($countResults < $count) { + break; + } + $page++; $results = $this->forPage($page, $count)->get(); + $countResults = count($results); } return true; diff --git a/src/Illuminate/Database/Query/Builder.php b/src/Illuminate/Database/Query/Builder.php index ba20e42c68b8..992a7031a2c9 100755 --- a/src/Illuminate/Database/Query/Builder.php +++ b/src/Illuminate/Database/Query/Builder.php @@ -1808,8 +1808,9 @@ public function cursor() public function chunk($count, callable $callback) { $results = $this->forPage($page = 1, $count)->get(); + $countResults = $results->count(); - while (! $results->isEmpty()) { + while ($countResults > 0) { // On each chunk result set, we will pass them to the callback and then let the // developer take care of everything within the callback, which allows us to // keep the memory low for spinning through large result sets for working. @@ -1817,9 +1818,14 @@ public function chunk($count, callable $callback) return false; } + if ($countResults < $count) { + break; + } + $page++; $results = $this->forPage($page, $count)->get(); + $countResults = count($results); } return true; diff --git a/tests/Database/DatabaseEloquentBuilderTest.php b/tests/Database/DatabaseEloquentBuilderTest.php index 0bded7cd104c..d2f807ce638c 100755 --- a/tests/Database/DatabaseEloquentBuilderTest.php +++ b/tests/Database/DatabaseEloquentBuilderTest.php @@ -159,8 +159,7 @@ public function testChunkExecuteCallbackOverPaginatedRequest() $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,get]', [$this->getMockQueryBuilder()]); $builder->shouldReceive('forPage')->once()->with(1, 2)->andReturn($builder); $builder->shouldReceive('forPage')->once()->with(2, 2)->andReturn($builder); - $builder->shouldReceive('forPage')->once()->with(3, 2)->andReturn($builder); - $builder->shouldReceive('get')->times(3)->andReturn(new Collection(['foo1', 'foo2']), new Collection(['foo3']), new Collection([])); + $builder->shouldReceive('get')->times(2)->andReturn(new Collection(['foo1', 'foo2']), new Collection(['foo3'])); $callbackExecutionAssertor = m::mock('StdClass'); $callbackExecutionAssertor->shouldReceive('doSomething')->with('foo1')->once(); From 02f2dcbe28b0c89ab4e11bda98958c986d8a713b Mon Sep 17 00:00:00 2001 From: vlakoff Date: Sun, 30 Oct 2016 09:01:30 +0100 Subject: [PATCH 2/5] Alternative code for chunk() methods This way the get() part isn't written twice. --- src/Illuminate/Database/Eloquent/Builder.php | 21 ++++++++++---------- src/Illuminate/Database/Query/Builder.php | 21 ++++++++++---------- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Builder.php b/src/Illuminate/Database/Eloquent/Builder.php index 0bb2314c21ed..3dff643cb20d 100755 --- a/src/Illuminate/Database/Eloquent/Builder.php +++ b/src/Illuminate/Database/Eloquent/Builder.php @@ -371,10 +371,16 @@ public function cursor() */ public function chunk($count, callable $callback) { - $results = $this->forPage($page = 1, $count)->get(); - $countResults = $results->count(); + $page = 1; + + do { + $results = $this->forPage($page, $count)->get(); + $countResults = $results->count(); + + if ($countResults == 0) { + break; + } - while ($countResults > 0) { // On each chunk result set, we will pass them to the callback and then let the // developer take care of everything within the callback, which allows us to // keep the memory low for spinning through large result sets for working. @@ -382,15 +388,8 @@ public function chunk($count, callable $callback) return false; } - if ($countResults < $count) { - break; - } - $page++; - - $results = $this->forPage($page, $count)->get(); - $countResults = count($results); - } + } while ($countResults == $count); return true; } diff --git a/src/Illuminate/Database/Query/Builder.php b/src/Illuminate/Database/Query/Builder.php index 992a7031a2c9..a8b04928c80b 100755 --- a/src/Illuminate/Database/Query/Builder.php +++ b/src/Illuminate/Database/Query/Builder.php @@ -1807,10 +1807,16 @@ public function cursor() */ public function chunk($count, callable $callback) { - $results = $this->forPage($page = 1, $count)->get(); - $countResults = $results->count(); + $page = 1; + + do { + $results = $this->forPage($page, $count)->get(); + $countResults = $results->count(); + + if ($countResults == 0) { + break; + } - while ($countResults > 0) { // On each chunk result set, we will pass them to the callback and then let the // developer take care of everything within the callback, which allows us to // keep the memory low for spinning through large result sets for working. @@ -1818,15 +1824,8 @@ public function chunk($count, callable $callback) return false; } - if ($countResults < $count) { - break; - } - $page++; - - $results = $this->forPage($page, $count)->get(); - $countResults = count($results); - } + } while ($countResults == $count); return true; } From 4d77f80c30aa7a06e0b104c872822564971d87a8 Mon Sep 17 00:00:00 2001 From: vlakoff Date: Sun, 30 Oct 2016 09:08:34 +0100 Subject: [PATCH 3/5] Compare directly against the Collections in chunk() tests --- .../Database/DatabaseEloquentBuilderTest.php | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/Database/DatabaseEloquentBuilderTest.php b/tests/Database/DatabaseEloquentBuilderTest.php index d2f807ce638c..25e9fb8e30a0 100755 --- a/tests/Database/DatabaseEloquentBuilderTest.php +++ b/tests/Database/DatabaseEloquentBuilderTest.php @@ -157,38 +157,36 @@ public function testValueMethodWithModelNotFound() public function testChunkExecuteCallbackOverPaginatedRequest() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,get]', [$this->getMockQueryBuilder()]); + $chunk1 = new Collection(['foo1', 'foo2']); + $chunk2 = new Collection(['foo3']); $builder->shouldReceive('forPage')->once()->with(1, 2)->andReturn($builder); $builder->shouldReceive('forPage')->once()->with(2, 2)->andReturn($builder); - $builder->shouldReceive('get')->times(2)->andReturn(new Collection(['foo1', 'foo2']), new Collection(['foo3'])); + $builder->shouldReceive('get')->times(2)->andReturn($chunk1, $chunk2); $callbackExecutionAssertor = m::mock('StdClass'); - $callbackExecutionAssertor->shouldReceive('doSomething')->with('foo1')->once(); - $callbackExecutionAssertor->shouldReceive('doSomething')->with('foo2')->once(); - $callbackExecutionAssertor->shouldReceive('doSomething')->with('foo3')->once(); + $callbackExecutionAssertor->shouldReceive('doSomething')->with($chunk1)->once(); + $callbackExecutionAssertor->shouldReceive('doSomething')->with($chunk2)->once(); $builder->chunk(2, function ($results) use ($callbackExecutionAssertor) { - foreach ($results as $result) { - $callbackExecutionAssertor->doSomething($result); - } + $callbackExecutionAssertor->doSomething($results); }); } public function testChunkCanBeStoppedByReturningFalse() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,get]', [$this->getMockQueryBuilder()]); + $chunk1 = new Collection(['foo1', 'foo2']); + $chunk2 = new Collection(['foo3']); $builder->shouldReceive('forPage')->once()->with(1, 2)->andReturn($builder); $builder->shouldReceive('forPage')->never()->with(2, 2); - $builder->shouldReceive('get')->times(1)->andReturn(new Collection(['foo1', 'foo2'])); + $builder->shouldReceive('get')->times(1)->andReturn($chunk1); $callbackExecutionAssertor = m::mock('StdClass'); - $callbackExecutionAssertor->shouldReceive('doSomething')->with('foo1')->once(); - $callbackExecutionAssertor->shouldReceive('doSomething')->with('foo2')->once(); - $callbackExecutionAssertor->shouldReceive('doSomething')->with('foo3')->never(); + $callbackExecutionAssertor->shouldReceive('doSomething')->with($chunk1)->once(); + $callbackExecutionAssertor->shouldReceive('doSomething')->with($chunk2)->never(); $builder->chunk(2, function ($results) use ($callbackExecutionAssertor) { - foreach ($results as $result) { - $callbackExecutionAssertor->doSomething($result); - } + $callbackExecutionAssertor->doSomething($results); return false; }); From 148564c49fd55be19ffb00d6cc506a232bec413d Mon Sep 17 00:00:00 2001 From: vlakoff Date: Sun, 30 Oct 2016 09:58:55 +0100 Subject: [PATCH 4/5] Add test when last chunk is complete --- .../Database/DatabaseEloquentBuilderTest.php | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/Database/DatabaseEloquentBuilderTest.php b/tests/Database/DatabaseEloquentBuilderTest.php index 25e9fb8e30a0..f92c3a31fef7 100755 --- a/tests/Database/DatabaseEloquentBuilderTest.php +++ b/tests/Database/DatabaseEloquentBuilderTest.php @@ -154,7 +154,28 @@ public function testValueMethodWithModelNotFound() $this->assertNull($builder->value('name')); } - public function testChunkExecuteCallbackOverPaginatedRequest() + public function testChunkWithLastChunkComplete() + { + $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,get]', [$this->getMockQueryBuilder()]); + $chunk1 = new Collection(['foo1', 'foo2']); + $chunk2 = new Collection(['foo3', 'foo4']); + $chunk3 = new Collection([]); + $builder->shouldReceive('forPage')->once()->with(1, 2)->andReturn($builder); + $builder->shouldReceive('forPage')->once()->with(2, 2)->andReturn($builder); + $builder->shouldReceive('forPage')->once()->with(3, 2)->andReturn($builder); + $builder->shouldReceive('get')->times(3)->andReturn($chunk1, $chunk2, $chunk3); + + $callbackExecutionAssertor = m::mock('StdClass'); + $callbackExecutionAssertor->shouldReceive('doSomething')->with($chunk1)->once(); + $callbackExecutionAssertor->shouldReceive('doSomething')->with($chunk2)->once(); + $callbackExecutionAssertor->shouldReceive('doSomething')->with($chunk3)->never(); + + $builder->chunk(2, function ($results) use ($callbackExecutionAssertor) { + $callbackExecutionAssertor->doSomething($results); + }); + } + + public function testChunkWithLastChunkPartial() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,get]', [$this->getMockQueryBuilder()]); $chunk1 = new Collection(['foo1', 'foo2']); From 08e514c9b116ee427d9320ce9d00443e674c474c Mon Sep 17 00:00:00 2001 From: vlakoff Date: Sun, 30 Oct 2016 11:00:32 +0100 Subject: [PATCH 5/5] Avoid extraneous query in chunkById() methods as well --- src/Illuminate/Database/Eloquent/Builder.php | 15 ++++++++----- src/Illuminate/Database/Query/Builder.php | 17 ++++++++------ .../Database/DatabaseEloquentBuilderTest.php | 21 +++++++++++++++--- tests/Database/DatabaseQueryBuilderTest.php | 22 ++++++++++++++++--- 4 files changed, 56 insertions(+), 19 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Builder.php b/src/Illuminate/Database/Eloquent/Builder.php index 3dff643cb20d..a14805656508 100755 --- a/src/Illuminate/Database/Eloquent/Builder.php +++ b/src/Illuminate/Database/Eloquent/Builder.php @@ -404,19 +404,22 @@ public function chunk($count, callable $callback) */ public function chunkById($count, callable $callback, $column = 'id') { - $lastId = null; + $lastId = 0; - $results = $this->forPageAfterId($count, 0, $column)->get(); + do { + $results = $this->forPageAfterId($count, $lastId, $column)->get(); + $countResults = $results->count(); + + if ($countResults == 0) { + break; + } - while (! $results->isEmpty()) { if (call_user_func($callback, $results) === false) { return false; } $lastId = $results->last()->{$column}; - - $results = $this->forPageAfterId($count, $lastId, $column)->get(); - } + } while ($countResults == $count); return true; } diff --git a/src/Illuminate/Database/Query/Builder.php b/src/Illuminate/Database/Query/Builder.php index a8b04928c80b..7e44ce9d1b11 100755 --- a/src/Illuminate/Database/Query/Builder.php +++ b/src/Illuminate/Database/Query/Builder.php @@ -1841,21 +1841,24 @@ public function chunk($count, callable $callback) */ public function chunkById($count, callable $callback, $column = 'id', $alias = null) { - $lastId = null; - $alias = $alias ?: $column; - $results = $this->forPageAfterId($count, 0, $column)->get(); + $lastId = 0; + + do { + $results = $this->forPageAfterId($count, $lastId, $column)->get(); + $countResults = $results->count(); + + if ($countResults == 0) { + break; + } - while (! $results->isEmpty()) { if (call_user_func($callback, $results) === false) { return false; } $lastId = $results->last()->{$alias}; - - $results = $this->forPageAfterId($count, $lastId, $column)->get(); - } + } while ($countResults == $count); return true; } diff --git a/tests/Database/DatabaseEloquentBuilderTest.php b/tests/Database/DatabaseEloquentBuilderTest.php index f92c3a31fef7..188c89d4fbab 100755 --- a/tests/Database/DatabaseEloquentBuilderTest.php +++ b/tests/Database/DatabaseEloquentBuilderTest.php @@ -213,16 +213,16 @@ public function testChunkCanBeStoppedByReturningFalse() }); } - public function testChunkPaginatesUsingId() + public function testChunkPaginatesUsingIdWithLastChunkComplete() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPageAfterId,get]', [$this->getMockQueryBuilder()]); $builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturn($builder); $builder->shouldReceive('forPageAfterId')->once()->with(2, 2, 'someIdField')->andReturn($builder); - $builder->shouldReceive('forPageAfterId')->once()->with(2, 10, 'someIdField')->andReturn($builder); + $builder->shouldReceive('forPageAfterId')->once()->with(2, 11, 'someIdField')->andReturn($builder); $builder->shouldReceive('get')->times(3)->andReturn( new Collection([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]), - new Collection([(object) ['someIdField' => 10]]), + new Collection([(object) ['someIdField' => 10], (object) ['someIdField' => 11]]), new Collection([]) ); @@ -230,6 +230,21 @@ public function testChunkPaginatesUsingId() }, 'someIdField'); } + public function testChunkPaginatesUsingIdWithLastChunkPartial() + { + $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPageAfterId,get]', [$this->getMockQueryBuilder()]); + $builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturn($builder); + $builder->shouldReceive('forPageAfterId')->once()->with(2, 2, 'someIdField')->andReturn($builder); + + $builder->shouldReceive('get')->times(2)->andReturn( + new Collection([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]), + new Collection([(object) ['someIdField' => 10]]) + ); + + $builder->chunkById(2, function ($results) { + }, 'someIdField'); + } + public function testPluckReturnsTheMutatedAttributesOfAModel() { $builder = $this->getBuilder(); diff --git a/tests/Database/DatabaseQueryBuilderTest.php b/tests/Database/DatabaseQueryBuilderTest.php index 82bb4aca20f9..11a3bac4da5d 100755 --- a/tests/Database/DatabaseQueryBuilderTest.php +++ b/tests/Database/DatabaseQueryBuilderTest.php @@ -1734,17 +1734,17 @@ public function testTableValuedFunctionAsTableInSqlServer() $this->assertEquals('select * from [users](1,2)', $builder->toSql()); } - public function testChunkPaginatesUsingId() + public function testChunkPaginatesUsingIdWithLastChunkComplete() { $builder = $this->getMockQueryBuilder(); $builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturn($builder); $builder->shouldReceive('forPageAfterId')->once()->with(2, 2, 'someIdField')->andReturn($builder); - $builder->shouldReceive('forPageAfterId')->once()->with(2, 10, 'someIdField')->andReturn($builder); + $builder->shouldReceive('forPageAfterId')->once()->with(2, 11, 'someIdField')->andReturn($builder); $builder->shouldReceive('get')->times(3)->andReturn( collect([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]), - collect([(object) ['someIdField' => 10]]), + collect([(object) ['someIdField' => 10], (object) ['someIdField' => 11]]), collect([]) ); @@ -1752,6 +1752,22 @@ public function testChunkPaginatesUsingId() }, 'someIdField'); } + public function testChunkPaginatesUsingIdWithLastChunkPartial() + { + $builder = $this->getMockQueryBuilder(); + + $builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturn($builder); + $builder->shouldReceive('forPageAfterId')->once()->with(2, 2, 'someIdField')->andReturn($builder); + + $builder->shouldReceive('get')->times(2)->andReturn( + collect([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]), + collect([(object) ['someIdField' => 10]]) + ); + + $builder->chunkById(2, function ($results) { + }, 'someIdField'); + } + public function testChunkPaginatesUsingIdWithAlias() { $builder = $this->getMockQueryBuilder();