#497 ActiveRecord#calculate broken for multiple fields in :group option - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#497 ✓committed

ActiveRecord#calculate broken for multiple fields in :group option

Reported by gix | June 27th, 2008 @ 03:45 AM | in 3.0.5

The docs to ActiveRecord#calculate state:

:group - An attribute name by which the result should

be grouped. Uses the GROUP BY SQL-clause.

Most likely to be compatible to finders. But this statement is only partially true. The method's algorithm assumes that :group contains only one field. When specifying multiple values -- e.g. :group => 'foo, bar' -- this does not work anymore.

If one were to use ActiveRecord#count to get only the size of a result set (as the will_paginate-plugin does for example) this used to work in Rails 2.0 and was broken by http://preview.tinyurl.com/3l77f7 [Rails-Changeset]. Now the final result array with grouped key => aggregated value pairs is totally broken because keys are overwritten:

>> Item.count(:group => 'item_id').size

=> 687

>> Item.count(:group => 'quality').size

=> 2

>> Item.count(:group => 'item_id, quality').size

=> 2 # item_id is primary key, should return 687

>> Item.count(:group => 'quality')

=> [[5, 2], [4, 685]]

>> Item.count(:group => 'item_id, quality')

=> [[4, 1], [5, 1]]

I do not know how this should or can be fixed. Ideally every field in :group should be taken into account and used as some sort of combined key. The major problem I see here is splitting the :group string into fields because a simple split(/\s*,\s*/) won't work for more advanced things like 'COALESCE(col1, col2)'.

Another solution would be to allow :group being an array, thus putting the splitting in the hands of the caller:

Model.calculate(:op, :group => ['c', 'COALESCE(c1, c2)'])

This should be changed for finders as well then.

Otherwise the docs should be changed to reflect that multiple fields are not possible.

Comments and changes to this ticket

josh September 30th, 2008 @ 05:53 PM
- State changed from “new” to “stale”
You flagged this item as spam.
Sebastian February 6th, 2009 @ 01:30 AM
+1 for fixing this issue.
You flagged this item as spam.
Seth Ladd March 27th, 2009 @ 10:22 PM
+1 for addressing this issue. or being much more explicit in the docs. I would want multiple group by support, achieved via an array as the original post suggests.
You flagged this item as spam.
Alex May 6th, 2009 @ 10:39 PM
+1 for having a way to group by multiple fields.
You flagged this item as spam.
jdwyah May 7th, 2009 @ 01:13 PM
+1 In the meantime, see my ugly hack: poor_mans_group_by.rb http://pastie.org/471055 coalesce deals with nulls and delimitters are chosen to be unicode values unlikely to be used in your code.
You flagged this item as spam.
Sebastian May 14th, 2009 @ 10:40 PM
Couldn't we use the returned row-hash (minus count_all) as the key?
You flagged this item as spam.
Matt White June 22nd, 2009 @ 02:04 AM
+1 for getting this fixed...
You flagged this item as spam.
Scott Brown June 25th, 2009 @ 04:21 AM
Here's another +1
You flagged this item as spam.
Jacob Kjeldahl July 8th, 2009 @ 03:26 PM
- Tag set to “activerecord, calculations, group, patch”
I have found a patch by Eric Lindvall here http://tinyurl.com/luas8x it is an attachment to another lighthouse ticket which I am unable to find.

The patch works against ActiveRecord v2.3.2 (For v2.1.0 check this gist http://gist.github.com/142343)

I have corrected a tiny error in it and attached it to this ticket.

From the test:
@@@ Ruby def test_should_group_by_multiple_fields
c = Account.count(1, :group => [:credit_limit, :firm_id]) assert_equal [[50, nil], [50, 1], [50, 6], [53, 9], [55, 6], [60, 2]], c.keys end
- multiple_group_columns_in_calculations.diff 5.6 KB
You flagged this item as spam.
Gavin Stark December 1st, 2009 @ 02:51 AM
Another +1. This patch would be very useful.
You flagged this item as spam.
Chris Taggart February 3rd, 2010 @ 08:31 AM
Another +1.
You flagged this item as spam.
James Brooks February 9th, 2010 @ 06:19 AM
+1
You flagged this item as spam.
Paul Holzberger February 19th, 2010 @ 04:35 PM
+1

for those looking for temporary support of this, there is a plugin i tracked down from some other references in this thread: http://github.com/patientslikeme/multi_field_group_by
Rizwan Reza May 19th, 2010 @ 10:33 AM
- no changes were found...
You flagged this item as spam.
Alex July 22nd, 2010 @ 11:42 PM
- Importance changed from “” to “Low”
Just created a patch that resolves this issue https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets.... It also works with COALESCE and other SQL functions. Please help verify the patch (see link above) if you guys are still interested in a solution.
You flagged this item as spam.
Alex August 23rd, 2010 @ 04:39 PM
Uploading a patch here in case any of you guys want to verify it.
- group_by_multiple_fields_in_calculations.diff 5.1 KB
You flagged this item as spam.
Sebastian September 11th, 2010 @ 03:24 AM
I verified your patch and made it work against the 3-0-stable branch. I also added one more test case for using sql-functions in the group clause.
- fixed_activerecord_calculations_when_grouped_by_multiple_fields.diff 4.4 KB
You flagged this item as spam.
Rafael Cardoso November 12th, 2010 @ 06:27 PM
+1
Santiago Pastorino November 12th, 2010 @ 07:57 PM
- State changed from “stale” to “open”
- Milestone set to “3.0.2”
- Assigned user set to “Aaron Patterson”
You flagged this item as spam.
Alex November 13th, 2010 @ 10:52 PM
I updated the patch to work against master branch.
- group_by_multiple_fields_in_calculations.diff 4.8 KB
Santiago Pastorino November 15th, 2010 @ 09:04 PM
- Milestone cleared.
[bulk edit]
Aaron Patterson November 16th, 2010 @ 12:07 AM
Hi everyone! I'd really like to push rails to do less SQL parsing. Parsing SQL is a dangerous game.

If we could support this via a list passed to the :group clause (as @gix suggests), would that be OK with everyone?
You flagged this item as spam.
Alex November 16th, 2010 @ 03:18 AM
I agree that is better. The only reason it was done that way was because the documentation when this issue was first opened hinted that the correct usage was to pass in a SQL frag. I updated the patches for master and 2-3-stable such that there is no SQL parsing. They are attached.
- group_by_multiple_fields_in_calculations.diff 4.7 KB
You flagged this item as spam.
Alex November 16th, 2010 @ 03:19 AM
Forgot to attach 2-3-stable patch.
- group_by_multiple_fields_in_calculations-2-3-stable.diff 5.4 KB
Aaron Patterson November 16th, 2010 @ 06:34 PM
@Alex excellent, thank you. I'll apply and push after verifying everything. Thanks for the patches!
Aaron Patterson November 16th, 2010 @ 07:10 PM
- State changed from “open” to “committed”
Applied and pushed. Thanks!
You flagged this item as spam.
Alex November 16th, 2010 @ 07:55 PM
Great news! Thank you.
You flagged this item as spam.
Ludovic Gasc January 4th, 2011 @ 11:06 AM
Are you sure this bug is fixed ?

I've tested with AR 3.0.3+your patch and AR 3.0.4.rc built with the Git repository, the SQL query is correct but not the response of the method.

code:
```
pp SavedForm.count(:group => 'agent_id, code_dispo')
```
result:
```
DEBUG -- :   SQL (1.4ms)  SELECT COUNT(*) AS count_all, agent_id, code_dispo AS agent_id_code_dispo FROM saved_forms GROUP BY agent_id, code_dispo
{"bte_vocale"=>3, "CB"=>1, "nc"=>1, "pasok"=>4, "wln"=>2, "ok"=>3}
```
```
# gem list

*** LOCAL GEMS ***

activemodel (3.0.4.rc)
activerecord (3.0.4.rc)
activesupport (3.0.4.rc)
```
I can't test with the master branch, I've this error: /usr/local/lib/ruby/gems/1.9.1/gems/activerecord-3.1.0.beta/lib/active_record/relation/calculations.rb:169:in `perform_calculation': undefined method `grep' for # (NoMethodError) from /usr/local/lib/ruby/gems/1.9.1/gems/activerecord-3.1.0.beta/lib/active_record/relation/calculations.rb:152:in `calculate' from /usr/local/lib/ruby/gems/1.9.1/gems/activerecord-3.1.0.beta/lib/active_record/relation/calculations.rb:147:in `calculate' from /usr/local/lib/ruby/gems/1.9.1/gems/activerecord-3.1.0.beta/lib/active_record/relation/calculations.rb:58:in `count' from /usr/local/lib/ruby/gems/1.9.1/gems/activerecord-3.1.0.beta/lib/active_record/base.rb:430:in `count' Thanks for your feedback.
You flagged this item as spam.
Ludovic Gasc January 4th, 2011 @ 11:17 AM
Alex, I've just tested with your patch of November 13th, 2010 @ 10:52 PM, it runs correctly.

Only your latest patch has a problem.

If I can help, don't hesitate to contact me.

Yours.
You flagged this item as spam.
Alex January 4th, 2011 @ 05:45 PM
The usage should be pp SavedForm.count(:group => [ 'agent_id', 'code_dispo']). The reason we do this is to avoid SQL parsing. This is not documented anywhere yet, so eventually we should be adding this to the rails doc.
Santiago Pastorino February 12th, 2011 @ 09:58 PM
- Milestone set to “3.0.5”
[bulk edit]
You flagged this item as spam.
Ben Woosley April 2nd, 2011 @ 09:18 PM
I can confirm this isn't fixed. And +1 for getting it fixed.
You flagged this item as spam.
Ben Woosley April 2nd, 2011 @ 09:20 PM
Also, I'll be taking another look at it this weekend.
You flagged this item as spam.
Ludovic Gasc May 2nd, 2011 @ 03:36 PM
I confirm this bug isn't fixed in 3.0.7.

I don't understand why the patch isn't applied on the source code since 6 months, what's the problem ?