Builder::chunk may consider previously set offset and limit

[vlakoff]

Currently, if one does this:

DB::table('records')->offset(1000)->limit(500)->chunk(50, $callback);

He might expect to retrieve only the records from 1000 to 1500, but that's not the case: the whole table is retrieved. This is particularly surprising because other clauses such as where would be taken into consideration.

I have come with a first draft, not yet tested, and it still feels clumsy…

public function chunk($count, callable $callback)
{
    $thisOffset = $this->getOffset(); // method to be implemented
    $thisLimit = $this->getLimit(); // same

    $page = 1;
    $rowsfetched = 0;

    do {
        $skip = $thisOffset + ($page - 1) * $count;

        if ($thisLimit) {
            $remaining = $thisLimit - $rowsfetched;
            if ($remaining == 0) {
                return true;
            }
            $take = min($count, $remaining);
        } else {
            $take = $count;
        }

        $results = $this->skip($skip)->take($take)->get();
        $countResults = $results->count();

        if ($countResults == 0) {
            break;
        }

        // 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.
        if (call_user_func($callback, $results) === false) {
            return false;
        }

        $page++;
        $rowsfetched += $countResults;
    }
    while ($countResults == $take);

    return true;
}

So,

• Do you agree on the idea?
• Suggestions for improving the code?

[JosephSilber]

This has recently been tried, but has been shot down by Taylor as expected behavior.

[vlakoff]

Yep, I've found it back: laravel/framework#9681. It's not that recent… Funny how my code came close to arrilot's one.

[arrilot]

@vlakoff there are not many other ways :) I (and probably many others) still disagree with @taylorotwell in this case but he is the boss in the end

[taylorotwell]

I will look into it again.

On May 29, 2016, at 7:23 AM, Nekrasov Ilya notifications@github.com wrote:

@vlakoff https://github.com/vlakoff there are not many other way :) I (and probably many others) still disagree with @taylorotwell https://github.com/taylorotwell in this case but he is the boss in the end

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #103 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAcRfrVy5XEzMq59Hm4fHif3br0S4ta6ks5qGYUugaJpZM4IpJnt.

[vlakoff]

Thanks :-)

Just a few reminders:

• Better check "until count($results) < $chunk/take", rather than "while count($result) > 0". This way, if the last chunk is partial, you know you have reached the end and don't do another query which returns no results. This could be applied to current code too.
• A case I had to keep in mind when working on the logic: if there is a $limit set and the last chunk is entire (not partial). Make sure you don't continue processing in this case.

[drmmr763]

Just want to offer a friendly +1 for changing this. I literally just wrote the same lines expecting the chunk method to iterate over my given limit and offset values. It works perfectly with "whereBetween".

thanks

[vlakoff]

I've worked on a revised version which seems to be quite good. Already posting it here, if you want to have a glance at it:

public function chunk($count, callable $callback)
{
    $offset = $this->getOffset() ?: 0; // method to be implemented
    $remaining = $this->getLimit(); // same

    do {
        if (! is_null($remaining)) {
            $limit = min($count, $remaining);
        } else {
            $limit = $count;
        }

        $results = $this->skip($offset)->take($limit)->get();

        $countResults = $results->count();

        if ($countResults == 0) {
            break;
        }

        // 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.
        if (call_user_func($callback, $results) === false) {
            return false;
        }

        $offset += $countResults;

        if (! is_null($remaining)) {
            $remaining -= $countResults;
            if ($remaining == 0) {
                break;
            }
        }
    } while ($countResults == $limit);

    return true;
}

[vlakoff]

Calling chunk() "pollutes" the offset and limit properties of the query builder instance. This may lead to side-effects if reusing the instance.

Would it be worth it to backup/restore these properties, or is it acceptable to consider the instance is "dirty" once the queries have been run?

edit: just submitted laravel/framework#16479 about this.

[ISaidHey]

Just chiming in with the need for this. Found this thread after writing limit()->chunk() logic and finding it not working.

[Kyle-J]

+1

Same issue here. Just came across a scenario expecting limit to work and am now trying to implement my own tick limit. It's isn't easy to break out because it's a closure passed to an internal loop, so break an continue don't work. If I use exit I can't output anything later so the only option is return and let it run it's course.

Anyone got a better way to break out of it?

if($tick_count >= $tick_limit) return;