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

4 files changed, 53 insertions, 19 deletions

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