Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add group_by to ActiveRecord::FinderMethods #10447

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,12 @@
* Add `ActiveRecord::FinderMethods#group_by` method, which collects records into sets, grouped by distinct values for the specified `field`. This patch leverages `ActiveRecord::Relation` to be far more efficient than `Enumerable#group_by` when selecting based on a column name.

Example:

User.group_by('role')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use symbol rather than string here as an example here.

# => {"normal" => #<ActiveRecord::Relation [...]>, "admin" => #<ActiveRecord::Relation [...]>}

*Aidan Feldman*

* Handle aliased attributes in ActiveRecord::Relation.

When using symbol keys, ActiveRecord will now translate aliased attribute names to the actual column name used in the database:
Expand Down
22 changes: 22 additions & 0 deletions activerecord/lib/active_record/relation/finder_methods.rb
Expand Up @@ -176,6 +176,28 @@ def exists?(conditions = :none)
false
end

# Collect records into sets, grouped by distinct values for the specified +field+.
#
# User.select([:id, :name])
# => [#<User id: 1, name: "Oscar">, #<User id: 2, name: "Oscar">, #<User id: 3, name: "Foo">
#
# User.group_by(:name)
# => {"Foo" => #<ActiveRecord::Relation [...]>, "Oscar" => #<ActiveRecord::Relation [...]>}
def group_by(*args, &block)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you only expect one name, why do you accept varargrs? super (Enumerable's group_by) doesn't accept args at all (other than block), so there is currently no need for method signature to accept more than a single name.

Granted, it might be interesting for multi-arg group_by to work, returning nested structure such as:

array.group_by(:first_attr, :second_attr)

# =>
#  
#   { 
#    :first_value_1 => { :second_value_1 => subrelation_1, :second_value_2 => subrelation_2 }, 
#    :first_value_2 =>  { :second_value_1 => subrelation_3, :second_value_2 => subrelation_4 }
#  } 

But this topic can be explored in a different PR. For this one, I think its fine to only accept one name, defaulting to nil, and call super(&block) if its nil.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I tried

def group_by(field = nil, &block)
  # ...
  super(&block)
  # ...
end

and had ArgumentErrors... must be going nuts.

field = args[0]
if field.nil? || block_given?
super(&block)
else
result = {}
self.select(field).distinct.each do |item|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need to use self. prefix in this method for select or where

value = item[field]
result[value] = self.where(field => value)
end

result
end
end

# This method is called whenever no records are found with either a single
# id or multiple ids and raises a +ActiveRecord::RecordNotFound+ exception.
#
Expand Down
25 changes: 25 additions & 0 deletions activerecord/test/cases/finder_test.rb
Expand Up @@ -849,6 +849,31 @@ def test_finder_with_offset_string
assert_nothing_raised(ActiveRecord::StatementInvalid) { Topic.all.merge!(:offset => "3").to_a }
end

def test_group_by_with_symbol
groups = Company.all.group_by(:type)
assert_kind_of Hash, groups

keys = groups.keys
assert_equal 5, keys.size
assert_equal [nil, 'Client', 'DependentFirm', 'ExclusivelyDependentFirm', 'Firm'].to_set, keys.to_set

assert_equal [2, 3, 5, 10], groups['Client'].map(&:id).sort
assert_equal [1, 4], groups['Firm'].map(&:id).sort
assert_equal nil, groups['Nonexistent']
end

def test_group_by_with_block
groups = Company.all.group_by { |c| c.firm_id }

group = groups[4]
assert_kind_of Array, group
assert_equal 2, group.size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps have more detailed test here, to ensure both values are what you expect, e.g. by comparing ids or some other attributes

end

def test_group_by_with_no_args
assert_kind_of Enumerable, Company.all.group_by
end

protected
def bind(statement, *vars)
if vars.first.is_a?(Hash)
Expand Down