Skip to content
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

Make average compatible across ruby versions #34858

Merged
merged 2 commits into from
Jan 4, 2019
Merged

Make average compatible across ruby versions #34858

merged 2 commits into from
Jan 4, 2019

Conversation

albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Jan 3, 2019

Summary

Since Ruby 2.6.0 NilClass#to_d is returning BigDecimal 0.0. This breaks average compatibility with prior Ruby versions. This patch makes average return nil in all Ruby versions when there are no rows.

Other Information

Reverts #34601
Fixes #34850

r? @rafaelfranca

@@ -401,11 +401,17 @@ def type_cast_calculated_value(value, type, operation = nil)
case operation
when "count" then value.to_i
when "sum" then type.deserialize(value || 0)
when "average" then value.respond_to?(:to_d) ? value.to_d : value
when "average" then cast_average(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just enough value&.respond_to?(:to_d) ? value.to_d : value or value && value.respond_to?(:to_d) ? value.to_d : value.

Suggested change
when "average" then cast_average(value)
when "average" then value && value.respond_to?(:to_d) ? value.to_d : value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @kamipo change applied. 🙏

Since Ruby 2.6.0 NilClass#to_d is returning `BigDecimal` 0.0, this
breaks `average` compatibility with prior Ruby versions. This patch
makes `average` return `nil` in all Ruby versions when there are no
rows.
@albertoalmagro
Copy link
Contributor Author

r? @kamipo

@kamipo
Copy link
Member

kamipo commented Jan 3, 2019

Returning nil for average no records is preferable to me.

The reason that I pasted the all operation cases in #34601 (comment) was for when "sum".

def type_cast_calculated_value(value, type, operation = nil)
case operation
when "count" then value.to_i
when "sum" then type.deserialize(value || 0)
when "average" then value.respond_to?(:to_d) ? value.to_d : value
else type.deserialize(value)

Databases also returns nil for sum no records, but now Active Record regards no records sum as 0 instead of nil.

I believe that is historical reason, but it is a good timing to discuss the odd inconsistency for no records sum and average.

@rafaelfranca
Copy link
Member

I think it makes sense to sum return 0 and average return nil. For sum it doesn't make difference if there is no records or if there is many that the sum returns 0, but for average it does make a difference since you don't know if 0 is because you have no record or because you have two records with -10 and 10 as value.

@albertoalmagro albertoalmagro changed the title Make rails compatible across ruby versions Make average compatible across ruby versions Jan 4, 2019
@kamipo
Copy link
Member

kamipo commented Jan 4, 2019

I don't know I get your point, you means that the difference (1) and (3) is less important than the difference (2) and (4), right?

root@localhost [test] > create table t (c int);
Query OK, 0 rows affected (0.03 sec)

root@localhost [test] > select sum(c) from t; -- (1)
+--------+
| sum(c) |
+--------+
|   NULL |
+--------+
1 row in set (0.00 sec)

root@localhost [test] > select avg(c) from t; -- (2)
+--------+
| avg(c) |
+--------+
|   NULL |
+--------+
1 row in set (0.00 sec)

root@localhost [test] > insert into t values (10), (-10);
Query OK, 2 rows affected (0.00 sec)
Records: 2  Duplicates: 0  Warnings: 0

root@localhost [test] > select sum(c) from t; -- (3)
+--------+
| sum(c) |
+--------+
|      0 |
+--------+
1 row in set (0.00 sec)

root@localhost [test] > select avg(c) from t; -- (4)
+--------+
| avg(c) |
+--------+
| 0.0000 |
+--------+
1 row in set (0.00 sec)

@rafaelfranca
Copy link
Member

Yes. I can't think in a case where I need to know the sum of all records is 0 because there are no records or because all records are 0.

@kamipo
Copy link
Member

kamipo commented Jan 4, 2019

I see, I agree that the difference is less important.

@kamipo kamipo merged commit a5a22c4 into rails:master Jan 4, 2019
kamipo added a commit that referenced this pull request Jan 4, 2019
…ccross-ruby-versions

Make average compatible across ruby versions
wpolicarpo added a commit to rails-sqlserver/activerecord-sqlserver-adapter that referenced this pull request Jan 23, 2019
Ruby 2.6 added `to_d` to BigDecimal and it was causing some tests
to fail because of that.

See rails/rails#34858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants