Skip to content

Commit

Permalink
Fix count(:all) to correctly work distinct with custom SELECT list
Browse files Browse the repository at this point in the history
Currently `count(:all)` with `distinct` doesn't work correctly because
SELECT list is always replaced to `*` or primary key in that case even
if having custom SELECT list.

And also, PostgreSQL has a limitation that ORDER BY expressions must
appear in select list for SELECT DISTINCT.

Therefore, we should not replace custom SELECT list when using
`count(:all)` with `distinct`.

Closes #31277.
  • Loading branch information
kamipo committed Dec 19, 2017
1 parent eb6bacc commit c6cd9a5
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 10 deletions.
4 changes: 4 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
* Fix `count(:all)` to correctly work `distinct` with custom SELECT list.

*Ryuta Kamizono*

* Using subselect for `delete_all` with `limit` or `offset`.

*Ryuta Kamizono*
Expand Down
18 changes: 8 additions & 10 deletions activerecord/lib/active_record/relation/calculations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ def perform_calculation(operation, column_name)
if operation == "count"
column_name ||= select_for_count
if column_name == :all
if distinct && (group_values.any? || !(has_limit_or_offset? && order_values.any?))
if distinct && (group_values.any? || select_values.empty? && order_values.empty?)
column_name = primary_key
end
elsif column_name =~ /\s*DISTINCT[\s(]+/i
Expand Down Expand Up @@ -249,7 +249,7 @@ def operation_over_aggregate_column(column, operation, distinct)
def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
column_alias = column_name

if operation == "count" && has_limit_or_offset?
if operation == "count" && (column_name == :all && distinct || has_limit_or_offset?)
# Shortcut when limit is zero.
return 0 if limit_value == 0

Expand Down Expand Up @@ -391,14 +391,12 @@ def select_for_count
end

def build_count_subquery(relation, column_name, distinct)
relation.select_values = [
if column_name == :all
distinct ? table[Arel.star] : Arel.sql(FinderMethods::ONE_AS_ONE)
else
column_alias = Arel.sql("count_column")
aggregate_column(column_name).as(column_alias)
end
]
if column_name == :all
relation.select_values = [ Arel.sql(FinderMethods::ONE_AS_ONE) ] unless distinct
else
column_alias = Arel.sql("count_column")
relation.select_values = [ aggregate_column(column_name).as(column_alias) ]
end

subquery = relation.arel.as(Arel.sql("subquery_for_count"))
select_value = operation_over_aggregate_column(column_alias || Arel.star, "count", false)
Expand Down
6 changes: 6 additions & 0 deletions activerecord/test/cases/calculations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,12 @@ def test_apply_distinct_in_count
end
end

def test_distinct_count_all_with_custom_select_and_order
accounts = Account.distinct.select("credit_limit % 10").order(Arel.sql("credit_limit % 10"))
assert_queries(1) { assert_equal 3, accounts.count(:all) }
assert_queries(1) { assert_equal 3, accounts.load.size }
end

def test_distinct_count_with_order_and_limit
assert_equal 4, Account.distinct.order(:firm_id).limit(4).count
end
Expand Down
6 changes: 6 additions & 0 deletions activerecord/test/cases/relations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,12 @@ def test_count_with_distinct
assert_equal 11, posts.distinct(false).select(:comments_count).count
end

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_update_all_with_scope
tag = Tag.first
Post.tagged_with(tag.id).update_all title: "rofl"
Expand Down

0 comments on commit c6cd9a5

Please sign in to comment.