[PATCH] Only Join Required Tables in Limited Eager Load Pre-Query

Looks good to me, however to nitpick a little:

The variable naming is a little off compared to what we normally do? full names rather than o_tables etc. and perhaps some comments outlining what's going on and why as it seems a little tangential.

New version of the patch tested against edge rails r8971

Thanks for the attention Koz, you always seem to pay attention when no one else does. I've refactored the patch with clear names and a comment. Additionally there was a new dependency on the include_eager_order? and include_eager_conditions? method signature that was introduced since I first created the patch. I've fixed that as well.

mysql test suite is passing for me. my application tests pass as well.

My new patch is against trunk. I haven't bothered applying to stable since performance enhancements don't go in stable, right?

(In [8977]) Improve performance on :include/:conditions/:limit queries by selectively joining in the pre-query. Closes #9560 [dasil003]

First-time commenter here, so I'm sorry if this is the wrong place to post...

I'm finding that this patch is breaking my queries in the following situation:

I have a model Play
 has_many :contributions

model Contribution
 belongs_to :play
 belongs_to :artist

If I now select

Play.find(:all, 
          {:include=>{:contributions=>:artist},
           :order=>" artists.last_name asc, artists.first_name asc "})

the query will error out because contributions is not included in the join. The new code filters out all joins listed directly in the "order by" clause, but not the intermediary joins needed to access those tables. The "artists" join is included, but the "contributions" join is not.

In other words I get: missing FROM-clause entry in subquery for table "contributions" P263 Fparse_relation.c L2008 RwarnAutoRange: SELECT * FROM (SELECT DISTINCT ON ("plays".id) "plays".id, artists.last_name AS alias_0, artists.first_name AS alias_1, plays.written_date AS alias_2, plays.written_mask AS alias_3, plays.name AS alias_4 FROM "plays" LEFT OUTER JOIN "artists" ON "artists".id = "contributions".artist_id ) AS id_list ORDER BY id_list.alias_0 , id_list.alias_1 , id_list.alias_2 , id_list.alias_3 , id_list.alias_4 LIMIT 20 OFFSET 0

I know I should include a patch, but I haven't quite parsed all the code yet. If I come up with one I will.

The following changes should fix the problem outlined above:

          def join_for_table_name(table_name)
            @joins.select{|j|j.table_name == table_name}.first rescue nil
          end
          
          def joins_for_table_name(table_name)
            join = join_for_table_name(table_name)            
            result = nil            
            if join && join.is_a?(JoinAssociation)
              result = [join]
              if join.parent && join.parent.is_a?(JoinAssociation)
                result = joins_for_table_name(join.parent.aliased_table_name) + result
              end
            end
            result
          end

Sorry, hit submit when I meant to hit preview.

        def construct_finder_sql_for_association_limiting(options, join_dependency)

          ...

          all_tables             = tables_from_conditions + tables_from_order
+          distinct_join_associations = all_tables.uniq.map{|table| 
+            join_dependency.joins_for_table_name(table)
+          }.flatten.compact.uniq

          ...

          if is_distinct
+            sql << distinct_join_associations.collect(&:association_join).join
-            sql << join_dependency.join_associations.reject{ |ja| !all_tables.include?(ja.table_name) }.collect(&:association_join).join
            add_joins!(sql, options, scope)
          end

        ...

        class JoinDependency
          ....

+          def join_for_table_name(table_name)
+            @joins.select{|j|j.table_name == table_name}.first rescue nil
+          end
          
+          def joins_for_table_name(table_name)
+            join = join_for_table_name(table_name)            
+            result = nil            
+            if join && join.is_a?(JoinAssociation)
+              result = [join]
+              if join.parent && join.parent.is_a?(JoinAssociation)
+                result = joins_for_table_name(join.parent.aliased_table_name) +             
+                         result
+              end
+            end
+            result
+          end

+1 for jdevine's patch.

Really important actually since this causes regressions for order + limit queries.

Could you provide this patch as a git patch. There are some instructions here:

http://www.tpope.net/rails-git-best-practices

I held off on submitting as a patch because I'm not sure the code I posted addresses table aliasing correctly... I've been using it with no problem in development so far, but I haven't taken the time to think it through or test it with multiple associations referencing the same table, etc.

If there's someone very familiar with the table-aliasing code who could give it a glance, that'd be great.

This certainly looks plausible and a move in the right direction (and you have my sympathy for having dived into JoinDependency & friends :-) ). I do share your concern about the murkiness of some of the table aliasing stuff, so it would be great to have some tests to go with this. If you really want to make sure the table aliasing stuff is doing the right thing, then perhaps some tests where the same table is joined more than once.

I wonder if perhaps joins_for_table_name(join.parent.aliased_table_name) should be joins_for_table_name(join.parent.table_name) ? joins_for_table_name iterates over @joins looking for one whose table_name matches the argument supplied , so if you give it aliased_table_name and aliased_table_name differs from table_name (ie the non trivial case) then I would expect joins_for_table_name to return nothing.

Added patch as suggested. Changed pasted code to handle table aliasing correctly & handled cases where the table name is quoted in the order or conditions clause.

I also came across another similar situation today when using includes with a select statement, but I'll open another ticket for that one. Patch is simple.

Sorry to be a pain but could you make a lighthouse ticket for this? That'll let us mark it as a 2.1 blocker

Submitted as ticket #109 on lighthouse.