Grouped query performance of total_count using Arel #1022
Conversation
kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb
Outdated
Show resolved
Hide resolved
| c | ||
| end | ||
| # Handle grouping with a subquery | ||
| @total_count = if c.group_values.any? |
There was a problem hiding this comment.
Can we get rid of if and always use the subquery approach? What would be the downside? Is it less efficient?
There was a problem hiding this comment.
Hum... there might be some issues when #total_count is called with a column_name...
There was a problem hiding this comment.
I wonder what is the use case for calling #total_count with specific column_name, though.
| # Handle grouping with a subquery | ||
| @total_count = if c.group_values.any? | ||
| sq = c.except(:select).select("1 AS record").arel.as("subquery") | ||
| c.model.connection.select_value Arel::SelectManager.new.from(sq).project(sq[:record].count) |
There was a problem hiding this comment.
Could also use Arel.star.count so it does not depend on alias set for subquery.
| c.model.connection.select_value Arel::SelectManager.new.from(sq).project(sq[:record].count) | |
| c.model.connection.select_value Arel::SelectManager.new.from(sq).project(Arel.star.count) |
|
Thank you @guigs for taking the time to look over this PR! I am currently on a different project from the one that's driving this change but will hopefully be able to come and work on this in the next couple of weeks. |
This test's data was built using a hash keyed by a name which was included twice, so it would always pass as long as total_count builds a valid query. Fixed by using a 2D array instead.
Improved performance for total_count on grouped ActiveRecord::Relation. This re-implements kaminari#979 using Arel to avoid issues kaminari#1012 and kaminari#1015
Using `Arel.star` so it does not depend on an alias set in the subquery, as suggested by @guigs on kaminari#1022.
Only count non-null values of column_name if supplied. This meets the semantics of the SQL count function without requiring that column_name is one of the grouped columns.
bendilley
force-pushed
the
total-count-grouped-arel
branch
from
a958f4e
to
221c68f
Compare
|
@guigs I have added two new commits to cover your N.B. I've also rebased this PR onto |
|
A lot of the Travis test failures for Rails 4.2 look like they're related 😞 Hopefully I'll have time to look at them soon... |
|
It seems that We've established from the reverted #979 that we need to build a clean query, and using The other option is to use raw SQL. I might reopen #1023 with a couple of modifications, to test whether that works better... |
|
How about dynamic checking for It would make it easier to migrate to preferred solution once support for Active Record < 5 is dropped. Regarding the logic for |
Arel::SelectManager.new without args is unsupported in ActiveRecord < 5.
|
@guigs please see my latest effort with SQL as a fallback for ActiveRecord < 5. Setting the two solutions side-by-side like this hopefully demonstrates that they really are doing the exact same thing. I'm also just going to throw-in this case against using Arel: it's not technically a public API anymore (see rails/rails#32723). |
|
@bendilley Fair point. How about unscoped.from(subquery, 'subquery').count |
|
Very cool! 🤣 Let me check that to understand why it is that not an option... |
|
Ok, so it also need to remove STI scope, as you are did for #1015. I'm not sure what's the issue with it, beside not being very nice syntax. |
@yuki24's concern was that AR evidently comes with a lot of baggage and, although |
Improved performance for total_count on grouped ActiveRecord::Relation.
This re-implements #979 using Arel to avoid issues #1012 and #1015
After implementing it this way, it became obvious that Arel really wasn't doing anything special that couldn't safely be achieved with plain SQL, so I'm opening a 2nd PR to proffer the choice...