[5.3] [WIP] Add relationship subqueries for avg, max, min and sum

[makuser]

I really liked withCount(), but I needed to sum a specific column of a relation, so I went ahead and implemented withSum():

Example:

// Model with relationship
class User extends Model {
    public function transactions() {
        return $this->hasMany(Transaction::class);
    }
}
// Query to get the Users with the sum of their transactions column 'amount'
$users = User::withSum('transactions', 'amount')->get();

Resulting query:

select *, (select sum(amount) from `transactions` where `transactions`.`user_id` = `users`.`id`) as `transactions_sum` from `users` where `users`.`deleted_at` is null

By using this function, you can easily orderBy('transactions_sum', 'desc'):

select *, (select sum(amount) from `transactions` where `transactions`.`user_id` = `users`.`id`) as `transactions_sum` from `users` where `users`.`deleted_at` is null order by `transactions_sum` desc

You can also apply additional constraints like you can with the function withCount(), in this example to only sum-up the transactions of the last 21 days and only those with a specific item_id.

$users = User::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();

Resulting query:

select *, (select sum(amount) from `transactions` where `transactions`.`user_id` = `users`.`id` and date(`created_at`) > ? and `item_id` in (?, ?, ?, ?, ?)) as `transactions_sum` from `users` where `users`.`deleted_at` is null order by `transactions_sum` desc

[makuser]

@terion-name @decadence

[terion-name]

hurray!

[terion-name]

BTW, maybe this should be generalized for all aggregations? withCount, withSum, withAvg, withMax, withMin. The difference de-facto will be only in sql function added

[makuser]

Well... It makes sense to generalize withSum, withAvg, withMax and withMin because they all apply to a specific column.
withCount on the other hand applies to the entire row, no matter of any column.

If you want you can go ahead and make a suggestion how to implement it.

[terion-name]

Oh, yes, count is failing out of the pattern.

Suggestion is fairly simple:

add getRelationAvgQuery and others in src/Illuminate/Database/Eloquent/Relations/Relation.php, or getRelationAggregateQuery that will accept aggregation function as first param.

extract current logic of withSum to protected function withAggregate($aggregate, $relation, $column) that will apply corresponding query and (to be explicit and avoid magic functions)

public function withSum($relation, $column)
{
   return $this->withAggregate('sum',$relation, $column);
}
public function withAvg($relation, $column)
{
   return $this->withAggregate('avg',$relation, $column);
}
// etc

[fernandobandeira]

You could actually create a method called withRaw

Instead of passing amount as a parameter and hard coding it like sum(amount) we could actually let ppl pass a DB::raw there, this way we generalize the functions and cover situations where you may have to do more operations before calling the sum, like sum(price - discount) for example

We could pass an optional third parameter to define the resulting column name, then if you desire you can add those methods withSum, withAvg and call this method behind the scenes similar to what @terion-name proposed, the difference is that we would have another method that we could use...

You could also generalize withCount with this since you'll pass a DB::raw to the method...

[decadence]

@makuser Thanks for your work. I hope it will be merged.

[makuser]

Travis test failed, because I changed the column name suffix / alias behavior and did not yet adjust the test case, because I wanted to clarify this first:

• If someone uses withCount('somecolumnname as mycount') the resulting column name should be mycount and not mycount_count as it is right now. Right? At least that is what I would expect when I explicitely use the as statement to specify an alias name. What do you think, Taylor?
• If someone uses withCount('somecolumnname') the resulting column name should be somecolumnname_count as it already is right now. This behavior right there is perfectly fine, because the developer did not specify an alias name.

@taylorotwell

[fernandobandeira]

Considering that this will change the behavior of withCount then it should probably target 5.4 (master), it should change btw, makes sense... 👍

[taylorotwell]

Can you summarize the breaking changes of this? @fernandobandeira @makuser

[makuser]

Can you summarize the breaking changes of this? @fernandobandeira @makuser

Sure thing. The breaking change would be, that withCount('column as alias') would no longer return alias_count but instead alias.

I think it should rightfully be alias, and not alias_count, because that's what was specified in the query.

[taylorotwell]

This doesn't work for me:

$users = App\User::withSum('transactions', 'amount')->get();

When I clone down the PR and try this with the correct tables in place and two transactions in the table I get "transactions_sum" as 0.

If I use withCount I get the correct count of 2 so the tables are setup correctly.

[makuser]

Which amount value do your two transactions have?

[taylorotwell]

[taylorotwell]

It's because "amount" is in quotes in the sum.

[taylorotwell]

This makes me wonder how good these integration tests are? Or if they are even running?

[makuser]

This makes me wonder how good these integration tests are? Or if they are even running?

They are running, I had them check against sum("amount") and not sum(amount).
Seems like these aggregation functions work different across MariaDB and MySQL versions... Damn.

Will investigate and fix.

[makuser]

So, now it's fixed. Sorry for that, I had to undo 59c89a6 and 8b3e271, because this wasn't working as expected. I thought we always need to escape the column name with double quotes (SQL-99 standard, or SQLite keywords) but it seems like in reality we are not supposed to do that.

Anyway, I now checked out the code again and tested it once against 10.0.28-MariaDB-0+deb8u1 and then mysql 5.5.53-0ubuntu0.14.04.1-log. Both seem to work fine without escaped column names, so I changed it back.

So what do you think @taylorotwell?

[taylorotwell]

I still don't think this is actually correct. The column should still be wrapped in the proper identifiers for the database driver. For example, for MySQL the ` character.

[fernandobandeira]

change this to

$this->query->grammar->wrap($column);

And call this directly for the withCount, * shouldn't be wrapped... that's what taylor is saying...

Tests should fail after you make this change, you'll have to update them accordingly to match the return...

[GrahamCampbell]

Might be better to target 5.4?

[JosephSilber]

We need real integration tests. Simply comparing the compiled SQL is not an integration test.

We should test this against models with proper relationships and test data, to ensure we actually get the results we expect.

[makuser]

We need real integration tests. Simply comparing the compiled SQL is not an integration test.

That is very true. Right now I simply took the tests that were already there for withCount() and extended them for my new functions.

We definitely do need some REAL integration tests, also because the escaping/wrapping is obviously different per SQL engine.
Right now the tests are written for SQLite, because that's what we are using phpunit with. MySQL would require different wrappings, and the exact same tests would fail when run with the MySQL data storage engine.

Anyone got an idea how to make some better integration tests?

[taylorotwell]

Closing this for now and maybe we can revisit on 5.4.

[makuser]

@taylorotwell when will be the time to bring this in for 5.4.?

It would actually be ready to merge with 5.3 right now. I fixed all of the code to actually work with the latest commits on upstream. I also adjusted all TestCases to work. These could be improved further, but they are also working as they are right now.

[alexweissman]

@makuser are you using this in production now?