Merge pull request #22241 from kamipo/add_columns_for_distinct_for_mysql57

Add `columns_for_distinct` for MySQL 5.7 with ONLY_FULL_GROUP_BY

3 files changed, 67 insertions, 5 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index 7bf548fcba..a918a8b035 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -1028,11 +1028,12 @@ module ActiveRecord
       end
 
       # Given a set of columns and an ORDER BY clause, returns the columns for a SELECT DISTINCT.
-      # Both PostgreSQL and Oracle overrides this for custom DISTINCT syntax - they
+      # PostgreSQL, MySQL, and Oracle overrides this for custom DISTINCT syntax - they
       # require the order columns appear in the SELECT.
       #
       #   columns_for_distinct("posts.id", ["posts.created_at desc"])
-      def columns_for_distinct(columns, orders) #:nodoc:
+      #
+      def columns_for_distinct(columns, orders) # :nodoc:
         columns
       end
 
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index 863d8d3b8f..0615e646b1 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -773,6 +773,21 @@ module ActiveRecord
         end
       end
 
+      # In MySQL 5.7.5 and up, ONLY_FULL_GROUP_BY affects handling of queries that use
+      # DISTINCT and ORDER BY. It requires the ORDER BY columns in the select list for
+      # distinct queries, and requires that the ORDER BY include the distinct column.
+      # See https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html
+      def columns_for_distinct(columns, orders) # :nodoc:
+        order_columns = orders.reject(&:blank?).map { |s|
+          # Convert Arel node to string
+          s = s.to_sql unless s.is_a?(String)
+          # Remove any ASC/DESC modifiers
+          s.gsub(/\s+(?:ASC|DESC)\b/i, '')
+        }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }
+
+        [super, *order_columns].join(', ')
+      end
+
       def strict_mode?
         self.class.type_cast_config_to_boolean(@config.fetch(:strict, true))
       end
@@ -952,11 +967,13 @@ module ActiveRecord
         subsubselect = select.clone
         subsubselect.projections = [key]
 
+        # Materialize subquery by adding distinct
+        # to work with MySQL 5.7.6 which sets optimizer_switch='derived_merge=on'
+        subsubselect.distinct unless select.limit || select.offset || select.orders.any?
+
         subselect = Arel::SelectManager.new(select.engine)
         subselect.project Arel.sql(key.name)
-        # Materialized subquery by adding distinct
-        # to work with MySQL 5.7.6 which sets optimizer_switch='derived_merge=on'
-        subselect.from subsubselect.distinct.as('__active_record_temp')
+        subselect.from subsubselect.as('__active_record_temp')
       end
 
       def mariadb?
diff --git a/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb
new file mode 100644
index 0000000000..4efd728754
--- /dev/null
+++ b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb
@@ -0,0 +1,44 @@
+require "cases/helper"
+
+class Mysql2AdapterTest < ActiveRecord::Mysql2TestCase
+  def setup
+    @conn = ActiveRecord::Base.connection
+  end
+
+  def test_columns_for_distinct_zero_orders
+    assert_equal "posts.id",
+      @conn.columns_for_distinct("posts.id", [])
+  end
+
+  def test_columns_for_distinct_one_order
+    assert_equal "posts.id, posts.created_at AS alias_0",
+      @conn.columns_for_distinct("posts.id", ["posts.created_at desc"])
+  end
+
+  def test_columns_for_distinct_few_orders
+    assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1",
+      @conn.columns_for_distinct("posts.id", ["posts.created_at desc", "posts.position asc"])
+  end
+
+  def test_columns_for_distinct_with_case
+    assert_equal(
+      'posts.id, CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0',
+      @conn.columns_for_distinct('posts.id',
+        ["CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END"])
+    )
+  end
+
+  def test_columns_for_distinct_blank_not_nil_orders
+    assert_equal "posts.id, posts.created_at AS alias_0",
+      @conn.columns_for_distinct("posts.id", ["posts.created_at desc", "", "   "])
+  end
+
+  def test_columns_for_distinct_with_arel_order
+    order = Object.new
+    def order.to_sql
+      "posts.created_at desc"
+    end
+    assert_equal "posts.id, posts.created_at AS alias_0",
+      @conn.columns_for_distinct("posts.id", [order])
+  end
+end