`chunkById`/`forPageAfterId` doesn't work with `orderby`

[LastDragon-ru]

• Laravel Version: 8.15.0
• PHP Version: 7.4.x
• Database Driver & Version: Mysql 8

Speps to reproduce:

Route::get('/test', function () {
    /** @var \DateTimeImmutable $date */
    $date = \Illuminate\Support\Facades\Date::now();

    \App\Models\User::factory()->create([
        'created_at' => $date,
    ]); // id = 1
    \App\Models\User::factory()->create([
        'created_at' => $date->subDays(1),
    ]); // id = 2
    \App\Models\User::factory()->create([
        'created_at' => $date->subDays(2),
    ]); // id = 3

    \App\Models\User::query()
        ->orderBy('created_at')
        ->chunkById(1, function (\Illuminate\Support\Collection $users) {
            var_dump($users->pluck('id')->all());
        });
});

Actual results:

array (size=1)
  0 => int 3

Expected Result

array (size=1)
  0 => int 1

array (size=1)
  0 => int 2

array (size=1)
  0 => int 3

Description

This happens because chunkById/forPageAfterId uses:

framework/src/Illuminate/Database/Query/Builder.php

Lines 2131 to 2141 in 8a3aaf4

	public function forPageAfterId($perPage = 15, $lastId = 0, $column = 'id')
	{
	$this->orders = $this->removeExistingOrdersFor($column);
	if (! is_null($lastId)) {
	$this->where($column, '>', $lastId);
	}
	return $this->orderBy($column, 'asc')
	->limit($perPage);
	}

probably it should remove all other "order by"s instead (= call reorder()).

[taylorotwell]

Why do you expect to receive 3 results in your chunk when you call chunkById with 1?

[rodrigopedra]

@taylorotwell first time I saw it I thought the same,

But then I tested the code without the ->orderBy('created_at') and realized it prints all the users within an array for each chunk.

The catch is that the code has a var_dump inside the callback, so it expects an array to be printed the results for each chunk.

I tried @LastDragon-ru hint on using reorder and was able to "fix" it by making this change on Illuminate\Database\Query\Builder@forPageAfterId

public function forPageAfterId($perPage = 15, $lastId = 0, $column = 'id')
{
    $orders = $this->removeExistingOrdersFor($column);

    if (! is_null($lastId)) {
        $this->where($column, '>', $lastId);
    }

    $this->reorder($column, 'asc');
    
    $this->orders = \array_merge($this->orders, $orders);

    return $this->limit($perPage);
}

That will reapply previous order by clauses unrelated to the column used as id.

Current implementation will generate this SQL:

select * from `users` order by `created_at` asc, `id` asc limit 1

As all users were created with the same created_at and created_at is sorted first, MySQL (where I tested) grabs the last created one (id = 3). The next chunk will use the previous chunk $lastId, then the next chunk will be empty, and only one chunk is var_dump'ed

After the modification above this SQL is generated:

select * from `users` order by `id` asc, `created_at` asc limit 1

Now the id is ordered first, and all subsequents chunks are retrieved.

I am working on a PR, but didn't added any tests yet. I had a very busy week and was planning on work on tests early next week.

But if you prefer I can send the PR right away.

[LastDragon-ru]

Why do you expect to receive 3 results in your chunk when you call chunkById with 1?
first time I saw it I thought the same,

@taylorotwell, @rodrigopedra, this is a copy-paste error 😅 "expected" should be (the first post also fixed):

array (size=1)
  0 => int 1

array (size=1)
  0 => int 2

array (size=1)
  0 => int 3

[taylorotwell]

Is there a reason you are using chunkById when you specifically want a custom sort that is NOT based on ID? Why not just use chunk?

[rodrigopedra]

I also think ->chunk(...) would be a better fit if the developer needs to rely on other columns for sorting.

But here is a more "likely to happen" example:

Route::get('/test', function () {
    /** @var \DateTimeImmutable $date */
    $date = \Illuminate\Support\Facades\Date::now();

    \App\Models\User::query()->delete();

    \App\Models\User::factory()->create([
        'email' => 'ccc@example.com',
    ]); // id = 1
    \App\Models\User::factory()->create([
        'email' => 'bbb@example.com',
    ]); // id = 2
    \App\Models\User::factory()->create([
        'email' => 'aaa@example.com',
    ]); // id = 3

    \App\Models\User::query()
        ->orderBy('email')
        ->chunkById(1, function (\Illuminate\Support\Collection $users) {
            var_dump($users->pluck('id')->all());
        });
});

Difference here is that on the first example all created_at were the same, it is unlikely to happen unless you are batching importing records.

In this example, when sorting by email before calling ->chunkById(...) the results are in a different order than ordering by ID, which is more likely to happen.

Currently output:

array(1) {
  [0]=>
  int(3)
}

Output after suggested "fix" (not a fix, a workaround):

array(1) {
  [0]=>
  int(1)
}
array(1) {
  [0]=>
  int(2)
}
array(1) {
  [0]=>
  int(3)
}

The advantage I see on using ->chunkById(...) over ->chunk(...) is mainly about performance as the database will be able to skip a lot of records due to executing the WHERE clause before ordering, instead of iterating over the whole recordset.

I didn't send a PR yet, as I still didn't add any tests, and also I am not sure the expected results, considering this case where the emails are in a different order, would be ordering first by ID.

One thing that crossed my mind to account for that case was first ordering in a sub-query and then chunking in the order query by ID, but that would be tons of time worse performing than using ->chunk(...).

Maybe a better solution would be adding a note into the docs saying the developer could have unexpected results if they have other order by clauses before using ->chunkById(...)

Also on both examples passing an additional parameter with the column that needed to be used as "id" would leads to the expected results, in the expected order

    \App\Models\User::query()
        ->chunkById(1, function (\Illuminate\Support\Collection $users) {
            var_dump($users->pluck('id')->all());
        }, 'email'); // use email as the "id" column

array(1) {
  [0]=>
  int(3)
}
array(1) {
  [0]=>
  int(2)
}
array(1) {
  [0]=>
  int(1)
}

Maybe this parameter could be highlighted in the docs too.

Hope this helps.

[LastDragon-ru]

I have common getQuery() with the "order by"s (in the more general case it may be a query from package or query with scopes), and this query also used to update models. The updated model may be excluded from results so the chunk() is not safe, and according to the docs:

when updating records while chunking, it is always best to use the chunkById method instead.

The current behavior is not very intuitive...

In my case reorder() works fine (actually I use a bit different approach now), because I need only order by <key> and other columns are not matter. But I have no idea how it should work in the general case, maybe @rodrigopedra suggestion will be better.

[taylorotwell]

OK. If you want to change some behavior submit a PR so it can be discussed. chunkById adds an id order by that is why it is called chunkById.

[Jafo232]

One of the benefits of using chunkById() over chunk is that if you are updating records that may affect your where conditions, it won't affect the query:

Post::where('processed', 0)->chunk(100, function($posts) {
    foreach($posts as $post) {
       $post->processed = 1;
       $post->save();
    }
});

https://dev.to/hungthai1401/laravel-chunk-be-careful-40jo

The ability to be able to at least by able to chunkById(...,'desc') would be great!