Make inner sql for limit/offset eager limiting friendly. Also put in some doc comments for extensions.

Author: metaskills

lib/arel/visitors/sqlserver.rb

@@ -2,21 +2,27 @@ module Arel
 
   module Nodes
 
+    # See the SelectManager#lock method on why this custom class is needed.
     class LockWithSQLServer < Arel::Nodes::Unary
     end
 
+    # In versions of ActiveRecord prior to v3.0.3 limits and offset were always integers via 
+    # a #to_i. Somewhere between ActiveRecord and ARel this is not happening anymore nor are they 
+    # in agreement which should be responsible. Since we need to make sure that these are visited 
+    # correctly and that we can do math with them, these are here to cast to integers.
     class Limit < Arel::Nodes::Unary
       def initialize expr
         @expr = expr.to_i
       end
     end
-    
     class Offset < Arel::Nodes::Unary
       def initialize expr
         @expr = expr.to_i
       end
     end
 
+    # Extending the Ordering class to be comparrison friendly which allows us to call #uniq on a
+    # collection of them. See SelectManager#order for more details.
     class Ordering < Arel::Nodes::Binary
       def hash
         expr.hash
@@ -35,32 +41,38 @@ class SelectManager < Arel::TreeManager
 
     alias :lock_without_sqlserver :lock
 
+    # Getting real Ordering objects is very important for us. We need to be able to call #uniq on 
+    # a colleciton of them reliably as well as using their true object attributes to mutate them 
+    # to grouping objects for the inner sql during a select statment with an offset/rownumber. So this
+    # is here till ActiveRecord & ARel does this for us instead of using SqlLiteral objects.
     def order(*exprs)
       @ast.orders.concat(exprs.map{ |x|
         case x
         when Arel::Attributes::Attribute
           c = engine.connection
           tn = x.relation.table_alias || x.relation.name
-          expr = Nodes::SqlLiteral.new "#{c.quote_table_name(tn)}.#{c.quote_column_name(x.name)}"
-          Nodes::Ordering.new expr
+          expr = Arel::Nodes::SqlLiteral.new "#{c.quote_table_name(tn)}.#{c.quote_column_name(x.name)}"
+          Arel::Nodes::Ordering.new expr
         when String
           x.split(',').map do |s|
             expr, direction = s.split
-            expr = Nodes::SqlLiteral.new(expr)
+            expr = Arel::Nodes::SqlLiteral.new(expr)
             direction = direction =~ /desc/i ? :desc : :asc
-            Nodes::Ordering.new expr, direction
+            Arel::Nodes::Ordering.new expr, direction
           end
         else
-          expr = Nodes::SqlLiteral.new x.to_s
-          Nodes::Ordering.new expr
+          expr = Arel::Nodes::SqlLiteral.new x.to_s
+          Arel::Nodes::Ordering.new expr
         end
       }.flatten)
       self
     end
 
+    # A friendly over ride that allows us to put a special lock object that can have a default or pass 
+    # custom string hints down. See the visit_Arel_Nodes_LockWithSQLServer delegation method.
     def lock(locking=true)
       if Arel::Visitors::SQLServer === @visitor
-        @ast.lock = Nodes::LockWithSQLServer.new(locking)
+        @ast.lock = Arel::Nodes::LockWithSQLServer.new(locking)
         self
       else
         lock_without_sqlserver(locking)
@@ -121,17 +133,12 @@ def visit_Arel_Nodes_SelectStatementWithOutOffset(o, windowed=false)
         if windowed
           projections = function_select_statement?(o) ? projections : projections.map { |x| projection_without_expression(x) }
         elsif eager_limiting_select_statement?(o)
-          raise "visit_Arel_Nodes_SelectStatementWithOutOffset - eager_limiting_select_statement?"
           groups = projections.map { |x| projection_without_expression(x) }
           projections = projections.map { |x| projection_without_expression(x) }
-          # TODO: Let's alter objects vs strings and make new order objects
           orders = orders.map do |x|
-            Arel::SqlLiteral.new(x.split(',').reject(&:blank?).map do |c|
-              max = c =~ /desc\s*/i
-              c = clause_without_expression(c).sub(/(asc|desc)/i,'').strip
-              max ? "MAX(#{c})" : "MIN(#{c})"
-            end.join(', '))
-          end          
+            expr = Arel::Nodes::SqlLiteral.new projection_without_expression(x.expr)
+            x.descending? ? Arel::Nodes::Max.new([expr]) : Arel::Nodes::Min.new([expr])
+          end
         end
         [ ("SELECT" if !windowed),
           (visit(o.limit) if o.limit && !windowed),
@@ -266,8 +273,9 @@ def rowtable_orders(o)
         end.reverse.uniq.reverse
       end
 
+      # TODO: We use this for grouping too, maybe make Grouping objects vs SqlLiteral.
       def projection_without_expression(projection)
-        Arel::SqlLiteral.new(projection.split(',').map do |x|
+        Arel::Nodes::SqlLiteral.new(projection.split(',').map do |x|
           x.strip!
           x.sub!(/^(COUNT|SUM|MAX|MIN|AVG)\s*(\((.*)\))?/,'\3')
           x.sub!(/^DISTINCT\s*/,'')

test/cases/scratch_test_sqlserver.rb

@@ -1,30 +1,46 @@
 require 'cases/sqlserver_helper'
-require 'models/tagging'
 require 'models/post'
-require 'models/topic'
+require 'models/tagging'
+require 'models/tag'
 require 'models/comment'
-require 'models/reply'
 require 'models/author'
-require 'models/comment'
-require 'models/entrant'
-require 'models/developer'
+require 'models/category'
 require 'models/company'
-require 'models/bird'
-require 'models/car'
-require 'models/engine'
-require 'models/tyre'
+require 'models/person'
+require 'models/reader'
+require 'models/owner'
+require 'models/pet'
+require 'models/reference'
+require 'models/job'
+require 'models/subscriber'
+require 'models/subscription'
+require 'models/book'
+require 'models/developer'
+require 'models/project'
 
 class ScratchTestSqlserver < ActiveRecord::TestCase
 
-  fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics, 'warehouse-things', 
-           :authors, :categorizations, :categories, :posts
+  fixtures :posts, :comments, :authors, :author_addresses, :categories, :categories_posts,
+            :companies, :accounts, :tags, :taggings, :people, :readers,
+            :owners, :pets, :author_favorites, :jobs, :references, :subscribers, :subscriptions, :books,
+            :developers, :projects, :developers_projects
 
   should 'pass' do
-    topics = Topic.send(:with_scope, :find => {:limit => 1, :offset => 1, :include => :replies}) do
-      Topic.find(:all, :order => "topics.id")
+    Post.send(:with_scope, :find => { :conditions => "1=1" }) do
+      posts = authors(:david).posts.find(:all,
+        :include    => :comments,
+        :conditions => "comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'",
+        :limit      => 2
+      )
+      assert_equal 2, posts.size
+
+      count = Post.count(
+        :include    => [ :comments, :author ],
+        :conditions => "authors.name = 'David' AND (comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment')",
+        :limit      => 2
+      )
+      assert_equal count, posts.size
     end
-    assert_equal 1, topics.size
-    assert_equal 2, topics.first.id
   end