Fix different count calculation when using size with DISTINCT select

[jvillarejo]

Summary

When using select with 'DISTINCT( ... )' if you use method size on a non loaded relation it overrides the column selected by passing :all so it returns different value than count.

Fixes #35214

[kamipo]

This change also affects to count(:all).
Can you add test case for count(:all) as well (e.g. ebc09ed)?

[jvillarejo]

Thanks for your reply! Totally! I will add more tests cases.

[jvillarejo]

Hey @kamipo I added more test cases, as some of them didn't gave the correct original count value when reassigning the column_name.

So in order to fix the issue and keep the functionality instead of changing the column_name, distinct is assigned with the select_for_count values.

I think this fixes the issue with the minor impact.

However in terms of code I don't think it's very understandable. Assigning distinct to select_for_count but then 4 lines below assign distinct to nil when column_name isn't :all is very confusing.

What do you think would be better?

Also I think the test cases about size shouldn't be on test/cases/calculations_test.rb file but inside test/cases/relation folder. Do you think that creating a file test/cases/relation/size_test.rb there would be a good approach?

Thanks!

[kamipo]

This should return 5, since it should be the same result with Account.select("DISTINCT accounts.firm_id").group("accounts.firm_id").load.size.

[jvillarejo]

Somehow executing:

Account.select("DISTINCT accounts.firm_id").group("accounts.firm_id").size
# { nil => 1, 1 => 1, 2 => 1, 6 => 2, 9 => 1 }

Now returns the hash. But I agree that it should return the same of:

Account.select("DISTINCT accounts.firm_id").group("accounts.firm_id").load.size
# 5

Should I investigate this for this PR ?

[kamipo]

Feel free to create another PR.

[localhostdotdev]

As an user, I expect .group.size (same as .group.count) to return an hash. And doing .load.size / to_a.size would return a collection of the instances of the model and then how many of them there are.

[kamipo]

Originally, relation.size behaves fragile, that is a breaking change though.

relation = Post.left_joins(:comments).group(:id)
relation.size # => return a hash
relation.each { |record| puts record.id }
relation.size # => return an integer

relation = Post.left_joins(:comments).group(:id)
relation.count # => return a hash
relation.each { |record| puts record.id }
relation.count # => return a hash

relation = Post.left_joins(:comments).group(:id)
relation.length # => return an integer
relation.each { |record| puts record.id }
relation.length # => return an integer

[jvillarejo]

But in my opinion size and load.size should have the same return value the difference should be that when using just size if the collection is loaded it will not execute a query but if it isn't it executes a query without loading the collection instances.

That they return different values when the collection is loaded and when it isn't is confusing.

[kamipo]

Also I think the test cases about size shouldn't be on test/cases/calculations_test.rb file

Yes, just put them in test/cases/relations_test.rb for now.

rails/activerecord/test/cases/relations_test.rb

Lines 957 to 973 in be442cc

	def test_size_with_distinct
	posts = Post.distinct.select(:author_id, :comments_count)
	assert_queries(1) { assert_equal 8, posts.size }
	assert_queries(1) { assert_equal 8, posts.load.size }
	end
	def test_size_with_eager_loading_and_custom_order
	posts = Post.includes(:comments).order("comments.id")
	assert_queries(1) { assert_equal 11, posts.size }
	assert_queries(1) { assert_equal 11, posts.load.size }
	end
	def test_size_with_eager_loading_and_custom_order_and_distinct
	posts = Post.includes(:comments).order("comments.id").distinct
	assert_queries(1) { assert_equal 11, posts.size }
	assert_queries(1) { assert_equal 11, posts.load.size }
	end

[jvillarejo]

There is a weird situation added a test with includes:

def test_size_with_eager_loading_and_manual_distinct_select_and_includes
  accounts = Account.select("DISTINCT accounts.firm_id").includes(:firm).order("firms.id")

  assert_queries(1) { assert_equal 5, accounts.size }
  assert_queries(1) { assert_equal 5, accounts.load.size }
end

It raises:
# ActiveRecord::StatementInvalid: SQLite3::SQLException: near "DISTINCT": syntax error
I can investigate further if you want.

But this also gets raised when using:

Account.distinct(:credit_limit).select("DISTINCT accounts.firm_id").count(:all)
# ActiveRecord::StatementInvalid: SQLite3::SQLException: near "DISTINCT": syntax error

What do you think about this?

[kamipo]

This is an issue for count with eager loading.
I'd prefer to address it in another PR, since it is a little complex than what this PR fixes.

[jvillarejo]

Ok I'll work it in another PR!

[kamipo]

Looks good to me.
Can you add changelog entry and squash your commits into one?

[jvillarejo]

Done! Thank you so much for your help! This is my first Rails PR 😄. Is it correct?

[kamipo]

Thanks! And congrats on your first Rails contribution 🎉