Fixed count method to handle result from grouped relations. #209

Closed
wants to merge 1 commit into
from

4 participants

@budu

Because AR return an OrderedHash as count result for grouped relations.

@jgwconsulting

Nice fix budu! Can we have this in a release please mislav? Thank you :)

@jgwconsulting

Thinking about it maybe bradseefeld fix is slightly preferable as the reduce method is (I think...) unnecessary. I guess the best fix would involve stripping any GROUP BY clause before issuing the SQL to count the records.

@budu

Yeah, that would definitely be cleaner.

@jgwconsulting

I'm all talk and no action though... My excuse is that I'm an after dark refugee from C# and all this Rails and Github business is still a bit scary :)

Maybe change line 63 to:

excluded = [:order, :limit, :offset, :group]

@bradseefeld

You cannot exclude the group because that changes the query. The count will be completely wrong in almost all cases. Another option would be to change how we count: SELECT COUNT(*) FROM ({client_query}) but I believe this has some performance implications.

@jgwconsulting

Ooops, what an idiot, I didn't think that one through much, did I? I like your idea of wrapping it in a SELECT COUNT - still better performance-wise than returning the full result set I'd imagine...?

@bradseefeld

Thinking about it some more, this is probably the most accurate way to do it (using a subselect). It would be nice if the sub query (the original query) maintained an optional upper limit so that we could stop counting at say 10k results. #page_entries_info would return something like "25- 50 of many results" or something like that if we had reached that upper limit.

@jgwconsulting

That sounds like an excellent plan. How are you fixed for things to do today? ;)

@mislav
Owner

@budu: I shouldn't change the type returned by count since it's a method from Active Record.

If you want to get the count in your code, use total_entries instead of count. That method has a workaround in case of grouped queries.

@mislav mislav closed this Jan 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment