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. # - # distinct("posts.id", 'posts.id', 'posts.created_at') - def distinct(columns, *order_columns) + # 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.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