ActiveRecord::Base finders select merging

I've attached a patch without tests as an example fix that only covers the basic case of chaining select calls (i.e. Topic.select(:id).select(:name)). There are a few issues surrounding it that may require discussion before implementing. Once I get some feedback I'll submit a full patch with tests.

The first issue is DISTINCTs. I think these really fall outside of the scope of this method (select) and should probably be handled with a new method or possibly two to properly handle DISTINCT ON. Adding a distinct method would just need an instance variable to capture the set state when its called so that the DISTINCT keyword can be prepended to the first @select_values element when build_arel gets called. DISTINCT ON is more complex, as some (all?) SQL implementations require the leftmost ORDER BY expressions to match the DISTINCT ON expressions.

The second issue is merge/&. Currently, the instance variables backing all multi-value methods except @join_values and @where_values are overwritten during a merge. I don't see why @select_values, @group_values and @order_values can't simply be appended, and @having_values handled like @where_values.

The third issue (non-issue) is handling this properly when raw SQL fragments are used. I'd say not handling it is the appropriate action to take here, but I thought I'd toss that out for consideration.

Having issues attaching the patch. It's short, so here it is:

.../lib/active_record/relation/query_methods.rb | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 7bca12d..89edd1e 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -175,10 +175,11 @@ module ActiveRecord

   quoted_table_name = @klass.quoted_table_name

   if selects.present?

•  selects.each do |s|
  

•  selects.reject! do |s|
     @implicit_readonly = false
  

•    arel = arel.project(s) if s.present?
  

•    !s.present?
   end
  

•  arel = arel.project(selects)
  

  else

   arel = arel.project(quoted_table_name + '.*')
  

  end

 .../lib/active_record/relation/query_methods.rb    |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 7bca12d..89edd1e 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -175,10 +175,11 @@ module ActiveRecord
       quoted_table_name = @klass.quoted_table_name
 
       if selects.present?
-        selects.each do |s|
+        selects.reject! do |s|
           @implicit_readonly = false
-          arel = arel.project(s) if s.present?
+          !s.present?
         end
+        arel = arel.project(selects)
       else
         arel = arel.project(quoted_table_name + '.*')
       end