-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Harmonize average aggregation with minimum and maximum #41418
Conversation
b9a0ec7
to
616b7d1
Compare
616b7d1
to
d24d254
Compare
d24d254
to
ed2fddb
Compare
@@ -389,6 +394,11 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc: | |||
result[key] = type_cast_calculated_value(row[column_alias], operation) do |value| | |||
type ||= column.try(:type_caster) || | |||
lookup_cast_type_from_join_dependencies(column_name.to_s) || Type.default_value | |||
|
|||
if operation == "average" && type.is_a?(Type::Integer) | |||
type = Type::Decimal.new |
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.
ditto #41418 (comment)
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.
type
comes in from above. It will be kept while looping. Isn't that an issue?
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 is intentionally kept since the block will be evaluated multiple times and lookup_cast_type_from_join_dependencies
is a bit expensive.
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.
Ok. I took in the second (identical) proposal as well because memoization is only there for expensive lookup_cast_type_from_join_dependencies
.
ed2fddb
to
5a12ba9
Compare
The real problem behind the previous implementation of average aggregation was not that float columns returned `BigDecimal` but that average skipped `ActiveModel` type casting. This change introduces handling for the only needed special case of average: integers. Now any fields based on `ActiveRecord::Type::Integer` will be casted to `BigDecimal` when aggregated with average.
5a12ba9
to
1a9b843
Compare
Follow up to rails#40351 and rails#41418. This fixes `average` on decimal and enum (on integer in general) attributes to not do `type.deserialize`. The precision and the scale on the column might be lower than the calculated result. And also, mapping the calculated result to enum label is quite meaningless, the mapping result is almost nil.
The real problem behind the previous implementation of average
aggregation was not that float columns returned
BigDecimal
but thataverage skipped
ActiveModel
type casting.This change introduces handling for the only needed special case
of average: integers. Any integer based fields will be casted to
BigDecimal
when aggregated with average now.