Oracle: fix limited id selection for eager loading. Closes #6515.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@5480 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

6 files changed, 58 insertions, 26 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 5564a750af..e4ada2a7ac 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -27,7 +27,7 @@
 
 * has_one :dependent => :nullify ignores nil associates.  #6528 [janovetz, Jeremy Kemper]
 
-* Oracle: resolve test failures, use prefetched primary key for inserts, check for null defaults. Factor out some common methods from all adapters.  #6515 [Michael Schoen]
+* Oracle: resolve test failures, use prefetched primary key for inserts, check for null defaults, fix limited id selection for eager loading. Factor out some common methods from all adapters.  #6515 [Michael Schoen]
 
 * Make add_column use the options hash with the Sqlite Adapter. Closes #6464 [obrie]
 
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 5921492433..3a9ddc3f8d 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1190,25 +1190,23 @@ module ActiveRecord
             "#{name} Load IDs For Limited Eager Loading"
           ).collect { |row| connection.quote(row[primary_key]) }.join(", ")
         end
- 
+
         def construct_finder_sql_for_association_limiting(options, join_dependency)
           scope       = scope(:find)
           is_distinct = include_eager_conditions?(options) || include_eager_order?(options)
           sql = "SELECT "
           if is_distinct
-            ordered_columns = options[:order].to_s.split(',').collect! { |s| s.split.first }
-            options[:order] = "#{table_name}.#{primary_key}, #{options[:order]}" if options[:order] && connection.requires_order_columns_in_distinct_clause?
-            sql << connection.distinct("#{table_name}.#{primary_key}", ordered_columns)
+            sql << connection.distinct("#{table_name}.#{primary_key}", options[:order])
           else
             sql << primary_key
           end
           sql << " FROM #{table_name} "
-          
+
           if is_distinct
             sql << join_dependency.join_associations.collect(&:association_join).join
             add_joins!(sql, options, scope)
           end
-          
+
           add_conditions!(sql, options[:conditions], scope)
           sql << "ORDER BY #{options[:order]} " if options[:order]
           add_limit!(sql, options, scope)
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index a098c02829..599d04e593 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -280,8 +280,11 @@ module ActiveRecord
         sql << " NOT NULL" if options[:null] == false
       end
 
-      # SELECT DISTINCT clause for a given set of columns.  PostgreSQL overrides this for custom DISTINCT syntax.
-      def distinct(columns, *order_columns)
+      # SELECT DISTINCT clause for a given set of columns and a given ORDER BY clause.
+      # Both PostgreSQL and Oracle overrides this for custom DISTINCT syntax.
+      #
+      #   distinct("posts.id", "posts.created_at desc")
+      def distinct(columns, order_by)
         "DISTINCT #{columns}"
       end
     end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index b753c248c5..949b8f7951 100755
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -56,12 +56,6 @@ module ActiveRecord
         false
       end
 
-      # Does this adapter require the order columns to be in the select clause
-      # of a DISTINCT query?  This is +false+ in all adapters except postgresql.
-      def requires_order_columns_in_distinct_clause?
-        false
-      end
-
       def reset_runtime #:nodoc:
         rt, @runtime = @runtime, 0
         rt
diff --git a/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb b/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb
index bd67ea7d75..b8d3ed5a95 100644
--- a/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/oracle_adapter.rb
@@ -314,7 +314,7 @@ begin
 
         def columns(table_name, name = nil) #:nodoc:
           (owner, table_name) = @connection.describe(table_name)
-          raise "Couldn't describe #{table_name}. Does it exist?" unless owner and table_name
+          raise "Could not describe #{table_name}. Does it exist?" unless owner and table_name
 
           table_cols = %Q{
             select column_name as name, data_type as sql_type, data_default, nullable,
@@ -427,6 +427,34 @@ begin
           end
         end
 
+        # SELECT DISTINCT clause for a given set of columns and a given ORDER BY clause.
+        #
+        # Oracle requires the ORDER BY columns to be in the SELECT list for DISTINCT
+        # queries. However, with those columns included in the SELECT DISTINCT list, you
+        # won't actually get a distinct list of the column you want (presuming the column
+        # has duplicates with multiple values for the ordered-by columns. So we use the 
+        # FIRST_VALUE function to get a single (first) value for each column, effectively
+        # making every row the same.
+        #
+        #   distinct("posts.id", "posts.created_at desc")
+        def distinct(columns, order_by)
+          return "DISTINCT #{columns}" if order_by.blank?
+
+          # construct a clean list of column names from the ORDER BY clause, removing
+          # any asc/desc modifiers
+          order_columns = order_by.split(',').collect! { |s| s.split.first }
+          order_columns.delete_if &:blank?
+
+          # simplify the ORDER BY to just use positional syntax, to avoid the complexity of
+          # having to create valid column aliases for the FIRST_VALUE columns
+          order_by.replace(((offset=columns.count(',')+2) .. offset+order_by.count(',')).to_a * ", ")
+
+          # construct a valid DISTINCT clause, ie. one that includes the ORDER BY columns, using
+          # FIRST_VALUE such that the inclusion of these columns doesn't invalidate the DISTINCT
+          order_columns.map! { |c| "FIRST_VALUE(#{c}) OVER (PARTITION BY #{columns} ORDER BY #{c})" }
+          sql = "DISTINCT #{columns}, "
+          sql << order_columns * ", "
+        end
 
         private
 
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index e16af71762..c0efd1a341 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -111,10 +111,6 @@ module ActiveRecord
         63
       end
 
-      def requires_order_columns_in_distinct_clause?
-        true
-      end
-
       # QUOTING ==================================================
 
       def quote(value, column = nil)
@@ -376,14 +372,27 @@ module ActiveRecord
         end
       end
       
-      # PostgreSQL requires the ORDER BY columns in the select list for distinct queries.
-      # If you select distinct by a column though, you must pass that column in the order by clause too:
+      # SELECT DISTINCT clause for a given set of columns and a given ORDER BY clause.
+      #
+      # PostgreSQL requires the ORDER BY columns in the select list for distinct queries, and
+      # requires that the ORDER BY include the distinct column.
       #
-      #   distinct("posts.id", 'posts.id', 'posts.created_at')
-      def distinct(columns, *order_columns)
+      #   distinct("posts.id", "posts.created_at desc")
+      def distinct(columns, order_by)
+        return "DISTINCT #{columns}" if order_by.blank?
+
+        # construct a clean list of column names from the ORDER BY clause, removing
+        # any asc/desc modifiers
+        order_columns = order_by.split(',').collect! { |s| s.split.first }
         order_columns.delete_if &:blank?
-        sql = "DISTINCT ON (#{columns}) #{columns}"
-        sql << (order_columns.any? ? ", #{order_columns * ', '}" : '')
+
+        # add the DISTINCT columns to the start of the ORDER BY clause
+        order_by.replace "#{columns}, #{order_by}"
+
+        # return a DISTINCT ON() clause that's distinct on the columns we want but includes
+        # all the required columns for the ORDER BY to work properly
+        sql = "DISTINCT ON (#{columns}) #{columns}, "
+        sql << order_columns * ', '
       end
 
       private