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

[11.x] Support DB aggregate by group #53209

Merged
merged 5 commits into from
Nov 18, 2024
Merged

Conversation

GromNaN
Copy link
Contributor

@GromNaN GromNaN commented Oct 17, 2024

Currently, the aggregate methods ignore the group by clause. This PR change the result of

$builder->from('users')->groupBy('role')->count()

to return a Collection of [role, count] instead of a single total count.

- select         count(*) as aggregate from "users"
+ select "role", count(*) as aggregate from "users" group by "role" 

Result before: 40
Result after:

role aggregate
admin 10
user 30

The aggregate result is always in a column named aggregate and the "group by" columns are added to the result.

Why?

I'm implementing withCount for MongoDB models and hybrid MongoDB to SQL models. This implementation is done during eagerLoad by doing a count by foreign key in the related table (i.e. select foreign_id, count(*) from mytable where foreign_id in (1,2,3) group by foreign_id). I managed to do that for MongoDB, and this PR makes it work for hybrid MongoDB to SQL models.

Union and group by

This also works with unions. I have to change the generated SQL to apply the group by clause to the union result instead of the first query of the union.

For the following query:

$builder->from('users')
    ->union($this->getBuilder()->from('members'))
    ->groupBy('role')
    ->aggregate('count')

The generated SQL is before/after the PR:

- select count(*) as aggregate from ((select * from "users" group by "role") union (select * from "members")) as "temp_table" 
+ select "role", count(*) as aggregate from ((select * from "users") union (select * from "members")) as "temp_table" group by "role"

If anyone need to generate the previous SQL, the query builder for the 1st subquery would have to be wrapped:

  $builder
-    ->from('users')
-    ->groupBy('role')
+    ->from($this->getBuilder()->from('users')->groupBy('role'), 'alias')
     ->union($this->getBuilder()->from('members'))
     ->aggregate('count');

Return type of count/sum/avg/min/max

The return type of the aggregate methods indicate that a single value is expected: the result of the aggregation. But since the group by clause is used, the result is a collection of [...groups, aggregate]. I had to change the return type in phpdoc to add Collection. This may impact the count function only, as other return mixed that already includes Collection.

An alternative could have been to add a new methods sumBy($function, $column, ...$groups) but I think this is already the role of ->groupBy(...$groups) to specify the expected groups.

@GromNaN GromNaN changed the title [11.x] Support DB aggregation by group [11.x] Support DB aggregate by group Oct 17, 2024
@taylorotwell
Copy link
Member

taylorotwell commented Oct 31, 2024

@GromNaN Would this ever break something like:

$builder->from('users')->groupBy('role')->paginate();

We run count queries on pagination.

@GromNaN
Copy link
Contributor Author

GromNaN commented Oct 31, 2024

Things are well done: the pagination count query encapsulates the initial SQL query as a subquery in a select count(*) as aggregation from (<paginated sql>) as "aggregate table":

return $this->newQuery()
->from(new Expression('('.$clone->toSql().') as '.$this->grammar->wrap('aggregate_table')))
->mergeBindings($clone)
->setAggregate('count', $this->withoutSelectAliases($columns))
->get()->all();

@taylorotwell taylorotwell merged commit 6a491c2 into laravel:11.x Nov 18, 2024
31 checks passed
taylorotwell added a commit that referenced this pull request Nov 19, 2024
taylorotwell added a commit that referenced this pull request Nov 19, 2024
@GromNaN GromNaN deleted the group-aggregate branch November 27, 2024 00:51
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.

2 participants