This project is archived and is in readonly mode.

#501 ✓resolved
Andrew White

Merge :joins instead of clobbering them

Reported by Andrew White | June 27th, 2008 @ 05:35 PM

The attached patch enhances with_scope, etc. by enabling the merging of :joins specified either in the new style or old style sql fragments.

1. Where both are new style they are merged in the same manner as :include.

2. Where both are strings they are concatenated together.

3. Where one is a new style and the other is a string then the new style :joins is converted to an sql fragment and then concatenated with the other sql fragment.

4. construct_finder_sql adds the DISTINCT sql keyword to limit the number of rows returned as joins generally multiply the number of rows returned - this is an enhancement to the change in ticket #46.

5. Tests have been added and all tests pass

6. Documentation has been updated to reflect that :joins as well as :include and :conditions are merged.

Comments and changes to this ticket

  • Jeremy Kemper

    Jeremy Kemper June 28th, 2008 @ 02:06 AM

    • State changed from “new” to “open”
    • Milestone cleared.
    • Assigned user set to “Jeremy Kemper”

    Hey Andrew, nice patch. Could you explain adding DISTINCT to the finder sql?

  • Andrew White

    Andrew White June 28th, 2008 @ 06:22 AM

    The DISTINCT is for when you're joining a has_many or habtm association - e.g. a post joined with comments:

    Post.find(:all, :joins => :comments, :conditions => 'comments.approved = 1')

    Ticket #46 scopes the select to the posts table (i.e. 'posts.*') but if a post has more than one comment approved you'll get back duplicate post instances. Adding 'DISTINCT posts.*' eliminates these duplicate records.

    There may by some circumstances where you don't want the DISTINCT in which case you can override it with :select, but the majority of cases would need it so that's why I've added it by default.

  • Pratik

    Pratik July 12th, 2008 @ 03:06 AM

    I agree with Andrew that DISTINCT would make sense in a lot of cases. However, using DISTINCT can easily become a performance problem if the query has LIMIT, ORDER BY etc. Apart from that, it'll also change the existing behavior, which can possibly break some stuff. So I'm not sure if it'll be a sensible default or not.

    How about adding :distinct option to find ?

    Interested in hearing more thoughts on this.

  • Andrew White

    Andrew White July 16th, 2008 @ 11:31 AM

    We could do a :distinct option and then set it true by default if we detect a has_many or habtm join being specified. This would only be when using the new style join syntax limiting any backward compatibility problems.

  • Pratik

    Pratik July 16th, 2008 @ 11:34 AM

    I'm not convinced about setting it to default.

  • Andrew White

    Andrew White July 16th, 2008 @ 12:13 PM

    If we don't set it as the default when the destination join has more than one matching row then we'll get duplicate instances of the parent. I can't think of a use case where this would correct or useful.

    However looking at count it sets :distinct => true for :include and leaves it out when using :joins, so I guess the precedent is to make the developer specify it.

  • Andrew White

    Andrew White July 29th, 2008 @ 03:14 PM

    I've modified the patch to only merge :joins and not bother about making the select statement distinct. I'll create a separate ticket and patch for adding a distinct option to find.

    One question regarding the distinct option for find - which of the following would be better:

    Post.find :all, :distinct => true, :joins => :comments

    or

    Post.find :distinct, :joins => :comments

  • Pratik

    Pratik July 29th, 2008 @ 03:18 PM

    "Post.find :all, :distinct => true, :joins => :comments"

    We can always do "Post.all, :distinct => true, :joins => :comments" once that's done.

  • Jeremy Kemper

    Jeremy Kemper August 28th, 2008 @ 07:38 AM

    The patch looks good. Could you rebase against master?

  • Andrew White

    Andrew White August 28th, 2008 @ 05:06 PM

    Updated patch rebased against master

  • Repository

    Repository August 28th, 2008 @ 08:09 PM

    • State changed from “open” to “resolved”

    (from [db22c89543f45d7f27847003af949afa21cb6fa1]) Merge scoped :joins together instead of overwriting them. May expose scoping bugs in your code!

    [#501 state:resolved]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...

  • David Stevenson

    David Stevenson September 18th, 2008 @ 07:31 PM

    • Tag changed from “activerecord, enhancement, joins, patch” to “activerecord, enhancement, joins, patch”

    Wait!

    Is there a reason merge_joins isn't used inside of with_scope to merge joins there too?

    Without it in that location, it's not possible to chain multiple named_scopes that have joins.

  • Andrew White

    Andrew White September 19th, 2008 @ 06:01 AM

    It is used in with_scope - chaining multiple named scopes works fine for me. Do you have an example that doesn't work?

  • Yi Lin

    Yi Lin March 5th, 2009 @ 03:28 AM

    What happened to the patch for DISTINCT to work?

  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 10:02 PM

    • Tag cleared.
    • Importance changed from “” to “Low”

    Automatic cleanup of spam.

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

Referenced by

Pages