mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Fix aggregate functions to return numeric value consistently even on custom attribute type
Currently, `count` and `average` always returns numeric value, but `sum`, `maximum`, and `minimum` not always return numeric value if aggregated on custom attribute type. I think that inconsistent behavior is surprising: ```ruby # All adapters except postgresql adapter are affected # by custom type casting. Book.group(:status).sum(:status) # => { "proposed" => "proposed", "published" => nil } ``` That is caused by fallback looking up cast type to `type_for(column)`. Now all supported adapters can return numeric value without that fallback, so I think we can remove that, it will also fix aggregate functions to return numeric value consistently.
This commit is contained in:
parent
43d3b60465
commit
89e1dd619e
3 changed files with 28 additions and 16 deletions
|
@ -1,3 +1,7 @@
|
|||
* Fix aggregate functions to return numeric value consistently even on custom attribute type.
|
||||
|
||||
*Ryuta Kamizono*
|
||||
|
||||
* Support bulk insert/upsert on relation to preserve scope values.
|
||||
|
||||
*Josef Šimánek*, *Ryuta Kamizono*
|
||||
|
|
|
@ -286,13 +286,12 @@ module ActiveRecord
|
|||
end
|
||||
|
||||
def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
|
||||
column_alias = column_name
|
||||
|
||||
if operation == "count" && (column_name == :all && distinct || has_limit_or_offset?)
|
||||
# Shortcut when limit is zero.
|
||||
return 0 if limit_value == 0
|
||||
|
||||
query_builder = build_count_subquery(spawn, column_name, distinct)
|
||||
skip_query_cache_if_necessary { @klass.connection.select_value(query_builder) }
|
||||
else
|
||||
# PostgreSQL doesn't like ORDER BY when there are no GROUP BY
|
||||
relation = unscope(:order).distinct!(false)
|
||||
|
@ -309,16 +308,14 @@ module ActiveRecord
|
|||
relation.select_values = [select_value]
|
||||
|
||||
query_builder = relation.arel
|
||||
end
|
||||
|
||||
result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder, nil) }
|
||||
row = result.first
|
||||
value = row && row.values.first
|
||||
type = result.column_types.fetch(column_alias) do
|
||||
type_for(column_name)
|
||||
end
|
||||
result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder, nil) }
|
||||
row = result.rows.first
|
||||
value = row && row.first
|
||||
type = result.column_types.fetch(column_alias, Type.default_value)
|
||||
|
||||
type_cast_calculated_value(value, type, operation)
|
||||
type_cast_calculated_value(value, type, operation)
|
||||
end
|
||||
end
|
||||
|
||||
def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
|
||||
|
@ -367,19 +364,19 @@ module ActiveRecord
|
|||
key_records = key_records.index_by(&:id)
|
||||
end
|
||||
|
||||
Hash[calculated_data.map do |row|
|
||||
calculated_data.each_with_object({}) do |row, result|
|
||||
key = group_columns.map { |aliaz, col_name|
|
||||
type = type_for(col_name) do
|
||||
calculated_data.column_types.fetch(aliaz, Type.default_value)
|
||||
end
|
||||
type_cast_calculated_value(row[aliaz], type)
|
||||
type.deserialize(row[aliaz])
|
||||
}
|
||||
key = key.first if key.size == 1
|
||||
key = key_records[key] if associated
|
||||
|
||||
type = calculated_data.column_types.fetch(aggregate_alias) { type_for(column_name) }
|
||||
[key, type_cast_calculated_value(row[aggregate_alias], type, operation)]
|
||||
end]
|
||||
type = calculated_data.column_types.fetch(aggregate_alias, Type.default_value)
|
||||
result[key] = type_cast_calculated_value(row[aggregate_alias], type, operation)
|
||||
end
|
||||
end
|
||||
|
||||
# Converts the given field to the value that the database adapter returns as
|
||||
|
@ -404,7 +401,7 @@ module ActiveRecord
|
|||
@klass.type_for_attribute(field_name, &block)
|
||||
end
|
||||
|
||||
def type_cast_calculated_value(value, type, operation = nil)
|
||||
def type_cast_calculated_value(value, type, operation)
|
||||
case operation
|
||||
when "count" then value.to_i
|
||||
when "sum" then type.deserialize(value || 0)
|
||||
|
|
|
@ -990,6 +990,17 @@ class CalculationsTest < ActiveRecord::TestCase
|
|||
assert_equal({ "proposed" => 2, "published" => 2 }, Book.group(:status).count)
|
||||
end
|
||||
|
||||
def test_aggregate_attribute_on_custom_type
|
||||
assert_equal 4, Book.sum(:status)
|
||||
assert_equal 1, Book.sum(:difficulty)
|
||||
assert_equal 0, Book.minimum(:status)
|
||||
assert_equal 1, Book.maximum(:difficulty)
|
||||
assert_equal({ "proposed" => 0, "published" => 4 }, Book.group(:status).sum(:status))
|
||||
assert_equal({ "proposed" => 0, "published" => 1 }, Book.group(:status).sum(:difficulty))
|
||||
assert_equal({ "proposed" => 0, "published" => 2 }, Book.group(:status).minimum(:status))
|
||||
assert_equal({ "proposed" => 0, "published" => 1 }, Book.group(:status).maximum(:difficulty))
|
||||
end
|
||||
|
||||
def test_select_avg_with_group_by_as_virtual_attribute_with_sql
|
||||
rails_core = companies(:rails_core)
|
||||
|
||||
|
|
Loading…
Reference in a new issue