fix activerecord sum methods #814

rhiroe · 2025-03-07T04:40:20Z

When a block is given, it is delegated to Enumerable#sum. See Enumerable#sum for reference.

Since it is more type-friendly for the types of initial_value, the block's return value, and the method's return value to match, I will accept some imprecision and unify all types as T.

When no block is given, the calculate method processes the result, so the return value is not necessarily an Integer.

https://github.com/rails/rails/blob/v6.0.6.1/activerecord/lib/active_record/relation/calculations.rb#L127

github-actions · 2025-03-07T04:40:37Z

@rhiroe Thanks for your contribution!

Please follow the instructions below for each change.
See also: https://github.com/ruby/gem_rbs_collection/blob/main/docs/CONTRIBUTING.md

Available commands

You can use the following commands by commenting on this PR.

/merge: Merge this PR if CI passes

`activerecord`

You changed RBS files for an existing gem.
You need to get approval from the reviewers of this gem.

@hibariya, @ksss, @Little-Rubyist, @tk0miya, please review this pull request.
If this change is acceptable, please make a review comment including APPROVE from here.

After that, the PR author or the reviewers can merge this PR.
Just comment /merge to merge this PR.

rhiroe · 2025-03-07T06:00:48Z

I’m a bit confused after receiving a comment outside of the PR that the return value in the case of a block is not necessarily T.

ksss · 2025-03-07T08:45:53Z

How about delegating to Enumerable#sum?

https://github.com/ruby/rbs/blob/1209f49fd9073374d5660e8d9fc25ce2fc5c1af0/core/enumerable.rbs#L1806-L1809

ksss

I welcome the accuracy of type information, but not the inconvenience.

It is likely that adjusting the results based on the column type for each model would be the optimal approach, but in most cases, the type would be either Integer or Float.

What specific problem does this change solve for you?

ksss · 2025-03-08T01:39:55Z

gems/activerecord/6.0/activerecord.rbs

+      def sum: (?untyped? column_name) -> untyped
+             | [T] () { (Model) -> T } -> (Integer | T)


The intent of my comment was to use overloading.
I don't know if it will work.

Suggested change

def sum: (?untyped? column_name) -> untyped

| [T] () { (Model) -> T } -> (Integer | T)

def sum: (?untyped? column_name) -> untyped

| ...

I didn’t realize that overloading could be used here, but that makes perfect sense. I'll check it out.

It seems to result in an InvalidOverloadMethodError. 😢

rhiroe · 2025-03-08T02:19:12Z

It is likely that adjusting the results based on the column type for each model would be the optimal approach, but in most cases, the type would be either Integer or Float.

I agree.

What specific problem does this change solve for you?

The issue I'm facing is that summing Float values results in an Integer.

ksss

I generally agree with cases that involve a block.

For cases without a block, let me think about it a bit more.

ksss · 2025-03-10T05:15:46Z

gems/activerecord/6.0/activerecord.rbs

-         | () { (Model) -> Integer } -> Integer
+  def sum: (?untyped? column_name) -> untyped
+         | [T] () { (Model) -> T } -> (Integer | T)
+         | [U] (U initial_value) { (Model) -> untyped } -> U


The return value does not necessarily match initial_value.

User.sum(0) { |user| user.decimal_column } #=> BigDecimal

Also, if there are zero records, the type of initial_value seems to be returned.

User.sum(0) { |user| user.decimal_column } #=> Integer(0) User.sum(0.0) { |user| user.decimal_column } #=> Float(0.0)

If you value practicality,

Suggested change

| [U] (U initial_value) { (Model) -> untyped } -> U

| [T] (untyped initial_value) { (Model) -> T } -> T

If you value accuracy,

Suggested change

| [U] (U initial_value) { (Model) -> untyped } -> U

| [T, U] (T initial_value) { (Model) -> U } -> (T | U)

ksss · 2025-03-11T08:29:14Z

@rhiroe

Here’s my thought on cases without a block:

In conclusion, I believe untyped is the best choice.

To express it completely accurately, the sum method would need to be overridden for each column.

class User < ActiveRecord::Base
  def self.sum: (:id) -> Integer
              | (:hight) -> Float
              | ...
end

A simpler approach would be to use -> (String | Integer | Float | BigDecimal)?, which would be sufficient.

However, neither seems particularly convenient to me.
Ultimately, annotating it directly in the code seems like the most reasonable approach.

sum_of_hight = User.sum(:hight) #: Float

Therefore, I think untyped is the best option.

For cases with a block and an initial_value, I believe one of the methods I proposed would be appropriate. What do you think?

rhiroe · 2025-03-11T09:04:15Z

Thank you for your suggestion. +1.

I believe the type for cases with initial_value should be [T] (untyped initial_value) { (Model) -> T } -> T. While this may lack precision, it is more type-friendly if the types of initial_value, the block's return value, and the method's return value match, and I believe this is how it should be implemented. Therefore, I think it makes sense to prioritize usability over strict accuracy.

Therefore, I’m considering defining the type as follows:

def sum: (?untyped? column_name) -> untyped
       | [T] (?T initial_value) { (Model) -> T } -> T

At this point, I don’t see the need to differentiate the type based on the presence of initial_value. If a concrete issue arises from this approach, we can reconsider it at that time.

This is my current thought on the matter. What do you think?

ksss · 2025-03-12T01:37:09Z

[T] (?T initial_value) { (Model) -> T } -> T looks great to me!

When a block is given, it is delegated to Enumerable#sum. See `Enumerable#sum` for reference. Since it is more type-friendly for the types of `initial_value`, the block's return value, and the method's return value to match, I will accept some imprecision and unify all types as `T`. When no block is given, the calculate method processes the result, so the return value is not necessarily an Integer. https://github.com/rails/rails/blob/v6.0.6.1/activerecord/lib/active_record/relation/calculations.rb#L127

github-actions · 2025-03-25T05:59:03Z

@rhiroe Thanks for your contribution!

Please follow the instructions below for each change.
See also: https://github.com/ruby/gem_rbs_collection/blob/main/docs/CONTRIBUTING.md

Available commands

You can use the following commands by commenting on this PR.

/merge: Merge this PR if CI passes

`activerecord`

You changed RBS files for an existing gem.
You need to get approval from the reviewers of this gem.

@hibariya, @ksss, @Little-Rubyist, @tk0miya, please review this pull request.
If this change is acceptable, please make a review comment including APPROVE from here.

After that, the PR author or the reviewers can merge this PR.
Just comment /merge to merge this PR.

ksss

APPROVE

github-actions · 2025-03-25T07:11:12Z

Thanks for your review, @ksss!

@rhiroe, @ksss This PR is ready to be merged.
Just comment /merge to merge this PR.

rhiroe · 2025-03-25T08:41:45Z

/merge

rhiroe force-pushed the fix/activerecord/sum branch 2 times, most recently from 6e4e96d to e33cee7 Compare March 7, 2025 23:56

ksss reviewed Mar 8, 2025

View reviewed changes

rhiroe force-pushed the fix/activerecord/sum branch 2 times, most recently from ea5c246 to 34f877d Compare March 10, 2025 04:25

ksss reviewed Mar 10, 2025

View reviewed changes

rhiroe marked this pull request as draft March 13, 2025 07:48

rhiroe force-pushed the fix/activerecord/sum branch from 34f877d to c9ad1f5 Compare March 13, 2025 11:10

rhiroe force-pushed the fix/activerecord/sum branch from 0c805ab to 6bc88df Compare March 25, 2025 05:58

rhiroe marked this pull request as ready for review March 25, 2025 05:58

rhiroe requested a review from ksss March 25, 2025 06:00

ksss reviewed Mar 25, 2025

View reviewed changes

github-actions bot merged commit b20c7f2 into ruby:main Mar 25, 2025
7 checks passed

rhiroe deleted the fix/activerecord/sum branch March 25, 2025 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix activerecord sum methods #814

fix activerecord sum methods #814

rhiroe commented Mar 7, 2025 •

edited

Loading

github-actions bot commented Mar 7, 2025

rhiroe commented Mar 7, 2025

ksss commented Mar 7, 2025

ksss left a comment

ksss Mar 8, 2025

rhiroe Mar 8, 2025

rhiroe Mar 8, 2025

rhiroe commented Mar 8, 2025

ksss left a comment

ksss Mar 10, 2025 •

edited

Loading

ksss commented Mar 11, 2025 •

edited

Loading

rhiroe commented Mar 11, 2025

ksss commented Mar 12, 2025

github-actions bot commented Mar 25, 2025

ksss left a comment

github-actions bot commented Mar 25, 2025

rhiroe commented Mar 25, 2025

		def sum: (?untyped? column_name) -> untyped
		\| [T] () { (Model) -> T } -> (Integer \| T)

	\| [U] (U initial_value) { (Model) -> untyped } -> U
	\| [T] (untyped initial_value) { (Model) -> T } -> T

	\| [U] (U initial_value) { (Model) -> untyped } -> U
	\| [T, U] (T initial_value) { (Model) -> U } -> (T \| U)

fix activerecord sum methods #814

fix activerecord sum methods #814

Conversation

rhiroe commented Mar 7, 2025 • edited Loading

github-actions bot commented Mar 7, 2025

Available commands

activerecord

rhiroe commented Mar 7, 2025

ksss commented Mar 7, 2025

ksss left a comment

Choose a reason for hiding this comment

ksss Mar 8, 2025

Choose a reason for hiding this comment

rhiroe Mar 8, 2025

Choose a reason for hiding this comment

rhiroe Mar 8, 2025

Choose a reason for hiding this comment

rhiroe commented Mar 8, 2025

ksss left a comment

Choose a reason for hiding this comment

ksss Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

ksss commented Mar 11, 2025 • edited Loading

rhiroe commented Mar 11, 2025

ksss commented Mar 12, 2025

github-actions bot commented Mar 25, 2025

Available commands

activerecord

ksss left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 25, 2025

rhiroe commented Mar 25, 2025

rhiroe commented Mar 7, 2025 •

edited

Loading

`activerecord`

ksss Mar 10, 2025 •

edited

Loading

ksss commented Mar 11, 2025 •

edited

Loading

`activerecord`