-
Notifications
You must be signed in to change notification settings - Fork 64
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 in GROUP BY querying #234
Conversation
Very small comments. Otherwise looks awesome! For the count I think we should have a different method otherwise the return type will be weird. How about “select_counts_by_group” or “select_group_counts”? |
Can be added in a separate PR though. This is useful even without a count method! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specs look good and overall LGTM. Just retying to understand the result of group_by because it looks the same as without group
.
@@ -746,14 +746,10 @@ describe Avram::Query do | |||
|
|||
users = UserQuery.new.group(&.age).group(&.id) | |||
users.query.statement.should eq "SELECT #{User::COLUMN_SQL} FROM users GROUP BY users.age, users.id" | |||
users.map(&.name).should contain "Dwight" | |||
users.map(&.name).should eq ["Dwight", "Michael", "Jim"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so does it just return an array? What does group by do in this case. I thought it'd be a hash or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns an array here of .map()
, but UserQuery.new.group(&.age).group(&.id)
just returns the BaseQuery
object like normal. It is confusing though because technically in this query, the group is useless.
I see this more as building blocks for the next parts to come as well as the underlying access. But if you have suggestions, or whatever, I'm all ears. Postgres grouping hurts my head lol
Yeah let’s merge it! I think this is a good starting point |
Fixes #193
User.group(:status).count #=> {"active" => 100, "inactive" => 4}
We have
select_count
currently, but maybe that needs to become aware if there's some grouping?