I started rails issue #7121, but I think this needs some discussion as it’s not just a simple “bug”. I might take a stab at it myself, but I’d like to get a blessing or consensus on this:
Given a relation:
When .group is added for conditions, this forces .count into returning a grouped count instead of a flat numeric count.
When .select(“virtual_col”) and .order(“virtual_col”) are used, .count fails with a SQL error because the .select is rewritten for COUNT(*) but the .order is still there pointing to a non-existent column.
I found a solution for these by wrapping the relation in a subquery. This isolates the relation so calculate doesn’t see its group_values and select_values and can’t mess them up:
inner_scope = self.all # 3.2: use self.scoped
Currently I use this by manually wrapping my relation before before calling .count on it: relation.wrapped.count
Is there any reason why Relation#calculate shouldn’t work this way by default? For grouped counts, here’s what I suggest:
count() on a relation that uses group_values should just return a numeric count of records, not a hash.
grouped counts must be done explicitly: .count(group: “column”) or .count_by(“column”) should return a hash.
Currently, finder methods on calculate are deprecated so using .count(group: “column”) logs a warning, but with the wrapped method it all works.
How to proceed?
relation.to_a.count should all return Fixnums…
In #7121, I opened the discussion about separating
count into a
Currently there’s a deprecation on
count(:group => :column), which advises to chain a group relation before calling count. I think this is the wrong approach. I suggest either:
count(:group => :col) in favour of a new
count(group: col) as the only way to get grouped counts.
With the change I’m proposing, either option means any current
relation.group(:col).count would change from returning a Hash to returning a Fixnum.
I’d like to look into fixing this when I have some time soon, If you have a preference for either of the above directions, let me know.
I’d like to get some traction or feedback on issue #7121 so I know if it’s worth me doing it. What are the objections to:
- Fixing brittle calculation methods via subqueries?
- Requiring count(:group=>:author_id) option explicitly when we want a Hash instead of Fixnum?
Or am I the only one who thinks this is broken?
I posted my proposed changes to the Calculations API, and resulting SQL queries here:
I’m preferring the “calculations_b.rb” version over the one with a separate grouped_count method: it would be a less intrusive change, and when we need to pass a :group option anyway, it doesn’t make sense to repeat
count(group:...) is obvious.
There have been times when I would have liked to be able to somehow consistently get a fixnum count, instead of sometimes a fixnum and sometimes a hash that I just have to loop through and add up the values!
Did this ever get patched? I’m running into this issue currently and would love to know if it got merged anywhere.
No, I couldn’t really get any traction on this so I let it slide. The consensus on the Github issue was to leave the
count inconsistency as-is. The last word from jonleighton was:
@avit coming back to this, I am feeling less confident that anything needs to be changed in AR.
We already have a way to create subquery via
#from and you can then perform a count on the subquery if you wish. This to me seems sufficient to handle what is a rare case in most applications.
So if I understand correctly, the remaining objection is that
might return a Fixnum or a Hash depending on the situation. I probably wouldn’t have designed it like that in the first place, but given it’s there I’m not sure there is a strong argument for changing it…
I still disagree and would’ve loved to work on a solution before Rails 4, but that’s where it’s at.