Fixed a problem where sum used with a group was not returning a Hash.

[kuldeepaggarwal]

\cc @senny

[rafaelfranca]

There is already a PR for this. Could you check if it was merged? Thanks
On Apr 18, 2014 5:39 PM, "Kuldeep Aggarwal" notifications@github.com
wrote:

\cc @senny https://github.com/senny

You can merge this Pull Request by running

git pull https://github.com/kuldeepaggarwal/rails null_relation_sum_fix

Or view, comment on, or merge it at:

#14803
Commit Summary

• Fixed a problem where sum used with a group was not returning a
  Hash.

File Changes

• M activerecord/CHANGELOG.mdhttps://github.com/Fixed a problem where sum used with a group was not returning a Hash. #14803/files#diff-0(4)
• M activerecord/lib/active_record/null_relation.rbhttps://github.com/Fixed a problem where sum used with a group was not returning a Hash. #14803/files#diff-1(4)
• M activerecord/test/cases/relations_test.rbhttps://github.com/Fixed a problem where sum used with a group was not returning a Hash. #14803/files#diff-2(8)

Patch Links:

• https://github.com/rails/rails/pull/14803.patch
• https://github.com/rails/rails/pull/14803.diff

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14803
.

[kuldeepaggarwal]

@rafaelfranca It has already been merged. here it is #14773

[matthewd]

Hmm. @eric-chahin, I thought you specifically checked the behaviour of sum.

Are there any other methods we should be addressing at the same time?

[rafaelfranca]

Do we can close this one right?
On Apr 18, 2014 6:36 PM, "Kuldeep Aggarwal" notifications@github.com
wrote:

@rafaelfranca https://github.com/rafaelfranca It has already been
merged. here it is #14773 #14773

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14803#issuecomment-40846644
.

[kuldeepaggarwal]

@matthewd I think this was the last method in NullRelation which might cause trouble.

[matthewd]

@rafaelfranca no; that was for count, this is for sum.

[eric-chahin]

I was under the impression that it was only count that should return the hash and not sum via https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/calculations.rb#L17

[matthewd]

Okay, well by my reading of https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/calculations.rb#L213, we're still not there...

I see no reason that average/minimum/maximum (or more generally, anything that ends up inside calculate -> perform_calculation) won't end up returning a hash. Am I missing something?

[kuldeepaggarwal]

@matthewd: But these(avg/max/min) methods are not defined in the NullRelation.

[eric-chahin]

@matthewd Would we actually need to add those into NullRelation?

    def count(*)
      calculate :count, nil
    end

    def sum(*)
      calculate :sum, nil
    end

    def average(*)
      calculate :average, nil
    end

    def minimum(*)
      calculate :minimum, nil
    end

    def maximum(*)
      calculate :maximum, nil
    end

    def calculate(operation, _column_name, _options = {})
      if [:count, :sum, :average, :minimum, :maximum].include? operation
        group_values.any? ? Hash.new : 0
      else
        nil
      end
    end

Unless my initial assumption was correct that count is the special exception! 😬

[eric-chahin]

@matthewd: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/calculations.rb#L83
It seems like maximum was not intended return a hash? 😕

[kuldeepaggarwal]

@matthewd: any suggestions?

[matthewd]

@eric-chahin you're misreading: the preceding line is pulling a value from the hash with the key "Drake".

@kuldeepaggarwal well, as you point out, they're currently not defined here at all. So we don't have the problem that they're returning zero. 😄

But I don't see any reason to expect them to be missing... as far as I understand, NullRelation is intended to act just like any other relation.. so it seems those methods should in fact be here.

That said, while they can all return an empty hash when grouped, they should not return zero in the ungrouped situation... the average, minimum, and maximum of an empty set is nil, not 0.

[kuldeepaggarwal]

@matthewd Should I add those methods too?

[srbiv]

I'm running into this issue, is anyone still working on this?

[kuldeepaggarwal]

@matthewd any suggestions?

[matthewd]

@kuldeepaggarwal sorry.. yes, please add those.

[kuldeepaggarwal]

@matthewd Added other aggregate methods and its test cases.

[kuldeepaggarwal]

Thanks 💚 💙 💛 💜