When using LIMIT/OFFSET without ordering try to order using projection rather than with the primary key

Author: aidanharan

CHANGELOG.md

@@ -3,6 +3,7 @@
 #### Fixed
 
 - [#1318](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1318) Reverse order of values when upserting
+- []() When using `LIMIT`/`OFFSET` without ordering try to order using projection rather than with the primary key.
 
 ## v8.0.5
 

lib/arel/visitors/sqlserver.rb

@@ -252,7 +252,11 @@ def visit_Arel_Nodes_SelectStatement_SQLServer_Lock(collector, options = {})
         collector
       end
 
+      # AIDO
       def visit_Orders_And_Let_Fetch_Happen(o, collector)
+
+        # binding.pry if $DEBUG
+
         make_Fetch_Possible_And_Deterministic o
         if o.orders.any?
           collector << " ORDER BY "
@@ -300,24 +304,48 @@ def select_statement_lock?
         @select_statement && @select_statement.lock
       end
 
+      # If LIMIT/OFFSET is used without ORDER BY, SQLServer will return an error.
+      # This method will add a deterministic ORDER BY clause to the query using following rules:
+      #   1. If the query has projections, use the first projection as the ORDER BY clause.
+      #   2. If the query has SQL literal projection, use the first part of the SQL literal as the ORDER BY clause.
+      #   3. If the query has a table with a primary key, use the primary key as the ORDER BY clause.
       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
+        # TODO: Refactor to list all projections and then find the first one that looks good.
+
+        projection = o.cores.first.projections.first
+
+
+        binding.pry if $DEBUG
+
+
+        if projection.is_a?(Arel::Attributes::Attribute) && !projection.name.include?("*")
+          o.orders = [projection.asc]
 
-        # Prefer deterministic vs a simple `(SELECT NULL)` expr.
-        o.orders = [pk.asc]
+        # TODO: Use better logic to find first projection that is usable for ordering.
+        elsif projection.is_a?(Arel::Nodes::SqlLiteral) && !projection.match?(/^\s*(1 as ONE|\*)(\s|,)*/i)
+
+          first_projection = Arel::Nodes::SqlLiteral.new(projection.split(",").first.split(/\sAS\s/i).first)
+          o.orders = [first_projection.asc]
+        else
+
+          pk = primary_Key_From_Table(table_From_Statement(o))
+          o.orders = [pk.asc] if pk
+        end
+
+      # rescue => e
+      #   binding.pry
       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 == ActiveRecord::FinderMethods::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/order_test_sqlserver.rb

@@ -150,4 +150,75 @@ class OrderTestSQLServer < ActiveRecord::TestCase
     sql = Post.order(:id).order("posts.id ASC").to_sql
     assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql
   end
+
+  describe "simple query containing limit" do
+    it "order by primary key if no projections" do
+      $DEBUG = false
+
+      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
+
+      $DEBUG = false
+    end
+
+    it "use order provided" do
+      # $DEBUG = true
+
+      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
+
+      # binding.pry
+
+    end
+
+    it "order by first projection if no order provided" do
+      # $DEBUG = true
+
+      sql = Post.select(:legacy_comments_count).limit(5).to_sql
+
+      assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
+
+      # binding.pry
+
+    end
+
+    it "order by first projection (when multiple projections) if no order provided" do
+      sql = Post.select(:legacy_comments_count, :tags_count).limit(5).to_sql
+
+      assert_equal "SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
+    end
+  end
+
+  describe "query containing FROM and limit" do
+    it "uses 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 the subquery the first projection is used for ordering if none 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].[legacy_comments_count] 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"
+
+      # binding.pry
+
+      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, [0, 5, 0]
+      end
+    end
+
+    it "in the subquery the primary key is used for ordering if none 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
 end