Counting groupBy() queries returns wrong result

[tomlankhorst]

• Laravel Version: 5.8.24
• PHP Version: 7.3.5

Closed duplicates: #4306 #14123

Description:

Imagine a table bakeries which has an Eloquent model Bakery.

id	city_id
1	40
2	50
3	50

A query builder's count() method uses SQL COUNT() to determine the row count.

SELECT COUNT(*) AS aggregate FROM bakeries

That will yield the correct row number: 3

However, when using groupBy('city_id'), the following query will be built:

SELECT COUNT(*) AS aggregate FROM bakeries GROUP BY city_id

As COUNT() is an aggregate function, it will be evaluated for every group. So the result in this case will be: [1, 2]. Then, count() will just pick the first result and return that.

... but this is not the count that users expect.

// assumption: 
assert($query->count() === count($query->get()));

Steps To Reproduce:

Bakery::get(); // returns 3 rows
Bakery::count(); // returns 3

Bakery::groupBy('city_id')->get(); // returns 2 rows
Bakery::groupBy('city_id')->count(); // returns 1! Should return 2. 

Possible fix:

To determine the number of resulting records of a group by query, we could use:

1. subqueries
2. window functions
3. distinct count

of which the last is most wide-spread supported and compatible with the current query builder.

The implementation might look as follows:

--- vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php	(date 1561315905000)
+++ vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php	(date 1561315905000)
@@ -2450,7 +2451,15 @@
      */
     public function count($columns = '*')
     {
-        return (int) $this->aggregate(__FUNCTION__, Arr::wrap($columns));
+        $groups = $this->groups;
+
+        $query = $this->cloneWithout(['groups']);
+
+        if (!empty($groups) && $columns === '*') {
+            $columns = DB::raw('DISTINCT '.$this->implode(', ', $groups));
+        }
+
+        return (int) $query->aggregate(__FUNCTION__, Arr::wrap($columns));
     }
 
     /**

I'm not saying this is the way to go, just want to have a discussion going about the best solution to this situation.

Yes, this breaks. For now, I feel like we should add a note in the documentation. Something along the lines of:

Counting a groupBy()? drop the groupBy() call and use count(DB::raw('distinct group_col'))

[staudenmeir]

Please fix "5.9" in the title.

[staudenmeir]

IMO, ->groupBy('city_id')->count() is not really a valid query and Laravel shouldn't have to support it.

The DISTINCT query is possible without a raw expression:

Bakery::distinct()->count('city_id');

[tomlankhorst]

::distinct() creates a SELECT DISTINCT .... , but to count the result of a GROUP BY, SELECT COUNT(DISTINCT ...) is needed.

So, you would call it undefined behaviour when one calls ->count() on a query like this? I feel like the semantics of ->count() convey that it should return the... count, while in practice it adds an aggregate function to the SELECT clause, executes the query and returns the first result... which equals the count for non-grouped queries.

[staudenmeir]

Bakery::distinct()->count('city_id'); executes this query:

select count(distinct "city_id") as aggregate from "bakeries"

So, you would call it undefined behaviour when one calls ->count() on a query like this?

Yes, that's my opinion.

[tomlankhorst]

Well, I think it has to be clear what ->count() and friends do:

1. Count the number of results, analogous to a collection, always returns 1 record.
2. Execute an aggregate function, possibly with groups, so might return N records, 1 when not grouping.

Now it's a bit of in the middle and it does not make sense for GROUP BY queries.

[driesvints]

@tomlankhorst I'm sorry but I have to agree with @staudenmeir on this. This doesn't seems like a valid count query as it returns multiple rows. At the very least this seems more of a feature request then a bug so I suggest you open up an issue on the ideas repo and after discussing a good implementation maybe attempt a PR. But imo this shouldn't be something that should be accepted to the framework core.

[tomlankhorst]

It is a valid query, it's just an aggregate function on a grouped query which is perfectly deterministic and well-defined. It's just a choice of the framework to only return the first result of this aggregate.

An aggregate function performs a calculation on a set of values, and returns a single value. Except for COUNT, aggregate functions ignore null values. Aggregate functions are often used with the GROUP BY clause of the SELECT statement.
Aggregate Functions @ Microsoft

IMO the current implementation of blindly picking the first value is less valid than returning all values. However, as I see it, it would be an improvement to return the whole array of values if a GROUP BY is set.

[driesvints]

@tomlankhorst feel free to open up an issue on the ideas repo and maybe send in a pr to the master branch.

[staudenmeir]

The documentation you are quoting refers to queries like this:

select city_id, count(*)
from bakeries
group by city_id

That's not what Laravel's count() method is for. count() executes a query and returns a single value.

[staudenmeir]

When I said that people should use ->distinct()->count('column') instead of ->groupBy('column')->count(), I only considered queries where they have a choice.

But they don't have one when using pagination. Pagination does treat GROUP BY queries differently, but the implementation isn't very efficient. There's a warning about this in the docs.

I would be interested in working with you on a solution in the ideas repository.

[tomlankhorst]

That's not what Laravel's count() method is for. count() executes a query and returns a single value.

To me, that's the whole point: what should count() actually do?
I intuitively think it should return the record count that one would get when calling get(), and I feel that's what most of us expect it to do.
The current implementation achieves this, provided that you're not using grouping.

But you're right, this has its place at the ideas repository instead of the framework's issues page.