Fix COUNT(DISTINCT ...) with ORDER BY and LIMIT

Since #26972, `ORDER BY` is kept if `LIMIT` is presented for
performance. But in most SQL servers (e.g. PostgreSQL, SQL Server, etc),
`ORDER BY` expressions must appear in select list for `SELECT DISTINCT`.
We should not replace existing select list in that case.

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT` to keep the existing select list.
+
+    *Ryuta Kamizono*
+
 *   When a `has_one` association is destroyed by `dependent: destroy`,
     `destroyed_by_association` will now be set to the reflection, matching the
     behaviour of `has_many` associations.

activerecord/lib/active_record/collection_cache_key.rb

@@ -16,7 +16,7 @@ module ActiveRecord
         column = "#{connection.quote_table_name(collection.table_name)}.#{connection.quote_column_name(timestamp_column)}"
         select_values = "COUNT(*) AS #{connection.quote_column_name("size")}, MAX(%s) AS timestamp"
 
-        if collection.limit_value || collection.offset_value
+        if collection.has_limit_or_offset?
           query = collection.spawn
           query.select_values = [column]
           subquery_alias = "subquery_for_cache_key"

activerecord/lib/active_record/relation.rb

@@ -648,6 +648,10 @@ module ActiveRecord
       @values == klass.unscoped.values
     end
 
+    def has_limit_or_offset? # :nodoc:
+      limit_value || offset_value
+    end
+
     protected
 
       def load_records(records)

activerecord/lib/active_record/relation/calculations.rb

@@ -131,7 +131,7 @@ module ActiveRecord
     def calculate(operation, column_name)
       if has_include?(column_name)
         relation = construct_relation_for_association_calculations
-        relation = relation.distinct if operation.to_s.downcase == "count"
+        relation.distinct! if operation.to_s.downcase == "count"
 
         relation.calculate(operation, column_name)
       else
@@ -214,8 +214,13 @@ module ActiveRecord
 
         if operation == "count"
           column_name ||= select_for_count
-          column_name = primary_key if column_name == :all && distinct
-          distinct = nil if column_name =~ /\s*DISTINCT[\s(]+/i
+          if column_name == :all
+            if distinct && !(has_limit_or_offset? && order_values.any?)
+              column_name = primary_key
+            end
+          elsif column_name =~ /\s*DISTINCT[\s(]+/i
+            distinct = nil
+          end
         end
 
         if group_values.any?
@@ -242,7 +247,7 @@ module ActiveRecord
       def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
         column_alias = column_name
 
-        if operation == "count" && (limit_value || offset_value)
+        if operation == "count" && has_limit_or_offset?
           # Shortcut when limit is zero.
           return 0 if limit_value == 0
 
@@ -381,14 +386,18 @@ module ActiveRecord
       end
 
       def build_count_subquery(relation, column_name, distinct)
-        column_alias = Arel.sql("count_column")
-        subquery_alias = Arel.sql("subquery_for_count")
+        relation.select_values = [
+          if column_name == :all
+            distinct ? Arel.star : Arel.sql("1")
+          else
+            column_alias = Arel.sql("count_column")
+            aggregate_column(column_name).as(column_alias)
+          end
+        ]
 
-        aliased_column = aggregate_column(column_name == :all ? 1 : column_name).as(column_alias)
-        relation.select_values = [aliased_column]
-        subquery = relation.arel.as(subquery_alias)
+        subquery = relation.arel.as(Arel.sql("subquery_for_count"))
+        select_value = operation_over_aggregate_column(column_alias || Arel.star, "count", false)
 
-        select_value = operation_over_aggregate_column(column_alias, "count", distinct)
         Arel::SelectManager.new(subquery).project(select_value)
       end
   end

activerecord/test/cases/calculations_test.rb

@@ -236,6 +236,18 @@ class CalculationsTest < ActiveRecord::TestCase
     end
   end
 
+  def test_distinct_count_with_order_and_limit
+    assert_equal 4, Account.distinct.order(:firm_id).limit(4).count
+  end
+
+  def test_distinct_count_with_order_and_offset
+    assert_equal 4, Account.distinct.order(:firm_id).offset(2).count
+  end
+
+  def test_distinct_count_with_order_and_limit_and_offset
+    assert_equal 4, Account.distinct.order(:firm_id).limit(4).offset(2).count
+  end
+
   def test_should_group_by_summed_field_having_condition
     c = Account.group(:firm_id).having("sum(credit_limit) > 50").sum(:credit_limit)
     assert_nil        c[1]