Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.2] Add relationship subquery count #13414

Merged
merged 4 commits into from
May 11, 2016
Merged

[5.2] Add relationship subquery count #13414

merged 4 commits into from
May 11, 2016

Conversation

barryvdh
Copy link
Contributor

@barryvdh barryvdh commented May 3, 2016

This uses the same subquery as the has() clause, but adds it to the select. This way the count for a relationship can be loaded in 1 query, without loading the relationship itself.

Example:

// Model with relationship
class Client extends Model {
    public function projects() {
        return $this->hasMany(Project::class);
    }
}
// Query to get the clients
$clients = Client::select('*')->selectCount('projects', 'projectCount')->get()

Resulting query

select *, (select count(*) from `projects` where `projects`.`client_id` = `clients`.`id`) as `projectCount` from `clients`

This will add an attributeprojectCount to each Client object. This can also be used with ->having('projectCount', '>=', 1) to avoid using an extra has() subquery. And also easy ordering in the query: ->orderBy('projectCount', 'desc')

I know this has been rejected before (#2813), but it seems that most of the 'complicated' logic has been implemented already (subquery selects and relationCountQuery).

The alternatives are:

  • $client->projects->count() -> Needs to load all relations, even when you don't need them.
  • $client->projects()->count() -> Needs to run 1 query per client, so not good for many rows.

Potential problem: Adding a select will stop the default select columns from being added, so an explicit select is needed, before doing the count select. Not sure if this needs working around.

@barryvdh barryvdh changed the title [WIP] Add relationship subquery count Add relationship subquery count May 3, 2016
@barryvdh barryvdh changed the title Add relationship subquery count [L5.2] Add relationship subquery count May 3, 2016
@barryvdh
Copy link
Contributor Author

barryvdh commented May 3, 2016

CS fixed and tests added :)

@acasar
Copy link
Contributor

acasar commented May 3, 2016

@barryvdh 👍

Would it be possible to have a syntax similar to with? Maybe something like that:

Article::withCount('comments', 'tags', 'categories')->get()

And with additional constraints:

Article::withCount(['comments' => function($query) {
    $query->where('published_at', '>=', Carbon::now());
}])->get()

@barryvdh
Copy link
Contributor Author

barryvdh commented May 3, 2016

Not easily, because you need both the relation name + the attribute name, in addition to an optional closure. Or you need to set a 'default' attribute name, but that might conflict with existing columns.

@GrahamCampbell GrahamCampbell changed the title [L5.2] Add relationship subquery count [5.2] Add relationship subquery count May 5, 2016
@taylorotwell
Copy link
Member

If I run this query (I tweaked your code slightly):

$users = User::withCount('posts')->get();

I only get the posts_count and not the rest of the columns like I would expected. I found this pretty counterintuitive. I'm guessing it's becuase ->get() is seeing there is already a "column" selected and is not adding * to the query.

@barryvdh
Copy link
Contributor Author

barryvdh commented May 9, 2016

Yes that's what I said in the OP:

Potential problem: Adding a select will stop the default select columns from being added, so an explicit select is needed, before doing the count select. Not sure if this needs working around.

If at least one column is selected, it ignores the get() param. So not sure what the best way to handle that, perhaps check what kind of columns are added already, and skip for some cases? Or add select('*') by default (but that would have unexpected side effects)

@acasar
Copy link
Contributor

acasar commented May 9, 2016

I think this should be addressed in 5.3. If I call addSelect on a query I wouldn't expect it to override default select. We already have select for that. That change would also solve the issue here.

A workaround may be to explicitly call select('*') if no other selects have been defined yet.

@barryvdh
Copy link
Contributor Author

barryvdh commented May 9, 2016

That was already submitted as PR, but Taylor said the current behaviour of addSelect was correct: #8048
Similar to that PR, it could also be fixed by using a different property to keep track of the added columns, and prepend that after the default columns. But indeed, changing the addSelect behaviour could be a BC break.

But if we decide to change addSelect in 5.3, we can still pull this in 5.2, and it will be improved in 5.3.
Or I can work around this if @taylorotwell wants that.

@barryvdh
Copy link
Contributor Author

barryvdh commented May 10, 2016

Updated the code to the withCount() syntax:

Article::withCount(['comments' => function($query) {
    $query->where('published_at', '>=', Carbon::now());
}])->get()

Also checked if columns were set, otherwise add ['*'] by default. The column name is snaked cased relation + '_count'.

@taylorotwell taylorotwell merged commit 67b821d into laravel:5.2 May 11, 2016
@taylorotwell
Copy link
Member

Thanks

@barryvdh barryvdh deleted the patch-10 branch May 11, 2016 14:56
@JayBizzle
Copy link
Contributor

JayBizzle commented May 18, 2016

@barryvdh How does this work with this kind of setup...

// User.php
class User extends Model
{

    // skipped usual model stuff for brevity

    public function comments()
    {
        return $this->hasMany('Comment')->where('status', 1);
    }
}
// Controller.php
class UserController extends Controller
{
    public function index($id)
    {
        dd(User::withCount('comments')->find($id));
    }
}

This seems to count ALL comments ignoring the status value which was defined in the where clause on the relationship...or am i missing something?

@taylorotwell
Copy link
Member

If that's true that would indeed be a large problem :)

@peterpan666
Copy link
Contributor

peterpan666 commented May 18, 2016

Nice work! Is there a way to get it auto eager loaded? Like with $with inside the Model

protected $with = ['projectsCount'];

Is this working?

EDIT

To answer my own question, there is a protected $withCount = ['projects']; attribute 😊

@barryvdh
Copy link
Contributor Author

@JayBizzle is that just with the count or also with the 'has' queries?

@barryvdh
Copy link
Contributor Author

Submitted a PR to merge the wheres: #13612

@JayBizzle
Copy link
Contributor

@barryvdh has works as expected

@barryvdh
Copy link
Contributor Author

barryvdh commented May 18, 2016

Yeah I noticed, PR is already submitted with a fix :) (see above)

@JayBizzle
Copy link
Contributor

Yeah, seen that, just testing it locally 👍

symfony-splitter pushed a commit to illuminate/database that referenced this pull request May 18, 2016
Fixes missing wheres defined on the relation, see laravel/framework#13414 (comment)
@chartspro
Copy link

Great job. Any updates related to $withCount model property (like @peterpan666 mentioned above)?

@chartspro
Copy link

Relations with not null deleted_at attribute should also be excluded from count.

@barryvdh
Copy link
Contributor Author

Can you submit a PR with tests + fix for that, if you run into it?

@acasar
Copy link
Contributor

acasar commented May 23, 2016

@chartspro @barryvdh that works with the latest fix, just hasn't been tagged yet.

@barryvdh
Copy link
Contributor Author

Okay good, thought I was still missing something. For softDeletes, these tests should cover it already: 4abb185

@chyupa
Copy link

chyupa commented Jun 23, 2016

Does this work with nested eager loading?
For ex I wish to display my store with all brands with all products and with product comment count.
Something like this:
Store::with('brands.products')->withCount('brands.products.comments')->find(1)

@barryvdh
Copy link
Contributor Author

No, but if you can figure out how to do it nicely, good chance it will get merged ;)

@terion-name
Copy link

terion-name commented Jul 8, 2016

@barryvdh awesome and so needed feature! how about other aggregates, such as withSum, withAvg, withMax, etc?

@tainmar
Copy link

tainmar commented Jul 12, 2016

Any news on @chyupa request ?

@jsphpl
Copy link

jsphpl commented Sep 7, 2016

Is there a way to 'lazy-eager-load' the counts with a $post->loadCount('comments') method on the model similar to $post->load('comments')?

It would be useful when doing a $post->fresh($with) to also have the counts calculated. Is there another way to do this, except for defining an accessor?

@decadence
Copy link
Contributor

Bump to @terion-name question.

@makuser
Copy link

makuser commented Dec 14, 2016

I really like withCount() and it would be awesome to have something like withSum() as well. For example to get a ranking of all transactions I am currently using this:

$users = $request->user()
    ->association
    ->users()
    ->withCount(['transactions' => function ($query) {
        $query->whereDate('created_at', '>', Carbon::today()->subDays(21));
        $query->whereIn('item_id', [100, 102, 103, 109, 111]);
    }])
    ->orderBy('transactions_count', 'desc')
    ->get();

But since a transaction actually has a column called 'amount', it would actually make more sense to weight each transaction individually. Therefore I don't need to count all transactions, but to call the sum(amount) function.
Usage could be like this:

$users = $request->user()
    ->association
    ->users()
    ->withSum('transactions', 'amount')
    ->orderBy('transactions_sum', 'desc')
    ->get();

This would sum the column 'amount' in all transactions.
Or again with additional constraints:

$users = $request->user()
    ->association
    ->users()
    ->withSum(['transactions' => function ($query) {
        $query->whereDate('created_at', '>', Carbon::today()->subDays(21));
        $query->whereIn('item_id', [100, 102, 103, 109, 111]);
    }], 'amount')
    ->orderBy('transactions_sum', 'desc')
    ->get();

What do you think @terion-name @decadence @barryvdh?

@sburgos
Copy link

sburgos commented Dec 14, 2016

But, is it not dangerous to make a subquery like this? What if you have 1 000 000 clients, this approach will end up doing 1 000 000 queries to projects table, I think it is a bad practice to make subqueries when you don't have control of the number of rows.

@makuser
Copy link

makuser commented Dec 15, 2016

But, is it not dangerous to make a subquery like this? What if you have 1 000 000 clients, this approach will end up doing 1 000 000 queries to projects table, I think it is a bad practice to make subqueries when you don't have control of the number of rows.

Yes, it might be. That's why it depends on the coder whether to use it in the project or not. In my case I do need it and I am very aware of the "risk" / slightly increased load.

I just implemented it and created a PR: #16815.

@GregorVoelkl
Copy link

In my experience using withCount('relation') is really slow if there is a huge amount of related models....

@chimit
Copy link
Contributor

chimit commented Dec 11, 2017

Hi, @barryvdh!
Could you implement a loadCount() method too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.