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

Conversation

afeld
Copy link

@afeld afeld commented May 3, 2013

Enables collecting of records into sets, grouped by distinct values for the
specified field. Leverages ActiveRecord::Relation to be far more
efficient than Enumerable#group_by when selecting based on a column
name.

Example:

User.group_by('role')
# => {
#   "normal" => #<ActiveRecord::Relation [...]>,
#   "admin" => #<ActiveRecord::Relation [...]>
# }

I work with a lot of people who are new to Rails, and I've had multiple people ask if there was a way to do this with ActiveRecord. Figured it was time to finally support it.

One question: should documentation be added suggesting an index on any column that group_by is being called on regularly? I haven't seen those kinds of tips anywhere else in the documentation, but think it might be beneficial.

Enables collecting of records into sets, grouped by distinct values for the
specified `field`. Leverages `ActiveRecord::Relation` to be far more
efficient than `Enumerable#group_by` when selecting based on a column
name.

    Example:

        User.group_by('role')
        # => {
        #   "normal" => #<ActiveRecord::Relation [...]>,
        #   "admin" => #<ActiveRecord::Relation [...]>
        # }
@egilburg
Copy link
Contributor

egilburg commented May 3, 2013

Could you provide some benchmarks to compare your group_by implementation performance to one that was previously available via Enumerable? Try both lazy (e.g. relations stay relations) and non lazy (all relations eventually become arrays)


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

@afeld
Copy link
Author

afeld commented May 4, 2013

Here you go: https://gist.github.com/afeld/5516129

The new group_by is also more memory-efficient, in that it doesn't need to hold all records in memory after they're grouped.

@egilburg
Copy link
Contributor

egilburg commented May 4, 2013

To have a basis for comparison, I was expecting to see your group_by vs Enumerable's group_by (i.e. how much performance gain we get after this patch as compared to before this patch). It seems your gist only gives benchmarks for after-patch case, unless I'm missing something. Could you run the same benchmarks before the patch was applied, and compare them to after-patch?


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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants