Merge branch 'master' into Khronos/master

* master:
  visitors can define their own cache strategy for dispatch. fixes #57
  Break Ordering into Ascending/Descending nodes, allow reversal
  remove unnecessary guarding agains literal
  LIMIT and OFFSET support for MS SQL
  Include Arel::Predicates to Arel::Nodes::Function so you can do table[:id].count.eq(2)
  updating spec
  zomg prep release
  make sure thread runs
  do not cache sql literal values
  no longer use this instance variable

5 files changed, 76 insertions, 18 deletions

diff --git a/lib/arel/visitors/depth_first.rb b/lib/arel/visitors/depth_first.rb
index 914b2d1999..6f9385de1b 100644
--- a/lib/arel/visitors/depth_first.rb
+++ b/lib/arel/visitors/depth_first.rb
@@ -22,6 +22,7 @@ module Arel
       alias :visit_Arel_Nodes_Not               :unary
       alias :visit_Arel_Nodes_Offset            :unary
       alias :visit_Arel_Nodes_On                :unary
+      alias :visit_Arel_Nodes_Ordering          :unary
       alias :visit_Arel_Nodes_Top               :unary
       alias :visit_Arel_Nodes_UnqualifiedColumn :unary
 
@@ -75,7 +76,6 @@ module Arel
       alias :visit_Arel_Nodes_NotEqual           :binary
       alias :visit_Arel_Nodes_NotIn              :binary
       alias :visit_Arel_Nodes_Or                 :binary
-      alias :visit_Arel_Nodes_Ordering           :binary
       alias :visit_Arel_Nodes_OuterJoin          :binary
       alias :visit_Arel_Nodes_TableAlias         :binary
       alias :visit_Arel_Nodes_Values             :binary
diff --git a/lib/arel/visitors/dot.rb b/lib/arel/visitors/dot.rb
index 92d05c7b67..8303279211 100644
--- a/lib/arel/visitors/dot.rb
+++ b/lib/arel/visitors/dot.rb
@@ -30,7 +30,6 @@ module Arel
       private
       def visit_Arel_Nodes_Ordering o
         visit_edge o, "expr"
-        visit_edge o, "direction"
       end
 
       def visit_Arel_Nodes_TableAlias o
diff --git a/lib/arel/visitors/mssql.rb b/lib/arel/visitors/mssql.rb
index ea7ab6394c..23dc06a936 100644
--- a/lib/arel/visitors/mssql.rb
+++ b/lib/arel/visitors/mssql.rb
@@ -3,21 +3,71 @@ module Arel
     class MSSQL < Arel::Visitors::ToSql
       private
 
-      def build_subselect key, o
-        stmt     = super
-        core     = stmt.cores.first
-        core.top = Nodes::Top.new(o.limit.expr) if o.limit
-        stmt
+      # `top` wouldn't really work here. I.e. User.select("distinct first_name").limit(10) would generate
+      # "select top 10 distinct first_name from users", which is invalid query! it should be
+      # "select distinct top 10 first_name from users"
+      def visit_Arel_Nodes_Top o
+        ""
       end
 
-      def visit_Arel_Nodes_Limit o
-        ""
+      def visit_Arel_Nodes_SelectStatement o
+        if !o.limit && !o.offset
+          return super o
+        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)
+          if select_count? x
+            x.projections = [row_num_literal(core_order_by)]
+            is_select_count = true
+          else
+            x.projections << row_num_literal(core_order_by)
+          end
+
+          visit_Arel_Nodes_SelectCore x
+        }.join
+
+        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
       end
 
-      def visit_Arel_Nodes_Top o
-        "TOP #{visit o.expr}"
+      def get_offset_limit_clause o
+        first_row = o.offset ? o.offset.expr.to_i + 1 : 1
+        last_row  = o.limit ? o.limit.expr.to_i - 1 + first_row : nil
+        if last_row
+          " _row_num BETWEEN #{first_row} AND #{last_row}"
+        else
+          " _row_num >= #{first_row}"
+        end
       end
 
+      def determine_order_by x
+        unless x.groups.empty?
+          "ORDER BY #{x.groups.map { |g| visit g }.join ', ' }"
+        else
+          "ORDER BY #{find_left_table_pk(x.froms)}"
+        end
+      end
+
+      def row_num_literal order_by
+        Nodes::SqlLiteral.new("ROW_NUMBER() OVER (#{order_by}) as _row_num")
+      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??
+      def find_left_table_pk o
+        return visit o.primary_key if o.instance_of? Arel::Table
+        find_left_table_pk o.left if o.kind_of? Arel::Nodes::Join
+      end
     end
   end
 end
diff --git a/lib/arel/visitors/to_sql.rb b/lib/arel/visitors/to_sql.rb
index 312f92ea2f..933edc15f2 100644
--- a/lib/arel/visitors/to_sql.rb
+++ b/lib/arel/visitors/to_sql.rb
@@ -203,8 +203,12 @@ key on UpdateManager using UpdateManager#key=
         "(#{visit o.expr})"
       end
 
-      def visit_Arel_Nodes_Ordering o
-        "#{visit o.expr} #{o.descending? ? 'DESC' : 'ASC'}"
+      def visit_Arel_Nodes_Ascending o
+        "#{visit o.expr} ASC"
+      end
+
+      def visit_Arel_Nodes_Descending o
+        "#{visit o.expr} DESC"
       end
 
       def visit_Arel_Nodes_Group o
@@ -416,7 +420,8 @@ key on UpdateManager using UpdateManager#key=
       end
 
       def quote_table_name name
-        @quoted_tables[name] ||= Arel::Nodes::SqlLiteral === name ? name : @connection.quote_table_name(name)
+        return name if Arel::Nodes::SqlLiteral === name
+        @quoted_tables[name] ||= @connection.quote_table_name(name)
       end
 
       def quote_column_name name
diff --git a/lib/arel/visitors/visitor.rb b/lib/arel/visitors/visitor.rb
index c9cdf34adb..8f9dd929e1 100644
--- a/lib/arel/visitors/visitor.rb
+++ b/lib/arel/visitors/visitor.rb
@@ -11,15 +11,19 @@ module Arel
         hash[klass] = "visit_#{(klass.name || '').gsub('::', '_')}"
       end
 
+      def dispatch
+        DISPATCH
+      end
+
       def visit object
-        send DISPATCH[object.class], object
+        send dispatch[object.class], object
       rescue NoMethodError => e
-        raise e if respond_to?(DISPATCH[object.class], true)
+        raise e if respond_to?(dispatch[object.class], true)
         superklass = object.class.ancestors.find { |klass|
-          respond_to?(DISPATCH[klass], true)
+          respond_to?(dispatch[klass], true)
         }
         raise(TypeError, "Cannot visit #{object.class}") unless superklass
-        DISPATCH[object.class] = DISPATCH[superklass]
+        dispatch[object.class] = dispatch[superklass]
         retry
       end
     end