This project is archived and is in readonly mode.

#528 ✓resolved
Peter Wagenet

Check joins when determining preloading of includes

Reported by Peter Wagenet | July 1st, 2008 @ 07:08 PM | in 2.x

Currently when calling ActiveRecord::Base#find with both an include clause and an order clause which references the include, ActiveRecord::Associations::ClassMethods#find_with_associations is used. This discards the select clause. In some cases, this is not the intended behavior. Take the following contrived example: 

Person.find(:all, :select => "people.*, email.address IS NOT NULL AS has_email", :order => "email.address", :include => :email, :phone, :address)

The generated query will end up looking something like:

SELECT `people`.`id` AS t0_r0, `people`.`first_name` AS t0_r1 ... FROM `people` LEFT OUTER JOIN `emails` ON `people`.id = `emails`.person_id LEFT OUTER JOIN `phones` ON `people`.id = `phones`.person_id LEFT OUTER JOIN `addresses` ON `people`.id = `addresses`.person_id ORDER BY email.address;

As you can see, the provided select clause has been discarded in place of the complex one needed to load the includes.

I suggest that the user should be able to also specify a join clause which, if it includes the right tables, will allow ActiveRecord::AssociationPreload::ClassMethods#preload_associations to be used instead.

This has a two fold benefit of allowing a select clause to be used. And of still splitting the eager loading into multiple queries.

A revised query would look something like: 

Person.find(:all, :select => "people.*, email.address IS NOT NULL AS has_email", :order => "email.address", :joins => :email, :include => :email, :phone, :address)

With the first query (there will now be multiple for eager loading), looking like:

SELECT people.*, email.address IS NOT NULL AS has_email FROM `people` INNER JOIN `emails` ON `people`.id = `emails`.person_id ORDER BY email.address;

A patch for this functionality is attached.

Comments and changes to this ticket

  • Peter Wagenet

    Peter Wagenet July 1st, 2008 @ 08:37 PM

    I've now added a test for this change.

  • Tiago Macedo

    Tiago Macedo July 1st, 2008 @ 09:54 PM

    I found a bug in your code (it isn't specific to YOUR code, but it wasn't there before):

    If you add this to your test:

    posts = Post.find(:all, :select => "posts.*, authors.name AS test_name",
                          :include => [ :comments, :author, :categories ], :limit => 1)
assert_equal "David", posts.first.test_name

    You'll see that it fails. This will only happen using the old behaviour and a limit clause. This wasn't an issue before because the select clause was being discarded.

    I think you'll have to check which tables are referenced in the select clause and force their eager load (in the same way that's being done for the conditions and order options)

  • Peter Wagenet

    Peter Wagenet July 1st, 2008 @ 11:44 PM

    Turns out the real issue isn't the order or limit clause. It's the select clause. If the any of the select, conditions or order reference the include tables then the select clause ends up getting discarded. With my fix, the solution in any of these events is to use an additional joins parameter as shown above. I've changed my tests to be more generic.

  • Frederick Cheung

    Frederick Cheung December 17th, 2008 @ 02:38 PM

    I've been working on this too. In that my patch is largely similar to yours, except that in the case when joins is a list/hash of association names I'm pulling the tables names straight out of the join dependency rather than turning the join dependency into a string and parsing that

  • Repository

    Repository December 18th, 2008 @ 07:22 PM

    • State changed from “new” to “resolved”

    (from [c9ab7098be7bdd748c0f4a49c8ef015b4aad3108]) Ensure :include checks joins when determining if it can preload [#528 state:resolved] http://github.com/rails/rails/co...

Create your profile

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

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages