I just got bit by a pretty subtle bug with eager loading. Basically what happened is mixing a :group and :include clause. The group clause of course collapsed the associations so I only got one of each association (this was not immediately obvious due to the data I''m working with). What makes it so subtle is that if the Preload strategy is used then there is no problem. In my case the condition it was even less obvious because the condition that was triggering the Join strategy was not applicable anyway. I realize that parsing the conditions and cross-referencing to other options in order to determine the true applicability of the Join is a rabbit hole no one wants to go down. But I was thinking about the other side. If there is a :group clause in a query, my gut instinct is that a large majority of the time it is going to clobber associations. Given the fact that it is pretty easy to add explicit joins to satisfy conditions, but pretty ugly to force the preload strategy, would there be any support for either: A) setting up :group clause to force the Preload strategy B) raising an exception or warning if :group is used with :include of a non-singular association C) giving access to the Preload strategy explicitly I''m finding a proliferation of Klass.send(:preload_associations, foo, :bar) throughout my codebase and it doesn''t smell good. The more I think about, the more C seems like it might be the only reasonable option. Thoughts?
> C) giving access to the Preload strategy explicitly > > I''m finding a proliferation of Klass.send(:preload_associations, > foo, :bar) throughout my codebase and it doesn''t smell good. The more > I think about, the more C seems like it might be the only reasonable > option. Thoughts?I''ve also found myself doing that :preload_associations trick from time to time and I''m similarly thinking that that shows two things: 1) Maybe we should make a nicer way for people to do this (fred cheung was playing with an array proxy to enable this kind of thing) 2) We might want to make :strategy=>:preload an option to find. I''ve no idea if :strategy is a good name for the option, but I think it''s probably a good idea to investigate whether the option is feasible and if it would be useful. Care to take a crack at it? -- Cheers Koz
Yeah I might give it a shot. I''m a little hammered at work right now, but the code in question has been pretty stable for a while and I think it''ll be reasonably easy to implement :strategy option. I''m also happy to collaborate on Fred''s work if he''s interested... I have some use for some detached type of preloading. One thought I had about :strategy => :preload is that compared to the current hacky approach it''s a bit less flexible, because it doesn''t let you mix and match. Given that drawback, I wonder if its even worth complicating the API. Maybe officially making :preload_associations public and documenting it would be worth it? On Jun 15, 1:53 am, Michael Koziarski <mich...@koziarski.com> wrote:> > C) giving access to the Preload strategy explicitly > > > I''m finding a proliferation of Klass.send(:preload_associations, > > foo, :bar) throughout my codebase and it doesn''t smell good. The more > > I think about, the more C seems like it might be the only reasonable > > option. Thoughts? > > I''ve also found myself doing that :preload_associations trick from > time to time and I''m similarly thinking that that shows two things: > > 1) Maybe we should make a nicer way for people to do this (fred cheung > was playing with an array proxy to enable this kind of thing) > 2) We might want to make :strategy=>:preload an option to find. > > I''ve no idea if :strategy is a good name for the option, but I think > it''s probably a good idea to investigate whether the option is > feasible and if it would be useful. > > Care to take a crack at it? > > -- > Cheers > > Koz
On Jun 15, 2009, at 6:34 PM, dasil003 wrote:> One thought I had about :strategy => :preload is that compared to the > current hacky approach it''s a bit less flexible, because it doesn''t > let you mix and match. Given that drawback, I wonder if its even > worth complicating the API. Maybe officially > making :preload_associations public and documenting it would be worth > it?I was thinking the same thing. I find preload_associations to be a useful tool to work with directly. +1 to making it (or something similar) public and documented.
Ideally I''d like to fix how :include works. I see :include as a pure optimization step. Lack or presence of it should only make things slower/faster and in no way change the way things work. My ideal fix would be : - Make :include *always* use preload strategy unless the required data set is explicitly loaded - Introduce :left_joins and :right_joins keys to the finder. Title says it all - Make user explicitly specify the required joins if they want to put conditions on the associations - For all the associations specified in :include, check if the required dataset has already been loaded by :*joins. If not, preload. Thoughts ? On Mon, Jun 15, 2009 at 11:34 PM, dasil003 <gabriel.d@gmail.com> wrote:> > Yeah I might give it a shot. I''m a little hammered at work right now, > but the code in question has been pretty stable for a while and I > think it''ll be reasonably easy to implement :strategy option. I''m > also happy to collaborate on Fred''s work if he''s interested... I have > some use for some detached type of preloading. > > One thought I had about :strategy => :preload is that compared to the > current hacky approach it''s a bit less flexible, because it doesn''t > let you mix and match. Given that drawback, I wonder if its even > worth complicating the API. Maybe officially > making :preload_associations public and documenting it would be worth > it? > > > > On Jun 15, 1:53 am, Michael Koziarski <mich...@koziarski.com> wrote: > > > C) giving access to the Preload strategy explicitly > > > > > I''m finding a proliferation of Klass.send(:preload_associations, > > > foo, :bar) throughout my codebase and it doesn''t smell good. The more > > > I think about, the more C seems like it might be the only reasonable > > > option. Thoughts? > > > > I''ve also found myself doing that :preload_associations trick from > > time to time and I''m similarly thinking that that shows two things: > > > > 1) Maybe we should make a nicer way for people to do this (fred cheung > > was playing with an array proxy to enable this kind of thing) > > 2) We might want to make :strategy=>:preload an option to find. > > > > I''ve no idea if :strategy is a good name for the option, but I think > > it''s probably a good idea to investigate whether the option is > > feasible and if it would be useful. > > > > Care to take a crack at it? > > > > -- > > Cheers > > > > Koz > > >-- Cheers! - Pratik http://m.onkey.org --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
> - Make :include *always* use preload strategy unless the required data set > is explicitly loaded > - Introduce :left_joins and :right_joins keys to the finder. Title says it > all > - Make user explicitly specify the required joins if they want to put > conditions on the associations > - For all the associations specified in :include, check if the required > dataset has already been loaded by :*joins. If not, preload. > Thoughts ?Sounds good to me, but I also like the idea of being able to do something like @orders = Order.find_by_sql(CRAZY_SHIT) @orders.preload(:line_items=>{:sku=>:title}) -- Cheers Koz
I like this approach. :include should definitely preload, and people should
have to use :joins and :left_joins when they need them all in the same
query.
What would :right_joins do though? You need a left record to load
associations of, otherwise how would you get to the right objects?
Regarding making people explicitly specify the required joins to put
conditions on the associations, if I understand you correctly, I think this
would tend to mean that you end up with numerous copies of your basic
foreign key-based join conditions throughout the codebase, which is not very
DRY.
IMHO when people want to load associations with extra conditions, what
they''re really saying is that they want a subset - for which they
should
ideally be adding a named_scope on the associated model and somehow telling
Rails to load that scope only.
For example, instead of:
Lunch.find(:all, :include => :pies, :conditions => {:pies => {:steak
=>
true}})
as it is currently, or
Lunch.find(:all, :joins => ["LEFT JOIN pies ON pie.lunch_id
lunches.idAND steak = ?", true])
which is my interpretation of what you''re saying below (please correct
me if
not), I would like some way to say
Lunch.find(:all, :include => :"pies.steak")
where Pie defines
named_scope :steak, :conditions => {:steak => true}.
However, it''s hard to think of a good syntax that shows what scope we
want
to be loaded, and in any case I don''t think we have loaded-ness for
scopes
yet as we do for associations, so there''d be a number of things that
would
need to be changed to make that kind of scheme work - it doesn''t sound
like
an easy approach.
So currently we''re stuck with defining extra associations
(Lunch.has_many
:steak_pies as well as the original has_many :pies). I''d love a way to
get
around that, other suggestions welcome!
Will
On Tue, Jun 16, 2009 at 9:57 PM, Pratik <pratiknaik@gmail.com> wrote:
> Ideally I''d like to fix how :include works. I see :include as a
pure
> optimization step. Lack or presence of it should only make things
> slower/faster and in no way change the way things work. My ideal fix would
> be :
>
> - Make :include *always* use preload strategy unless the required data set
> is explicitly loaded
> - Introduce :left_joins and :right_joins keys to the finder. Title says it
> all
> - Make user explicitly specify the required joins if they want to put
> conditions on the associations
> - For all the associations specified in :include, check if the required
> dataset has already been loaded by :*joins. If not, preload.
>
> Thoughts ?
>
> On Mon, Jun 15, 2009 at 11:34 PM, dasil003 <gabriel.d@gmail.com>
wrote:
>
>>
>> Yeah I might give it a shot. I''m a little hammered at work
right now,
>> but the code in question has been pretty stable for a while and I
>> think it''ll be reasonably easy to implement :strategy option.
I''m
>> also happy to collaborate on Fred''s work if he''s
interested... I have
>> some use for some detached type of preloading.
>>
>> One thought I had about :strategy => :preload is that compared to
the
>> current hacky approach it''s a bit less flexible, because it
doesn''t
>> let you mix and match. Given that drawback, I wonder if its even
>> worth complicating the API. Maybe officially
>> making :preload_associations public and documenting it would be worth
>> it?
>>
>>
>>
>> On Jun 15, 1:53 am, Michael Koziarski <mich...@koziarski.com>
wrote:
>> > > C) giving access to the Preload strategy explicitly
>> >
>> > > I''m finding a proliferation of
Klass.send(:preload_associations,
>> > > foo, :bar) throughout my codebase and it doesn''t
smell good. The more
>> > > I think about, the more C seems like it might be the only
reasonable
>> > > option. Thoughts?
>> >
>> > I''ve also found myself doing that :preload_associations
trick from
>> > time to time and I''m similarly thinking that that shows
two things:
>> >
>> > 1) Maybe we should make a nicer way for people to do this (fred
cheung
>> > was playing with an array proxy to enable this kind of thing)
>> > 2) We might want to make :strategy=>:preload an option to
find.
>> >
>> > I''ve no idea if :strategy is a good name for the option,
but I think
>> > it''s probably a good idea to investigate whether the
option is
>> > feasible and if it would be useful.
>> >
>> > Care to take a crack at it?
>> >
>> > --
>> > Cheers
>> >
>> > Koz
>>
>>
>
>
> --
> Cheers!
> - Pratik
> http://m.onkey.org
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
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 Tue, Jun 16, 2009 at 11:53 PM, Will Bryant <will.bryant@gmail.com> wrote:> I like this approach. :include should definitely preload, and people > should have to use :joins and :left_joins when they need them all in the > same query. > What would :right_joins do though? You need a left record to load > associations of, otherwise how would you get to the right objects? >Right join is just one of the gazzilion JOINs a typical RDBMS supports. But I guess we could live without it.> > Regarding making people explicitly specify the required joins to put > conditions on the associations, if I understand you correctly, I think this > would tend to mean that you end up with numerous copies of your basic > foreign key-based join conditions throughout the codebase, which is not very > DRY. >Not really. You can already specify associations in :joins arguments. So your example would be : Lunch.find(:all, :left_joins => :pies, :conditions => {:pies => {:steak => true}}) Or if you also want to instantiate pies : Lunch.find(:all, :left_joins => :pies, :conditions => {:pies => {:steak => true}}, :include => :pies) In this specific case, you could just use :joins too - which uses INNER JOIN. As for your scope example, I think that''s a different problem altogether. Thanks! <snip /> -- Cheers! - Pratik http://m.onkey.org --~--~---------~--~----~------------~-------~--~----~ 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 Wed, Jun 17, 2009 at 11:31 AM, Pratik <pratiknaik@gmail.com> wrote:> Not really. You can already specify associations in :joins arguments. So > your example would be : >> Lunch.find(:all, :left_joins => :pies, :conditions => {:pies => {:steak > => true}}) >Ah, when you said "Make user explicitly specify the required joins if they want to put conditions on the associations" I thought you were suggesting getting rid of that - I interpreted that as saying that you would have to write out the join conditions. No worries then, I like what you''re suggesting. But just to clarify, if you write the above statement - a left join, plus conditions on the joined table, will those conditions work as they currently do? I ask because there''s a big difference between adding terms into the left join conditions and adding terms into the overall statement conditions - the former would exclude the right rows (pies) if the conditions aren''t true, whereas the latter excludes the left rows (lunches) if the conditions aren''t true also. We only have a way to do the latter at the moment; would it be worth adding a way to do the former? Lunch.find(:all, :left_joins => :pies, :conditions => {:pies => {:steak => true}}) Would load only lunches that have steak pies currently - meaning that statement will give just the same results as Lunch.find(:all, :joins => :pies, :conditions => {:pies => {:steak => true}}) Which somewhat defeats the purpose of using a left join - it would be nice to have a way to load all lunches plus any steak pies that they have, since that''s presumably why you''re using a left join in the first place. (I rate this only a "nice to have" with left joins because admittedly, you can do this by writing the conditions out as a string instead, and use "OR pies.id IS NULL". But I think that beginners wouldn''t expect to have to do this every time.) --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
>> - Make :include *always* use preload strategy unless the required data set >> is explicitly loaded >>With that you mean that it''s not going to interpret the select, order and condition strings anymore to determine if they''re referencing some table? I''d be all for that! The interpretation of those string currently uses a too simple heuristic and is therefor just not working in practice.>> - Introduce :left_joins and :right_joins keys to the finder. Title says it >> all >>+1. Though I prefer the terms :left_outer_joins and :right_outer_joins. For completeness sake also introduce :full_outer_joins.>> - Make user explicitly specify the required joins if they want to put >> conditions on the associations >> - For all the associations specified in :include, check if the required >> dataset has already been loaded by :*joins. If not, preload. >>I guess this means that the :joins method will no longer accept a string? (or if they will, then a string value will be ignored to determine if preloading must occur or not)> Sounds good to me, but I also like the idea of being able to do something like > > @orders = Order.find_by_sql(CRAZY_SHIT) > @orders.preload(:line_items=>{:sku=>:title}) >+1 on a public preload method as well. Cheers, Lawrence
On Wed, Jun 17, 2009 at 3:04 AM, Lawrence Pit <lawrence.pit@gmail.com>wrote:> > > >> - Make :include *always* use preload strategy unless the required data > set > >> is explicitly loaded > >> > > With that you mean that it''s not going to interpret the select, order > and condition strings anymore to determine if they''re referencing some > table? I''d be all for that! The interpretation of those string > currently uses a too simple heuristic and is therefor just not working > in practice.Exactly.> > > >> - Introduce :left_joins and :right_joins keys to the finder. Title says > it > >> all > >> > > +1. Though I prefer the terms :left_outer_joins and :right_outer_joins. > For completeness sake also introduce :full_outer_joins. > > >> - Make user explicitly specify the required joins if they want to put > >> conditions on the associations > >> - For all the associations specified in :include, check if the required > >> dataset has already been loaded by :*joins. If not, preload. > >> > > I guess this means that the :joins method will no longer accept a > string? (or if they will, then a string value will be ignored to > determine if preloading must occur or not) >Well, we''ll definitely need a way to supply string joins, even if they''re gonna be ignored to determine preloading. So the API will need some more brainstorming. Possibly rename :joins to :inner_joins and make the new :joins only accept strings. -- Cheers! - Pratik http://m.onkey.org --~--~---------~--~----~------------~-------~--~----~ 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 16 Jun 2009, at 11:00, Michael Koziarski wrote:> >> - Make :include *always* use preload strategy unless the required >> data set >> is explicitly loaded >> - Introduce :left_joins and :right_joins keys to the finder. Title >> says it >> all >> - Make user explicitly specify the required joins if they want to put >> conditions on the associations >> - For all the associations specified in :include, check if the >> required >> dataset has already been loaded by :*joins. If not, preload. >> Thoughts ? > > Sounds good to me, but I also like the idea of being able to do > something like > > @orders = Order.find_by_sql(CRAZY_SHIT) > @orders.preload(:line_items=>{:sku=>:title})I''d love that too (and the result set proxy I fiddled with did get you that. Don''t know if it still works with current versions of rails). Also handy when the bit fetching your orders has no idea that you''re going to be displaying the orders along with various associations (although often scopes and what not can mitigate that). There is one particular case where preload is broken for :include, when you have a hmt with conditions on the join model (I''ve been meaning to do this for a little while but not gotten around to it. Last time I thought about it it needed a slight refactor of HMT so that preload doesn''t have to duplicate the code that generates the join statements you need) Fred> > -- > Cheers > > Koz > > >