1 files changed, 40 insertions, 22 deletions

diff --git a/lib/arel/visitors/mssql.rb b/lib/arel/visitors/mssql.rb
index 23dc06a936..0e5b75ec59 100644
--- a/lib/arel/visitors/mssql.rb
+++ b/lib/arel/visitors/mssql.rb
@@ -1,6 +1,8 @@
 module Arel
   module Visitors
     class MSSQL < Arel::Visitors::ToSql
+      RowNumber = Struct.new :children
+
       private
 
       # `top` wouldn't really work here. I.e. User.select("distinct first_name").limit(10) would generate
@@ -10,30 +12,43 @@ module Arel
         ""
       end
 
-      def visit_Arel_Nodes_SelectStatement o
+      def visit_Arel_Visitors_MSSQL_RowNumber o, collector
+        collector << "ROW_NUMBER() OVER (ORDER BY "
+        inject_join(o.children, collector, ', ') << ") as _row_num"
+      end
+
+      def visit_Arel_Nodes_SelectStatement o, collector
         if !o.limit && !o.offset
-          return super o
+          return super
         end
 
-        select_order_by = "ORDER BY #{o.orders.map { |x| visit x }.join(', ')}" unless o.orders.empty?
-
         is_select_count = false
-        sql = o.cores.map { |x|
-          core_order_by = select_order_by || determine_order_by(x)
+        o.cores.each { |x|
+          core_order_by = row_num_literal determine_order_by(o.orders, x)
           if select_count? x
-            x.projections = [row_num_literal(core_order_by)]
+            x.projections = [core_order_by]
             is_select_count = true
           else
-            x.projections << row_num_literal(core_order_by)
+            x.projections << core_order_by
           end
+        }
 
-          visit_Arel_Nodes_SelectCore x
-        }.join
+        if is_select_count
+          # fixme count distinct wouldn't work with limit or offset
+          collector << "SELECT COUNT(1) as count_id FROM ("
+        end
+
+        collector << "SELECT _t.* FROM ("
+        collector = o.cores.inject(collector) { |c,x|
+          visit_Arel_Nodes_SelectCore x, c
+        }
+        collector << ") as _t WHERE #{get_offset_limit_clause(o)}"
 
-        sql = "SELECT _t.* FROM (#{sql}) as _t WHERE #{get_offset_limit_clause(o)}"
-        # fixme count distinct wouldn't work with limit or offset
-        sql = "SELECT COUNT(1) as count_id FROM (#{sql}) AS subquery" if is_select_count
-        sql
+        if is_select_count
+          collector << ") AS subquery"
+        else
+          collector
+        end
       end
 
       def get_offset_limit_clause o
@@ -46,26 +61,29 @@ module Arel
         end
       end
 
-      def determine_order_by x
-        unless x.groups.empty?
-          "ORDER BY #{x.groups.map { |g| visit g }.join ', ' }"
+      def determine_order_by orders, x
+        if orders.any?
+          orders
+        elsif x.groups.any?
+          x.groups
         else
-          "ORDER BY #{find_left_table_pk(x.froms)}"
+          pk = find_left_table_pk(x.froms)
+          pk ? [pk] : []
         end
       end
 
       def row_num_literal order_by
-        Nodes::SqlLiteral.new("ROW_NUMBER() OVER (#{order_by}) as _row_num")
+        RowNumber.new order_by
       end
 
       def select_count? x
         x.projections.length == 1 && Arel::Nodes::Count === x.projections.first
       end
 
-      # fixme raise exception of there is no pk?
-      # fixme!! Table.primary_key will be depricated. What is the replacement??
+      # FIXME raise exception of there is no pk?
+      # FIXME!! Table.primary_key will be deprecated. What is the replacement??
       def find_left_table_pk o
-        return visit o.primary_key if o.instance_of? Arel::Table
+        return o.primary_key if o.instance_of? Arel::Table
         find_left_table_pk o.left if o.kind_of? Arel::Nodes::Join
       end
     end