Patch Review for selective joining of eager-loaded tables in pre-query

For those of you following the earlier saga of the performance of limited eager loading, I’ve decided to give up on pursuing my original patch. I received some support on the simplification argument (see my blog if you don’t know what I’m talking about), but it doesn’t seem like it’s worth breaking existing behavior.

So I’m moving onto promoting http://dev.rubyonrails.org/ticket/9497 which gathers up the list of tables that are referenced in conditions and joins and uses them to restrict what is actually joined. Although the diff looks a little confusing due a slight refactoring of the methods, in actuality there is less than one line of new code here.

I haven’t added a test, because:

a) this patch’s correctness is largely defined by the fact that it doesn’t break other tests which have pretty wide coverage over this area of ActiveRecord.

b) I didn’t see offhand a great way to verify the removal of the joins.

Anyway, if anyone can do a quick review and hopefully garner some +1s I’d love to get this in before 2.0.

So I'm moving onto promoting http://dev.rubyonrails.org/ticket/9497 which gathers up the list of tables that are referenced in conditions and joins and uses them to restrict what is actually joined. Although the diff looks a little confusing due a slight refactoring of the methods, in actuality there is less than one line of new code here.

Do you mean http://dev.rubyonrails.org/ticket/9560 ?

a) this patch's correctness is largely defined by the fact that it doesn't break other tests which have pretty wide coverage over this area of ActiveRecord. b) I didn't see offhand a great way to verify the removal of the joins.

I may be reading incorrectly, but is it the case that if you were to have class A    has_one :b end class B    has_one :c end and you did

A.find(:all, :include => {:b => :c}, :conditions => "cs.foo = bar")

then this would try and only join c (even though you need to go through b to get to c) ? No clue whether this is useful or not ( I could even be doing it myself)

Fred

Do you mean http://dev.rubyonrails.org/ticket/9560 ?

Uh, yeah, my bad.

I may be reading incorrectly, but is it the case that if you were to have class A has_one :b end class B has_one :c end and you did

A.find(:all, :include => {:b => :c}, :conditions => " cs.foo = bar")

then this would try and only join c (even though you need to go through b to get to c) ? No clue whether this is useful or not ( I could even be doing it myself)

Good question. There should be a test for that in any case. I don’t have time right now, but I’ll check this out in the next couple days and then repost the review request here.