Skip to content

Commit c9e4c84

Browse files
committed
Don't repeat same expression in SELECT and GROUP BY clauses
This refactors `execute_grouped_calculation` and slightly changes generated GROUP BY queries, since I'd not prefer to repeat same expression in SELECT and GROUP BY clauses. Before: ``` SELECT COUNT(*) AS count_all, "topics"."author_name" AS topics_author_name, COALESCE(type, title) AS coalesce_type_title FROM "topics" GROUP BY "topics"."author_name", COALESCE(type, title) ``` After: ``` SELECT COUNT(*) AS count_all, "topics"."author_name" AS topics_author_name, COALESCE(type, title) AS coalesce_type_title FROM "topics" GROUP BY topics_author_name, coalesce_type_title ``` Although we generally don't guarantee to support Arel node constructed by user itself, this also fixes rails#24207.
1 parent 0856f7c commit c9e4c84

File tree

1 file changed

+20
-26
lines changed

1 file changed

+20
-26
lines changed

activerecord/lib/active_record/relation/calculations.rb

Lines changed: 20 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -308,25 +308,23 @@ def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
308308
end
309309

310310
def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
311-
group_attrs = group_values
311+
group_fields = group_values
312312

313-
if group_attrs.first.respond_to?(:to_sym)
314-
association = @klass._reflect_on_association(group_attrs.first)
315-
associated = group_attrs.size == 1 && association && association.belongs_to? # only count belongs_to associations
316-
group_fields = Array(associated ? association.foreign_key : group_attrs)
317-
else
318-
group_fields = group_attrs
313+
if group_fields.size == 1 && group_fields.first.respond_to?(:to_sym)
314+
association = klass._reflect_on_association(group_fields.first)
315+
associated = association && association.belongs_to? # only count belongs_to associations
316+
group_fields = Array(association.foreign_key) if associated
319317
end
320318
group_fields = arel_columns(group_fields)
321319

322-
group_aliases = group_fields.map { |field| column_alias_for(field) }
320+
group_aliases = group_fields.map { |field|
321+
field = connection.visitor.compile(field) if Arel.arel_node?(field)
322+
column_alias_for(field)
323+
}
323324
group_columns = group_aliases.zip(group_fields)
324325

325-
if operation == "count" && column_name == :all
326-
aggregate_alias = "count_all"
327-
else
328-
aggregate_alias = column_alias_for([operation, column_name].join(" "))
329-
end
326+
aggregate_alias = "#{operation}_#{column_name.to_s.downcase}"
327+
aggregate_alias = column_alias_for(aggregate_alias) unless aggregate_alias.match?(/\A\w+\z/)
330328

331329
select_values = [
332330
operation_over_aggregate_column(
@@ -345,7 +343,7 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
345343
}
346344

347345
relation = except(:group).distinct!(false)
348-
relation.group_values = group_fields
346+
relation.group_values = group_aliases
349347
relation.select_values = select_values
350348

351349
calculated_data = skip_query_cache_if_necessary { @klass.connection.select_all(relation.arel, nil) }
@@ -378,18 +376,14 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
378376
# column_alias_for("sum(id)") # => "sum_id"
379377
# column_alias_for("count(distinct users.id)") # => "count_distinct_users_id"
380378
# column_alias_for("count(*)") # => "count_all"
381-
def column_alias_for(keys)
382-
if keys.respond_to? :name
383-
keys = "#{keys.relation.name}.#{keys.name}"
384-
end
385-
386-
table_name = keys.to_s.downcase
387-
table_name.gsub!(/\*/, "all")
388-
table_name.gsub!(/\W+/, " ")
389-
table_name.strip!
390-
table_name.gsub!(/ +/, "_")
391-
392-
@klass.connection.table_alias_for(table_name)
379+
def column_alias_for(field)
380+
column_alias = field.to_s.downcase
381+
column_alias.gsub!(/\*/, "all")
382+
column_alias.gsub!(/\W+/, " ")
383+
column_alias.strip!
384+
column_alias.gsub!(/ +/, "_")
385+
386+
connection.table_alias_for(column_alias)
393387
end
394388

395389
def type_for(field, &block)

0 commit comments

Comments
 (0)