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

eloquent count() does not work well with groupBy() #22883

Closed
fico7489 opened this issue Jan 22, 2018 · 17 comments
Closed

eloquent count() does not work well with groupBy() #22883

fico7489 opened this issue Jan 22, 2018 · 17 comments

Comments

@fico7489
Copy link

Eloquent count() completely ignores what is in select and just adds count(*) as aggregate into select part. If get() method generates this query :

select sellers.* from sellers
        left join locations
        on locations.seller_id = sellers.id
        group by sellers.id

count() would generate this :

select count(*) as aggregate from sellers
        left join locations
        on locations.seller_id = sellers.id
        group by sellers.id

but what should really generate is this :

SELECT count(*) FROM (
    select sellers.* from sellers
        left join locations
        on locations.seller_id = sellers.id
        group by sellers.id) as aggregate ;

If we have 2 sellers the :
The first query generated from count() call returns result 1, but there are 2 sellers.
The second query returns 2 as it should.

@Dylan-DPC-zz
Copy link

I feel this is subjective. If I have to do a groupby-count in SQL i would write the same query that eloquent is generating.

@fico7489
Copy link
Author

fico7489 commented Jan 24, 2018

@Dylan-DPC I don't feel this is subjective, eloquent allows using groupBy() with count() but then result is not correct and this is the fact, no subjective....

@rs-sliske
Copy link

rs-sliske commented Feb 6, 2018

aggregate queries + group by return the aggregate for each group. eg how many locations the first seller has, then how many the second seller has etc etc

your example for what you think it "should" generate isnt even a valid query in default installs of more recent versions of mysql

what it looks like you are trying to do is get the number of sellers, which should be done as select count(distinct sellers.id) from sellers (or even just select count(*) from sellers as the id should be unique anyway). also as far as i know that left join in your query is useless, you arent using any of the fields from the joined table, and as it is left join its not going to effect the count

@fico7489
Copy link
Author

fico7489 commented Feb 6, 2018

@rs-sliske If you want to sort by related table attribute you must join related table (there is no other way). If the relation is hasOne you must use groupBy() because you don't want duplicated records accidentally, but in that case you can't use count() with groupBy().

I don't know how this looks like from your perspective, but for me this is a huge problem...

@rs-sliske
Copy link

from your comments so far, it sounds like distinct is what you need, not group by

aggregate functions (such as count, sum, min, max, etc) + group by return the value for each group

what you are trying to do is get the number of different "groups", which would be count(distinct field)

@fico7489
Copy link
Author

fico7489 commented Feb 9, 2018

No, no no. This question is not about finding appropriate query for that what I want. I want exact this query, and this query works perfectly for me.

select sellers.* from sellers
        left join locations
        on locations.seller_id = sellers.id
        group by sellers.id

Above query is easily generated by laravel eloquent functions and get() at the end

But running count() on the same builder gives me wrong result.

$builder->get()->count() //result here is 10
$builder->count() //result here is 1

How this can be normal ?

There are 2 solutions :

1.eloquent should forbid running count() when groupBy() is applied because the result is wrong
2.or eloquent can change count() function as I described in my first post.

//result here is 10
Seller::select('sellers.* ')
    ->join('locations', 'locations.seller_id', '=', 'sellers.id')
    ->groupBy('sellers.id')
    ->get()
    ->count();

//result here is 1
Seller::select('sellers.* ')
    ->join('locations', 'locations.seller_id', '=', 'sellers.id')
    ->groupBy('sellers.id')
    ->count()

Do you understand now what is the problem ?

PS. If you are confused, I am joining locations because I want sort by locations. address, but that is really not important here...

@Miguel-Serejo
Copy link

count() simply adds , count(*) as aggregate_count to your selects. That's the expected outcome of adding a count to a query.

What you described as your expected behavior of this function in your original post is not the result of adding count() to a query, but counting the results of a subquery.

You can do that in Laravel by either executing the query first and then counting the number of results you get, or by specifying a subquery to run the count on at the database level.
A grouped SQL count is not the same as just the number of rows. I suggest you read up on the behaviors of count and group by to clear up any doubts you may have about it.

But running count() on the same builder gives me wrong result.

The issue here is that you think you're calling the same method on the same object. You are not. In the first example, you're calling Support\Collection::count() which you can find here https://github.com/laravel/framework/blob/5.5/src/Illuminate/Support/Collection.php#L1672 whereas in your second example, you're calling Database\Query\Builder::count(), found here: https://github.com/laravel/framework/blob/5.5/src/Illuminate/Database/Query/Builder.php#L1965

In short, a count(*) on a grouped query will return the number of items that belong to each group. An Eloquent count() doesn't know about your query, and doesn't care either. It just tells you the number of rows that were returned.

@fico7489
Copy link
Author

fico7489 commented Feb 9, 2018

@36864 you are 100% but I already know what is happening behind scene, that second count() call adds "count(*) as aggregate_count" to select(), and first count() performs count() on collection, but that is not the question here, only question is if this behaviour is normal :

//result here is 10
Seller::select('sellers.* ')
    ->join('locations', 'locations.seller_id', '=', 'sellers.id')
    ->groupBy('sellers.id')
    ->get()
    ->count();

//result here is 1
Seller::select('sellers.* ')
    ->join('locations', 'locations.seller_id', '=', 'sellers.id')
    ->groupBy('sellers.id')
    ->count()

?

I don't think so, and I think that 99% people which use above eloquent example
expect 10 in second example.

@Miguel-Serejo
Copy link

I expect a grouped count to return the count of each group. That's literally what you're asking the builder to do.

I would not expect the behavior of Collection::count() to be the same as that of Builder::count() in this context, much like I don't expect Collection::groupBy() to behave like Builder::groupBy(), and I don't expect the result of calling Model::groupBy()->get()->count() to have the same result as Mode::all()->groupBy()->count().

While these methods have the same name, they behave very differently, and their behavior is well documented and has been in use for a long time. Changing it now would just cause confusion.

How would I get a grouped count if the method was changed as you're suggesting? Would I need to add a raw select to my query? That's inconvenient at best, not to mention a huge breaking change.

@fico7489
Copy link
Author

@36864 ok, then we are not expecting the same thing.

1.I am an expection same value for count() on builder and on collection
2.I am expection exact count() value for builder so I can calculate pagination values(hasMorePages, nextPage, totalPages etc.).

I would be happy with answer "yes, you are right, but this is breaking change we can only change this in laravel 6. x"

@timmcleod
Copy link

timmcleod commented Mar 23, 2018

I've encountered this on a few different occasions and it gets me every time (including today). Just noticed that the paginator uses a workaround for this problem @fico7489.

When paginating (is that a word? ha), builder uses $this->toBase()->getCountForPagination() to find the total number of records instead of just querying with ->count(). That method checks to see if the query is grouping, and if so, just counts the results using count($results).

/**
 * Get the count of the total records for the paginator.
 *
 * @param  array  $columns
 * @return int
 */
public function getCountForPagination($columns = ['*'])
{
    $results = $this->runPaginationCountQuery($columns);

    // Once we have run the pagination count query, we will get the resulting count and
    // take into account what type of query it was. When there is a group by we will
    // just return the count of the entire results set since that will be correct.
    if (isset($this->groups)) {
        return count($results);
    } elseif (! isset($results[0])) {
        return 0;
    } elseif (is_object($results[0])) {
        return (int) $results[0]->aggregate;
    }

    return (int) array_change_key_case((array) $results[0])['aggregate'];
}

So, slightly annoying, but looks like we're stuck with ->get()->count().

@hopeseekr
Copy link

hopeseekr commented May 24, 2018

Is there not a method to remove groupBy()s?

Right now, I do this:

$ids = $complexQuery->join()->join()->join()->where()->where()->where()
    ->select('id')->pluck('id');

$limitedQuery->whereIn('id, $ids);

The problem is when one of the methods that builds the complex query's joins and wheres also needs a groupBy. Then, $complexQuery->count() returns 1, while the number of rows returned by ->pluck()` can be many more.

To get past this unfortunate bug that seems to be in a WONT_FIX state, I use the following hack:

        $totalPeople = $peopleQuery->count();

        // Hack to get the real count if a groupBy had to be used in a filter.
        // See the official Laravel bug report for more info:
        // https://github.com/laravel/framework/issues/22883
        if ($totalPeople === 1) {
            $totalPeople = $peopleQuery->pluck('people.id')->count();
        }

I truly hope that this really helps someone else out.

@timmcleod Thank you for showing me that ->get()->count() works. My method saves a lot of memory, as it only deals with primary keys and no mass object creation.

@boonedev
Copy link

@hopeseekr - This should remove the groupings from the query builder so you can get the count:

$q = clone($query->getQuery());
$q->groups = null;
$total = $q->count();

@jes490
Copy link

jes490 commented Jul 4, 2018

One way to do it is to use DB::raw() expression as per this answer.

$sub = Abc::where(..)->groupBy(..); // Eloquent Builder instance

$count = DB::table( DB::raw("({$sub->toSql()}) as sub") )
    ->mergeBindings($sub->getQuery()) // you need to get underlying Query Builder
    ->count();

It's cleaner than remove groups and can be done in one line.

@svkmedia
Copy link

I agree it's not working good with joins and groupBy. I was also facing this problem when building pagination. I just applied all filters in query and then used ->get()->count()

$data = QUERY->get(); $dataCount = $data->count(); $data = $data->splice(OFFSET, LIMIT);

@lazluiz
Copy link

lazluiz commented Nov 14, 2018

I've had the same issue and I've just solved it using getCountForPagination().

So I did: $queryWithGroupBy->getQuery()->getCountForPagination();

The issue with $queryWithGroupBy->get()->count() is that the query gets executed before counting the results, which is useless since you could just do count($queryWithGroupBy->get()).

But if you want to get the number of rows before doing any queries (in my case I needed this number to calculate the query progress time), this is might be the one of the best ways.

@judgej
Copy link
Contributor

judgej commented Aug 3, 2019

Just hit this. Doing a $query->get()->count() can be avoided by doing the count in the database instead of in application memory:

select count(*) from (
...main query...
) as dummyAlias

If there is some way to "wrap" a query like this, then the correct results can be achieved. It may still involve a lot of work in the database (reducing the number of select columns just for the count may help).


These two work for me with an eloquent query in Lumen 5.8:

$query = MyModel::groupBy(...);

$count = \DB::table( \DB::raw("({$query->toSql()}) as sub") )
    ->mergeBindings($query->toBase())
    ->count();

$count = $query->toBase()->getCountForPagination();

They are variants on various solutions provided above. However, in Lumen 5.8 at least, $query->getQuery() for an eloquent query seems to return an eloquent query, so getBase() is used instead to get the query builder query. I feel like I must be missing something here, and maybe someone else can point out the flaws.

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

No branches or pull requests