#269 ✓resolved
Troy K

n+1 when using :fields

Reported by Troy K | May 9th, 2008 @ 01:29 PM

Company.all(:fields=>[:id,:name], :name.like => "%a%", :order=>[:name])

produces the following queries:

"SELECT `id`, `name` FROM `companies` WHERE (`name` LIKE '%a%') ORDER BY name asc"

"SELECT `id`, `updated_at` FROM `companies` WHERE (`id` IN (2, 4, 3))"

"SELECT `id`, `created_at` FROM `companies` WHERE (`id` IN (2, 3, 4))"

"SELECT `id`, `owner_id` FROM `companies` WHERE (`id` IN (2, 3, 4))"

"SELECT `id`, `num_contacts` FROM `companies` WHERE (`id` IN (2, 3, 4))"

The last 4 queries is dm going after each additional property not specified in the original :fields, if the resource had 5 more properties there would be 5 more queries

Comments and changes to this ticket

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) May 14th, 2008 @ 10:54 AM

    Troy, in this case, are you actually accessing those extra fields, or are the queries firing regardless of whether or not you access those fields?

    Assuming you haven't defined those other fields in different lazy contexts, if you accessed the extra fields not in the original query, at most you should see two queries: one for the original query of just :id and :name, and a second for the other attributes. At least that's how its supposed to work.

    Would you mind also pasting the model you're using so we can see how you've defined the properties?

  • Troy K

    Troy K May 14th, 2008 @ 01:27 PM

    Sorry for sitting on my brain... I was entering `Company.all(:fields=>[:id,:name], :name.like => "%a%", :order=>[:name])` from irb, which results in the attribute access via Resource::inspect.

    But, it still does many queries instead of just 2, which I've uploaded a patch.

    Maybe inspect should only access loaded_attributes? I'm new to Ruby and do not have a deep understanding of inspect's purpose outside of irb, but if it would be desirable to not have inspect cause additional queries, I would love to code it up.

    I'm stoked with the direction dm is headed, you guys are doing an awesome job!

  • Sam Smoot

    Sam Smoot May 14th, 2008 @ 10:28 PM

    • State changed from “new” to “invalid”

    Troy,

    This is by design. You might want to familiarize yourself with this article: http://datamapper.org/articles/s... which describes the contextual lazy-loading that Dan described.

    #inspect does force the other attributes to load. In IRB it will prompt this exact question basically, but the alternative is to see something like: just because @body hasn't been loaded yet. Which is (to me at least) even more confusing. I want it to tell me wether it's really nil or not.

    In the context of an actual application you're unlikely to ever call #inspect outside of logging, so it doesn't really have any impact there.

  • Troy K

    Troy K May 14th, 2008 @ 11:58 PM

    • Title changed from “aggressive queries w/ :fields specified (.09)” to “n+1 when using :fields”

    Sam,

    I completely agree with the concepts of lazy loading and lazy contexts as described in the article, however I believe the patch I submitted makes the lazy concept even more strategic by creating a virtual lazy context of all the properties that have not been defined as lazy. For example:

    class Company

    include DataMapper::Resource

    property :id, Integer, :serial => true

    property :updated_at, DateTime, :lazy=>true

    property :created_at, DateTime, :nullable => false

    property :owner_id, Integer, :nullable => false

    property :num_contacts, Integer, :nullable => false

    property :name, String, :nullable => false

    property :description, Text, :lazy => true

    end

    1) Company.all(:fields=>[:name]) implies I'm expecting just to use the name property from the company resource (at least most of the time), so lets save some CPU and IO -- cool

    2) If I access :created_at, I think it would be more strategic to grab all of the non-lazy properties, under the assumption that those may be accessed as well, and if they were, a n+1 situation exists, and that getting all of the unloaded non-lazy properties would be more efficient then the n+1 scenario.

    3) properties defined as lazy stay lazy, as they are assumed to be heavy and already support the concept of a context.

    I guess another way to put it is that non-lazy properties deserve context support just like lazy properties, but it would be silly to define a context for them because it is only relevant when limiting the properties loaded via :fields, thus we handle it virtually.

    what are your thoughts?

    • troy
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) May 15th, 2008 @ 12:15 AM

    Sam, I think Troy does bring up one case where we could further improve the performance of lazy loading.

    Right now we have properties that are lazy or not, and some are in one or more context's. You load a field in a context and all the other fields in that context are loaded too. That part is fine, and I think we all understand that. I was thinking though, what if every property that is not lazy belongs to a :default context? That way if you had an object that only had it's primary key loaded, and you accessed one of the :default context, the normal attributes in the resource would be loaded at once.

    Troy's patch essentially gives this now, but I would kind of prefer to be explicit with this. If we stop thinking of properties as lazy or not lazy, and instead think of them as belonging to "load groups" we can get this behavior. We could have property.load_contexts that returns a list of contexts a property belongs to.. when it would normally be empty, it returns a single Array containing [:default].

    Above I mentioned a case where you might have a resource with just its primary keys defined. This can happen in different ways, but the most common one is belongs_to associations. In those cases you know the primary key of the parent resource, so you could get back a resource object instantly by doing resource.parent without hitting the back end. Now if you're just using this parent's keys or something you save on the backend hit, but if you access one of the :default properties the resource will then load itself into memory with no effort on the calling code's part.

  • Sam Smoot

    Sam Smoot May 15th, 2008 @ 12:21 AM

    • State changed from “invalid” to “open”
  • Sam Smoot

    Sam Smoot May 20th, 2008 @ 09:27 PM

    • State changed from “open” to “resolved”

    Patch applied and pushed.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

People watching this ticket

Attachments

Tags

Pages