Skip to content

Commit

Permalink
Fix minimum and maximum on non numeric column
Browse files Browse the repository at this point in the history
I supposed all aggregation functions will return numeric result in
#39039, but that assumption was incorrect for `minimum` and `maximum`,
if an aggregated column is non numeric type.

I've restored type casting aggregated result for `minimum` and `maximum`.

Fixes #39110.
  • Loading branch information
kamipo committed May 2, 2020
1 parent 7669dc2 commit 767edaf
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 13 deletions.
54 changes: 41 additions & 13 deletions activerecord/lib/active_record/relation/calculations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -304,8 +304,17 @@ def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
query_builder = relation.arel
end

value = skip_query_cache_if_necessary { @klass.connection.select_value(query_builder) }
type_cast_calculated_value(value, operation)
result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder) }

type_cast_calculated_value(result.cast_values.first, operation) do |value|
if value.is_a?(String) &&
column = klass.columns_hash[column_name.to_s]
type = connection.lookup_cast_type_from_column(column)
type.deserialize(value)
else
value
end
end
end

def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
Expand Down Expand Up @@ -351,17 +360,33 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
key_records = key_records.index_by(&:id)
end

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.deserialize(row[aliaz])
}
key_types = group_columns.each_with_object({}) do |(aliaz, col_name), types|
types[aliaz] = type_for(col_name) do
calculated_data.column_types.fetch(aliaz, Type.default_value)
end
end

hash_rows = calculated_data.cast_values(key_types).map! do |row|
calculated_data.columns.each_with_object({}).with_index do |(column, hash), i|
hash[column] = row[i]
end
end

type = nil
hash_rows.each_with_object({}) do |row, result|
key = group_aliases.map { |aliaz| row[aliaz] }
key = key.first if key.size == 1
key = key_records[key] if associated

result[key] = type_cast_calculated_value(row[column_alias], operation)
result[key] = type_cast_calculated_value(row[column_alias], operation) do |value|
if value.is_a?(String) &&
(type || column = klass.columns_hash[column_name.to_s])
type ||= connection.lookup_cast_type_from_column(column)
type.deserialize(value)
else
value
end
end
end
end

Expand Down Expand Up @@ -389,9 +414,12 @@ def type_for(field, &block)

def type_cast_calculated_value(value, operation)
case operation
when "sum" then value || 0
when "average" then value&.respond_to?(:to_d) ? value.to_d : value
else value
when "count", "sum"
value || 0
when "average"
value&.respond_to?(:to_d) ? value.to_d : value
else # "minimum", "maximum"
yield value
end
end

Expand Down
7 changes: 7 additions & 0 deletions activerecord/test/cases/calculations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,13 @@ def test_aggregate_attribute_on_custom_type
assert_equal({ "proposed" => 0, "published" => 1 }, Book.group(:status).maximum(:difficulty))
end

def test_minimum_and_maximum_on_non_numeric_type
assert_equal Date.new(2004, 4, 15), Topic.minimum(:last_read)
assert_equal Date.new(2004, 4, 15), Topic.maximum(:last_read)
assert_equal({ false => Date.new(2004, 4, 15), true => nil }, Topic.group(:approved).minimum(:last_read))
assert_equal({ false => Date.new(2004, 4, 15), true => nil }, Topic.group(:approved).maximum(:last_read))
end

def test_select_avg_with_group_by_as_virtual_attribute_with_sql
rails_core = companies(:rails_core)

Expand Down

0 comments on commit 767edaf

Please sign in to comment.