Merge pull request #29848 from kamipo/fix_distinct_count_with_order_and_limit

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

activerecord/CHANGELOG.md

+*   Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT` to keep the existing select list.
+
+    *Ryuta Kamizono*
+
 *   Fix `unscoped(where: [columns])` removing the wrong bind values
 
     When the `where` is called on a relation after a `or`, unscoping the column of that later `where`, it removed

activerecord/lib/active_record/collection_cache_key.rb

@@ -14,7 +14,7 @@ def collection_cache_key(collection = all, timestamp_column = :updated_at) # :no
         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

@@ -646,6 +646,10 @@ def empty_scope? # :nodoc:
       @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

@@ -111,7 +111,7 @@ def sum(column_name = nil)
     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
@@ -194,8 +194,13 @@ def perform_calculation(operation, column_name)
 
         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?
@@ -222,7 +227,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" && (limit_value || offset_value)
+        if operation == "count" && has_limit_or_offset?
           # Shortcut when limit is zero.
           return 0 if limit_value == 0
 
@@ -361,16 +366,19 @@ def select_for_count
       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 ? table[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)
 
-        sm = Arel::SelectManager.new relation.engine
-        select_value = operation_over_aggregate_column(column_alias, "count", distinct)
-        sm.project(select_value).from(subquery)
+        Arel::SelectManager.new(subquery).project(select_value)
       end
   end
 end

activerecord/test/cases/calculations_test.rb

@@ -241,6 +241,30 @@ def test_apply_distinct_in_count
     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_distinct_joins_count_with_order_and_limit
+    assert_equal 3, Account.joins(:firm).distinct.order(:firm_id).limit(3).count
+  end
+
+  def test_distinct_joins_count_with_order_and_offset
+    assert_equal 3, Account.joins(:firm).distinct.order(:firm_id).offset(2).count
+  end
+
+  def test_distinct_joins_count_with_order_and_limit_and_offset
+    assert_equal 3, Account.joins(:firm).distinct.order(:firm_id).limit(3).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]

activerecord/test/models/account.rb

+# frozen_string_literal: true
+
+class Account < ActiveRecord::Base
+  belongs_to :firm, class_name: "Company"
+  belongs_to :unautosaved_firm, foreign_key: "firm_id", class_name: "Firm", autosave: false
+
+  alias_attribute :available_credit, :credit_limit
+
+  def self.destroyed_account_ids
+    @destroyed_account_ids ||= Hash.new { |h, k| h[k] = [] }
+  end
+
+  # Test private kernel method through collection proxy using has_many.
+  def self.open
+    where("firm_name = ?", "37signals")
+  end
+
+  before_destroy do |account|
+    if account.firm
+      Account.destroyed_account_ids[account.firm.id] << account.id
+    end
+  end
+
+  validate :check_empty_credit_limit
+
+  private
+    def check_empty_credit_limit
+      errors.add("credit_limit", :blank) if credit_limit.blank?
+    end
+
+    def private_method
+      "Sir, yes sir!"
+    end
+end

activerecord/test/models/company.rb

@@ -192,37 +192,4 @@ class SpecialClient < Client
 class VerySpecialClient < SpecialClient
 end
 
-class Account < ActiveRecord::Base
-  belongs_to :firm, class_name: "Company"
-  belongs_to :unautosaved_firm, foreign_key: "firm_id", class_name: "Firm", autosave: false
-
-  alias_attribute :available_credit, :credit_limit
-
-  def self.destroyed_account_ids
-    @destroyed_account_ids ||= Hash.new { |h, k| h[k] = [] }
-  end
-
-  # Test private kernel method through collection proxy using has_many.
-  def self.open
-    where("firm_name = ?", "37signals")
-  end
-
-  before_destroy do |account|
-    if account.firm
-      Account.destroyed_account_ids[account.firm.id] << account.id
-    end
-    true
-  end
-
-  validate :check_empty_credit_limit
-
-  private
-
-    def check_empty_credit_limit
-      errors.add("credit_limit", :blank) if credit_limit.blank?
-    end
-
-    def private_method
-      "Sir, yes sir!"
-    end
-end
+require "models/account"