Rails core - Sep 2007 - 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.

-- 
Gabe da Silveira
http://darwinweb.net

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 26 Sep 2007, at 00:50, Gabe da Silveira wrote:
>
> 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




--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 9/25/07, Frederick Cheung <frederick.cheung@gmail.com> wrote:

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.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---