Merge pull request #329 from veracross/fix-distinct-plus-limit

Fix for Issue #306 - DISTINCT + TOP + ORDER BY Associated Table

Author: annaswims

lib/active_record/connection_adapters/sqlserver/schema_statements.rb

@@ -44,17 +44,12 @@ def columns(table_name, _name = nil)
         end
 
         # like postgres, sqlserver requires the ORDER BY columns in the select list for distinct queries, and
-        # requires that the ORDER BY include the distinct column.
-        # this method is idental to the postgres method
+        # requires that the ORDER BY include the distinct column. Unfortunately, sqlserver does not support
+        # DISTINCT ON () like Posgres, or FIRST_VALUE() like Oracle (at least before SQL Server 2012). Because
+        # of these facts, we don't actually add any extra columns for distinct, but instead have to create
+        # a subquery with ROW_NUMBER() and DENSE_RANK() in our monkey-patches to Arel.
         def columns_for_distinct(columns, orders) #:nodoc:
-          order_columns = orders.map do |s|
-                            # Convert Arel node to string
-                            s = s.to_sql unless s.is_a?(String)
-                            # Remove any ASC/DESC modifiers
-                            s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
-                          end.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }
-
-          [super, *order_columns].join(', ')
+          columns
         end
 
         def rename_table(table_name, new_name)

lib/arel/visitors/sqlserver.rb

@@ -7,6 +7,8 @@ class SQLServer < Arel::Visitors::ToSql
       def visit_Arel_Nodes_SelectStatement(o, a)
         if complex_count_sql?(o)
           visit_Arel_Nodes_SelectStatementForComplexCount(o, a)
+        elsif distinct_non_present_orders?(o, a)
+          visit_Arel_Nodes_SelectStatementDistinctNonPresentOrders(o, a)
         elsif o.offset
           visit_Arel_Nodes_SelectStatementWithOffset(o, a)
         else
@@ -47,6 +49,76 @@ def visit_Arel_Nodes_Bin(o, a)
 
       # SQLServer ToSql/Visitor (Additions)
 
+      # This constructs a query using DENSE_RANK() and ROW_NUMBER() to allow
+      # ordering a DISTINCT set of data by columns in another table that are
+      # not part of what we actually want to be DISTINCT. Without this, it is
+      # possible for the DISTINCT qualifier combined with TOP to return fewer
+      # rows than were requested.
+      def visit_Arel_Nodes_SelectStatementDistinctNonPresentOrders(o, a)
+        core = o.cores.first
+        projections = core.projections
+        groups = core.groups
+        orders = o.orders.uniq
+
+        select_frags = projections.map do |x|
+          frag = projection_to_sql_remove_distinct(x, core, a)
+          # Remove the table specifier
+          frag.gsub!(/^[^\.]*\./, '')
+          # If there is an alias, remove everything but
+          frag.gsub(/^.*\sAS\s+/i, '')
+        end
+
+        if o.offset
+          select_frags << 'ROW_NUMBER() OVER (ORDER BY __order) AS __offset'
+        else
+          select_frags << '__order'
+        end
+
+        projection_list = projections.map { |x| projection_to_sql_remove_distinct(x, core, a) }.join(', ')
+
+        sql = [
+          ('SELECT'),
+          (visit(core.set_quantifier, a) if core.set_quantifier and !o.offset),
+          (visit(o.limit, a) if o.limit and !o.offset),
+          (select_frags.join(', ')),
+          ('FROM ('),
+            ('SELECT'),
+            (
+              [
+                (projection_list),
+                (', DENSE_RANK() OVER ('),
+                  ("ORDER BY #{orders.map { |x| visit(x, a) }.join(', ')}" if !orders.empty?),
+                (') AS __order'),
+                (', ROW_NUMBER() OVER ('),
+                  ("PARTITION BY #{projection_list}" if !orders.empty?),
+                  (" ORDER BY #{orders.map { |x| visit(x, a) }.join(', ')}" if !orders.empty?),
+                (') AS __joined_row_num')
+              ].join('')
+            ),
+            (source_with_lock_for_select_statement(o, a)),
+            ("WHERE #{core.wheres.map { |x| visit(x, a) }.join ' AND ' }" unless core.wheres.empty?),
+            ("GROUP BY #{groups.map { |x| visit(x, a) }.join ', ' }" unless groups.empty?),
+            (visit(core.having, a) if core.having),
+          (') AS __sq'),
+          ('WHERE __joined_row_num = 1'),
+          ('ORDER BY __order' unless o.offset)
+        ].compact.join(' ')
+
+        if o.offset
+          sql = [
+            ('SELECT'),
+            (visit(core.set_quantifier, a) if core.set_quantifier),
+            (visit(o.limit, a) if o.limit),
+            ('*'),
+            ('FROM (' + sql + ') AS __osq'),
+            ("WHERE __offset > #{visit(o.offset.expr, a)}"),
+            ('ORDER BY __offset')
+          ].join(' ')
+        end
+
+        sql
+      end
+
       def visit_Arel_Nodes_SelectStatementWithOutOffset(o, a, windowed = false)
         find_and_fix_uncorrelated_joins_in_select_statement(o)
         core = o.cores.first
@@ -123,6 +195,18 @@ def visit_Arel_Nodes_SelectStatementForComplexCount(o, a)
 
       # SQLServer Helpers
 
+      def projection_to_sql_remove_distinct(x, core, a)
+        frag = Arel.sql(visit(x, a))
+        # In Rails 4.0.0, DISTINCT was in a projection, whereas with 4.0.1
+        # it is now stored in the set_quantifier. This moves it to the correct
+        # place so the code works on both 4.0.0 and 4.0.1.
+        if frag =~ /^\s*DISTINCT\s+/i
+          core.set_quantifier = Arel::Nodes::Distinct.new()
+          frag.gsub!(/\s*DISTINCT\s+/, '')
+        end
+        frag
+      end
+
       def source_with_lock_for_select_statement(o, a)
         core = o.cores.first
         source = "FROM #{visit(core.source, a).strip}" if core.source
@@ -166,6 +250,68 @@ def single_distinct_select_statement?(o)
             p1.respond_to?(:include?) && p1.include?('DISTINCT'))
       end
 
+      # Determine if the SELECT statement is asking for DISTINCT results,
+      # but is using columns not part of the SELECT list in the ORDER BY.
+      # This is necessary because SQL Server requires all ORDER BY entries
+      # be in the SELECT list with DISTINCT. However, these ordering columns
+      # can cause duplicate rows, which affect when using a limit.
+      def distinct_non_present_orders?(o, a)
+        projections = o.cores.first.projections
+
+        sq = o.cores.first.set_quantifier
+        p1 = projections.first
+
+        found_distinct = sq and sq.class.to_s =~ /Distinct/
+        if (p1.respond_to?(:distinct) && p1.distinct) || (p1.respond_to?(:include?) && p1.include?('DISTINCT'))
+          found_distinct = true
+        end
+
+        return false if !found_distinct or o.orders.uniq.empty?
+
+        tables_all_columns = []
+        expressions = projections.map do |p|
+          visit(p, a).split(',').map do |x|
+            x.strip!
+            # Rails 4.0.0 included DISTINCT in the first projection
+            x.gsub!(/\s*DISTINCT\s+/, '')
+            # Aliased column names
+            x.gsub!(/\s+AS\s+\w+/i, '')
+            # Identifier quoting
+            x.gsub!(/\[|\]/, '')
+            star_match = /^(\w+)\.\*$/.match(x)
+            if star_match
+              tables_all_columns << star_match[1]
+            end
+            x.strip.downcase
+          end.join(', ')
+        end
+
+        # Make sure each order by is in the select list, otherwise there needs
+        # to be a subquery with row_numbe()
+        o.orders.uniq.each do |order|
+          order = visit(order, a)
+          order.strip!
+
+          order.gsub!(/\s+(asc|desc)/i, '')
+          # Identifier quoting
+          order.gsub!(/\[|\]/, '')
+
+          order.strip!
+          order.downcase!
+
+          # If we selected all columns from a table, the order is ok
+          table_match = /^(\w+)\.\w+$/.match(order)
+          next if table_match and tables_all_columns.include?(table_match[1])
+
+          next if expressions.include?(order)
+
+          return true
+        end
+
+        # We didn't find anything in the order by no being selected
+        false
+      end
+
       def windowed_single_distinct_select_statement?(o)
         o.limit &&
           o.offset &&

test/cases/finder_test_sqlserver.rb

@@ -10,34 +10,30 @@ class FinderTestSqlserver < ActiveRecord::TestCase
 end
 
 class FinderTest < ActiveRecord::TestCase
-  fixtures :authors, :author_addresses, :posts
+  fixtures :authors, :author_addresses, :posts, :categorizations
   COERCED_TESTS = [
     :test_exists_does_not_select_columns_without_alias,
     :test_string_sanitation,
-    :test_take_and_first_and_last_with_integer_should_use_sql_limit,
-    :test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct
-
+    :test_take_and_first_and_last_with_integer_should_use_sql_limit
   ]
 
   include SqlserverCoercedTest
 
 
-  # TODO This test passes in rails 4.0.0 but not 4.0.1-2
-  def test_coerced_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct
-   p =  Post.all.merge!(includes: { authors: :author_address },
-      order: 'author_addresses.id DESC ',
-      limit: 2)
-   # ar_version = Gem.loaded_specs['activerecord'].version.version
-   # arel_to_png(p, "#{ar_version}")
-    count = p.to_a.size
-    #puts "*****#{ActiveRecord::SQLCounter.log_all.join("\n\n")}"
-    assert_equal 2, count
-
-     assert_equal 3, Post.all.merge!(includes: { author: :author_address, authors: :author_address},
-                              order: 'author_addresses_authors.id DESC ', limit: 3).to_a.size
+  def test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct_and_offset
+    # Based on test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct
+    # and issue https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/issues/306
+    assert_equal(
+      posts(:welcome, :thinking),
+      Post.all.merge!(
+        :includes => { authors: :author_address },
+        :order => ['authors.name DESC'],
+        :limit => 2,
+        :offset => 1
+      ).to_a
+    )
   end
 
-
   def test_coerced_exists_does_not_select_columns_without_alias
     assert_sql(/SELECT TOP \(1\) 1 AS one FROM \[topics\]/i) do
       Topic.exists?