What is the use case for using the number of counts of groups in a group query instead of the number of rows? #373

Closed
codeodor opened this Issue Mar 27, 2013 · 10 comments

5 participants

@codeodor

I'm talking about this line: https://github.com/amatsuda/kaminari/blob/master/lib/kaminari/models/active_record_relation_methods.rb#L19

Is there a reason that total_count method is not just like this:

c = except(:offset, :limit, :order)
connection.select_one("select count(*) as count from (#{c.to_sql}) subquery_for_count")['count']

I see this pull request implemented it to begin with: #216

But is there a use where you'd want Kaminari to count like that, instead of the count of the full set? For instance, I have used group by in a query, and the OrderedHash returned by #count is {nil=>29} and I can't figure out where the 29 comes from at all, but since it only has one key, I get one page of results, even though there are thousands of rows coming from the query.

If there is not a valid use for it, I was thinking of submitting a pull request which has it implemented like I've shown above.

Please let me know if that would be welcome, and thanks for your time!

@voke

+1

@gphil

I have the same issue, I don't think the counting implementation makes sense as it is either. I'm actually currently figuring out how to override it in the meantime.

@zzak zzak closed this Aug 7, 2013
@codeodor

Should we take the fact that the issue was closed as a sign a pull request is not welcome, or has this issue been fixed, or are you saying it's not a problem at all?

If you don't see a problem at all, can we get your ideas as to why the counting makes sense to keep as is? I ask that to help me determine if I need to maintain a separate fork to fix the issue for myself, or just change my code to accommodate how kaminari sees the world.

Thanks!

@zzak
Collaborator

Submit a patch and we can talk, thank you

@gphil

@zzak are you saying that if I write a patch changing the current semantics to what @codeodor is suggesting it would be well-received? I'm confused as well as to whether the current implementation is broken, or if there is a reason for it that I don't understand. I don't want to spend the effort figuring this out without any kind of assurance that it's in the spirit of what the project wants, but if it is I would be happy to take a look (if @codeodor isn't already.)

@zzak
Collaborator

It's really hard to say without a patch, but for example: if the patch consists of a sql interpolation, its doubtful that it would be accepted.

@gphil

OK, thanks, so the issue is with the interpolation rather than being conceptually wrong. I'll think about if there's a way around that but I'm not sure if there is without just pushing the interpolation upstream to ActiveRecord since ultimately you have to build up the subquery as a string somewhere. @codeodor do you have any ideas there?

@codeodor

I'm not advanced enough in my understanding of activerecord to understand why we can't trust it's #to_sql to generate safe SQL, but nevertheless, I think you can avoid the interpolation by changing the line

connection.select_one("select count(*) as count from (#{c.to_sql}) subquery_for_count")['count']

to something following this strategy:

 sm = Arel::SelectManager.new relation.engine
 select_value = "count(*) as count"
 sm.project(select_value).from(c.arel.as("subquery_for_count")).count

It's basically how Rails does a subquery for the count operation. If that's not close enough to working, have a look at https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/calculations.rb#L379 and see if you can unravel it backwards from there. Don't use it's private methods though, because that will become a pain to maintain!

If you have trouble let me know and I may have some time to look in depth later today.

Also wanted to say thanks to @zzak for being open to looking at a patch! ❤️

@codeodor

Pull request #431 includes an example test case demonstrating how this behavior is incorrect, and includes a fix for it.

@dup2

+1, please merge this - we have a similar case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment