Add Queryable#group_count functionality #778
Conversation
src/avram/queryable.cr
Outdated
| queryable: schema_class.name, | ||
| ) do |rs| | ||
| k = query.groups.map do | ||
| rs.read PG::PGValue | Int64 # TODO: Why isn't Int64 part of PGValue? |
There was a problem hiding this comment.
@jwoertink @robcole and I were talking about this, not sure why it's not a part of PG::PGValue. Maybe @will knows?
There was a problem hiding this comment.
I think it may just be an oversight. I'd open up a PR adding that, plus the other supported Ints.
src/avram/queryable.cr
Outdated
| def group_count | ||
| database.query_all( |
There was a problem hiding this comment.
We will want to make sure we raise an error here if you try to call it when there's no groups. Would be even better if we could make it a compile-time error somehow...
There was a problem hiding this comment.
It still works if there aren't groups. It will return a count of all records with an empty array as the grouping key.
There was a problem hiding this comment.
Yep! It becomes 1 group of [], which is canonically correct!
|
@matthewmcgarvey Still a draft but thanks for your support! |
grepsedawk
force-pushed
the
do-the-impossible
branch
from
438e547 to
9a857b5
Compare
grepsedawk
changed the title
grepsedawk
force-pushed
the
do-the-impossible
branch
from
9a857b5 to
9bcc140
Compare
grepsedawk
force-pushed
the
do-the-impossible
branch
from
9bcc140 to
63effb0
Compare
There was a problem hiding this comment.
Overall I think this is great. Really great work on this.
My only concern is the issue when there's no grouping. It seems that when there's no group, you get an empty array. Then ([] + ["COUNT(*)"]).statement would just end up being the same as the select_count method. If you feel this is a case that may be wanted, we could have an escape hatch method group_count? that would just give the value. Then group_count would do the raise. I say group_count for raising instead of group_count! because I would want it to be the default use. What are your thoughts?
|
@grepsedawk just need to fix the formatting issue |
By using `Model.group(&.column).group_count`, it allows for a Hash of mapped values to their respective counts Resolves luckyframework#777
grepsedawk
force-pushed
the
do-the-impossible
branch
from
87d5662 to
f7bbcf6
Compare
|
🎉 |
No description provided.