[5.0] Refactor how aggregates are calculated #6985
Conversation
| * @param array $columns | ||
| * @return mixed | ||
| */ | ||
| protected function aggregateQuery($function, $columns = array('*')) |
There was a problem hiding this comment.
The additional method is for clarity. Moreover, we could chose to make it public such that it can be used from the tests and then calling toSql to check the query using PHPUnit asserts instead of relying on Mockery to intercept the calls to the database.
|
It's not possible to solve some of the pagination issues. This just does the full query and counts. That's fine but it has performance implications. You can see Doctrine just throws exceptions in the few situations that pagination can not be solved correctly, which is something we need to do as well. |
|
Regarding performance implications: I'd like to see those as I don't suspect there to be much difference. Otherwise this can be made smarter by optionally doing the subtable. Can you show queries which will not work, as I cannot think of any. |
|
You're just selecting the entire query and then counting it... that's the On Mon, Jan 12, 2015 at 12:12 PM, Joost Koehoorn [email protected]
|
|
Some benchmarking tells me that it is approx. 10% slower, when only the necessary field is selected. This is more than I expected really. We can chose to only use the subquery approach when a group by clause is present, which would actually solve some issues. |
|
I'd rather just throw exceptions. It's not performant, especially on large On Mon, Jan 12, 2015 at 12:46 PM, Joost Koehoorn [email protected]
|
|
@JoostK, @taylorotwell is #6191 still broken then? |
|
Yes. I agree that we need to detect when this subtable approach is required, as apparently it is somewhat more expensive (like I said, I would have thought this would be optimized for any performance degradation to be negligent, however I tested in MySQL and it was actually more expensive depending on the size of your table.) |
This PR tends to show a change to how aggregate queries are constructed, which is more robust than the current implementation. It reuses #5577 (however slightly altered) to allow for selecting from a query, which is required here.
This should solve #6191 and numerous issues from the past with queries using
havingand pagination.This definitely needs some thorough review and I don't expect this to merged in for 5.0, considering the unforeseen effects this may have.
See inline comments for some reasoning behind the changes.