When using FETCH without specific ordering, use projection to order

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - [#1318](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1318) Reverse order of values when upserting
 - [#1321](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1321) Fix SQL statement to calculate `updated_at` when upserting
+- [1322](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1322) When using `FETCH` without ordering try to order using projection rather than the primary key.
 
 ## v8.0.5
 

lib/arel/visitors/sqlserver.rb

@@ -9,6 +9,8 @@ class SQLServer < Arel::Visitors::ToSql
       FETCH0 = " FETCH FIRST (SELECT 0) "
       ROWS_ONLY = " ROWS ONLY"
 
+      ONE_AS_ONE = ActiveRecord::FinderMethods::ONE_AS_ONE
+
       private
 
       # SQLServer ToSql/Visitor (Overrides)
@@ -300,24 +302,68 @@ def select_statement_lock?
         @select_statement && @select_statement.lock
       end
 
+      # FETCH cannot be used without an order. If no order is given, then try to use the projections for the ordering.
+      # If no suitable projection is present, then fallback to using the primary key of the table.
       def make_Fetch_Possible_And_Deterministic(o)
         return if o.limit.nil? && o.offset.nil?
         return if o.orders.any?
 
-        t = table_From_Statement o
-        pk = primary_Key_From_Table t
-        return unless pk
+        if any_groupings?(o) || has_non_table_join_sources?(o)
+          if (projection = projection_to_order_by_for_fetch(o))
+            o.orders = [projection.asc]
+            return
+          end
+        end
+
+        if (pk = primary_Key_From_Table(table_From_Statement(o)))
+          o.orders = [pk.asc]
+        end
+      end
+
+      def any_groupings?(o)
+        o.cores.any? { |core| core.groups.present? }
+      end
+
+      def has_non_table_join_sources?(o)
+        o.cores.none? { |core| core.source.left.is_a?(Arel::Table) }
+      end
+
+      # Find the first projection or part of projection that can be used for ordering. Cannot use
+      # projections with '*' or '1 AS one' in them.
+      def projection_to_order_by_for_fetch(o)
+        o.cores.first.projections.each do |projection|
+          case projection
+          when Arel::Attributes::Attribute
+            return projection unless projection.name.include?("*")
+          when Arel::Nodes::SqlLiteral
+            projection.split(",").each do |p|
+              next if p.match?(/#{Regexp.escape(ONE_AS_ONE)}/i) || p.include?("*")
 
-        # Prefer deterministic vs a simple `(SELECT NULL)` expr.
-        o.orders = [pk.asc]
+              return Arel::Nodes::SqlLiteral.new(remove_last_AS_from_projection(p))
+            end
+          end
+        end
+
+        nil
+      end
+
+      # Remove last AS from projection. Example projections:
+      #   - 'name'
+      #   - 'name AS first_name'
+      #   - 'AVG(accounts.credit_limit AS DECIMAL) AS avg_credit_limit)'
+      def remove_last_AS_from_projection(projection)
+        parts = projection.split(/\sAS\s/i)
+        parts.pop if parts.length > 1
+        parts.join(" AS ")
       end
 
       def distinct_One_As_One_Is_So_Not_Fetch(o)
         core = o.cores.first
         distinct = Nodes::Distinct === core.set_quantifier
-        oneasone = core.projections.all? { |x| x == ActiveRecord::FinderMethods::ONE_AS_ONE }
-        limitone = [nil, 0, 1].include? node_value(o.limit)
-        if distinct && oneasone && limitone && !o.offset
+        one_as_one = core.projections.all? { |x| x == ONE_AS_ONE }
+        limit_one = [nil, 0, 1].include? node_value(o.limit)
+
+        if distinct && one_as_one && limit_one && !o.offset
           core.projections = [Arel.sql("TOP(1) 1 AS [one]")]
           o.limit = nil
         end

test/cases/fetch_test_sqlserver.rb

@@ -2,8 +2,11 @@
 
 require "cases/helper_sqlserver"
 require "models/book"
+require "models/post"
 
 class FetchTestSqlserver < ActiveRecord::TestCase
+  fixtures :posts
+
   let(:books) { @books }
 
   before { create_10_books }
@@ -59,6 +62,74 @@ class FetchTestSqlserver < ActiveRecord::TestCase
         query.to_sql
       end
     end
+
+    it "order by the provided orderings" do
+      sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
+
+      assert_queries_match(/#{Regexp.escape(sql)}/) do
+        result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
+        assert_equal result, [11, 5, 1]
+      end
+    end
+
+    it "in subquery order by first projection if no order provided" do
+      sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
+
+      assert_queries_match(/#{Regexp.escape(sql)}/) do
+        result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
+        assert_equal result, [10, 5, 0]
+      end
+    end
+
+    it "in subquery order by primary key if no projections and order provided" do
+      sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
+
+      assert_queries_match(/#{Regexp.escape(sql)}/) do
+        result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
+        assert_equal result, [10, 5, 0]
+      end
+    end
+  end
+
+  describe "GROUP query" do
+    it "order by primary key if not a GROUP query" do
+      assert_queries_match(/#{Regexp.escape("ORDER BY [posts].[id] ASC")}/i) do
+        Post.pick(:title)
+      end
+    end
+
+    it "invalid SQL generated by `find_nth_with_limit` adding primary key ordering" do
+      error = assert_raises(ActiveRecord::StatementInvalid) do
+        Post.select(:title, "count(*)").group(:title).first(2)
+      end
+      assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message)
+    end
+
+    it "valid SQL generated if order specified" do
+      assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title]")}/i) do
+        Post.select(:title, "count(*)").group(:title).order(:title).first(2)
+      end
+    end
+  end
+
+  describe "LIMIT query" do
+    it "order by primary key if no projections" do
+      sql = Post.limit(5).to_sql
+
+      assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
+    end
+
+    it "order by primary key if projections and no order provided" do
+      sql = Post.select(:legacy_comments_count).limit(5).to_sql
+
+      assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
+    end
+
+    it "use order provided" do
+      sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql
+
+      assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
+    end
   end
 
   protected