Patch Review: #576 adds a :load option to ActiveRecord

I like the preloading functionality in ActiveRecord. Can we expose an API option for it? I know it's important to be careful with adding AR options, but I believe that adding a :load option would complement the existing :include and :joins options as follows:

* :include names associations that should be eager loaded but may also be needed for sql references (e.g. in conditions, orders, etc.) * :joins names associations that are needed for sql references but should not be loaded * :load names associations that should be loaded but are not needed for sql references

Comments and discussion appreciated! When I talked with Pratik in IRC, his concern was adding another explicit option when perhaps more intelligence could instead be applied to existing options. This is a case where it seems cleaner to add another option. Thoughts?

-Lance

I'd quite like to be able to preload associations after the records have been loaded (which is of course exactly what happens under the hood anyway).

You get back some objects from something that has no idea what you're going to do, but you know that in this case you're going to be iterating over them and displaying the value of association X - it would be neat to be able to say - I'm going to be looking at X - fetch those objects (of course datamapper style automatic doing this could potentially be even better).

I think this would avoid the adding of an extra option to the find method and actually buy you some flexability since it divorces the notions of loading the records and preloading an association(s)

Fred

You can actually call the preload_associations method whenever you want. It's not public, though, so you need to use send() -- perhaps that should change?

You can actually call the preload_associations method whenever you want.

I know, I wrote it :slight_smile:

It's not public, though, so you need to use send() -- perhaps that should change?

Something along those lines maybe some tidying up if it was to be for
public consumption. on association proxies you could do something like
post.comments.load(:author => :avatar). but for a Foo.find we can't do
that yet since we don't have a proxy in that case.

Fred

Something along those lines maybe some tidying up if it was to be for public consumption. on association proxies you could do something like post.comments.load(:author => :avatar). but for a Foo.find we can't do that yet since we don't have a proxy in that case.

Something like this could be very nice, though the ideal would be if AR figured out what you wanted and preloaded lazily. Though obviously this requires the notion of a result set and all the associated classes. If someone wants to work on this it could be a really nice enhancement.

> You can actually call the preload_associations method whenever you > want.

I know, I wrote it :slight_smile:

Hey thanks! Love it. :slight_smile:

> It's not public, though, so you need to use send() -- perhaps > that should change?

Something along those lines maybe some tidying up if it was to be for
public consumption. on association proxies you could do something like
post.comments.load(:author => :avatar). but for a Foo.find we can't do
that yet since we don't have a proxy in that case.

I do like that notation. In my case I've got quite a few associations that I'd like to have preloaded, so being able to pass arrays/hashes to preload_associations (well, indirectly through the :load option) make it much simpler.

Another benefit of a :load option for find is the ability to merge scopes. I actually like to set up named_scopes in the format "for_#{view-name}"; these named_scopes tack on all the association loading necessary for that view to render without n+1 problems (or 3n +1 even). With :load, I can make that happen without worrying about side effects on the result set itself.

Could we get away with find :all returning a proxy that looked enough like an array that it wouldn't break everyone's apps ?

Fred

Could we get away with find :all returning a proxy that looked enough like an array that it wouldn't break everyone's apps ?

Probably, but it's probably a fairly large amount of work. If people want to work on it then I'm happy to help provide guidance and the like, but it's probably going to take till 2.3 till we could have something like this land.

That's definitely the right approach but it's not going to be s/Array/NewFancyProxy/g :wink:

Well I'm up for a challenge :slight_smile:

Was just playing around with things and adding

def records.load associations    self.first.class.send :preload_associations, self, associations unless self.empty?    self end

to find_every allows you to do stuff like

conversations = Conversation.find :all conversations.load :messages => :author

It's not hard to add this to association collection either. I don't think this approach can be taking any further and it does mean that you can't Marshal.dump what you get out of a find (not sure how much of an issue this is but there are tests around that area).

I may just be being prematurely paranoid but if find returns a result set object and if the objects in that collection know the result set object (so that for example accessing an association can preemptively load that association for all of them) would you not have the problem that ruby would hold onto the entire result set as long as anyone was holding onto any single object in that collection ?

Fred

It's not hard to add this to association collection either. I don't think this approach can be taking any further and it does mean that you can't Marshal.dump what you get out of a find (not sure how much of an issue this is but there are tests around that area).

I'd definitely prefer to use a class we defined rather than the metaclass approach, but this is the kind of thing you'll need to do.

I may just be being prematurely paranoid but if find returns a result set object and if the objects in that collection know the result set object (so that for example accessing an association can preemptively load that association for all of them) would you not have the problem that ruby would hold onto the entire result set as long as anyone was holding onto any single object in that collection ?

No you're not being paranoid, if the objects can reach their collection (and the have to be able to) then the whole collection will stay in memory while any instance is. ORMs which use this approach and have large user bases typically have things like the notion of a 'detached' object. In this case user's would have to detach those objects.

Or you could hold the association to the collection in a WeakRef, but I'm not sure how well tested or widely used that class is right now...

There could be some other approach I'm forgetting, but it seems to me it's a restriction you'd have to keep in mind.

It's not hard to add this to association collection either. I don't think this approach can be taking any further and it does mean that you can't Marshal.dump what you get out of a find (not sure how much of an issue this is but there are tests around that area).

I'd definitely prefer to use a class we defined rather than the metaclass approach, but this is the kind of thing you'll need to do.

Sure, just thought it was a fun little hack.

I may just be being prematurely paranoid but if find returns a result set object and if the objects in that collection know the result set object (so that for example accessing an association can preemptively load that association for all of them) would you not have the problem that ruby would hold onto the entire result set as long as anyone was holding onto any single object in that collection ?

No you're not being paranoid, if the objects can reach their collection (and the have to be able to) then the whole collection will stay in memory while any instance is. ORMs which use this approach and have large user bases typically have things like the notion of a 'detached' object. In this case user's would have to detach those objects.

Or you could hold the association to the collection in a WeakRef, but I'm not sure how well tested or widely used that class is right now...

Thanks for the confirmation. I'll probably try and give this a go as a
plugin ( if it doesn't end up being too much hassle) as it really
sounds like something that would benefit from people being able to
play with it easily

Fred

Thanks for the confirmation. I'll probably try and give this a go as a plugin ( if it doesn't end up being too much hassle) as it really sounds like something that would benefit from people being able to play with it easily

Yeah it'd be great to be able to get this into people's hands to figure out the real world benefits and costs of this kind of change. It's a lovely theoretical improvement, it'd be nice to validate that. If it gets too cumbersome to hack around, we could add some hooks for the plugin to work with.