#903 ✓resolved
Tony Pitale

Has Many :through Many is so close, but wrong

Reported by Tony Pitale | June 14th, 2009 @ 03:31 PM | in 0.10.2

I have a fairly complex relationship chain. The first has many through (Sale => SaleItem) works fine. So I can say user.sale_items and get back what I want. SaleItem belongs to an Item. So, User has many items through the already established sale_items. When I then ask for user.items, the query that is generated is below:


The problem given is that the where condition is looking for sale_items of an id, the id belonging to the user that should really be in the where. Everything else in that query is great!

Comments and changes to this ticket

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 14th, 2009 @ 06:26 PM

    • Assigned user set to “Dan Kubb (dkubb)”
    • State changed from “new” to “unconfirmed”

    Tony: Can you create a stand-alone script that allows me to reproduce this problem locally? It's kind of difficult to work from an SQL query backwards since there are multiple paths the code could take to arrive at this specific query.

  • Tony Pitale

    Tony Pitale June 14th, 2009 @ 08:17 PM

    Working on it. I'm trying to find the cause exactly. I'm having difficulty reproducing outside of a complex example.

  • Tony Pitale
  • Tony Pitale

    Tony Pitale June 14th, 2009 @ 09:33 PM

    A fix for this example is to change target_key to source_key within the DM::Associations::ManyToMany::Relationship#source_scope method. Not sure how well this works all around. I'm now seeking out the spec for this.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 15th, 2009 @ 12:27 PM

    • State changed from “unconfirmed” to “accepted”

    Changing ManyToMany::Relationship#source_scope to something like: through.source_scope(source) solves the problem, but it causes a few other spec failures. More investigation is necessary to find the optimal approach.

    I'm going to experiment with using subqueries to retrieve the m:m collection data rather than joins. It solves a few problems, one of them being aliasing, and conceptually it's simpler to understand and debug, and the basic strategy is applicable to all storage engines. I want to see if perhaps that change can resolve this problem as well, or at the very least make it simpler for us to debug and fix.

  • Tony Pitale

    Tony Pitale June 15th, 2009 @ 01:16 PM

    I was going to mention the performance implications of subqueries, but I guess join is almost as bad depending on the database used. Definitely worth the time to try it out.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 15th, 2009 @ 06:24 PM

    Tony: Yeah, it's definitely something I'm keeping in mind and probably the reason I hadn't done it sooner. However, from what I've read the problem is actually only with MySQL, and only really visible when querying large amounts of data, i.e. 10K-100K+ rows. The typical DM use case would be to pull back alot less number of rows -- to put it bluntly, unless you're pulling back thousands or tens of thousands of rows regularly in a single query, the differences should be extremely tiny. (and if you are pulling back that much data you'll have other bottlenecks besides query time)

    As far as other databases go, most of the ones we support, or plan to support, generate roughly the same query plan for a subquery as the equivalent join. SQLite3 does no optimizations on a subquery vs. joins, so both will be equally as slow.

    Plus there are some cases that are impossible to support with joins, such as when the intermediary table uses LIMIT/OFFSET. In those cases your options are either: a) perform a subquery, b) perform a separate query and specify the PK in an IN, or AND/OR clause when there are multiple PKs, and c) ignore it. Currently we do "c", mostly because many other things had to happen first. The plan was always to support subqueries for this case.

    It's not set in stone, but I will be experimenting with this locally and see how far I can take it. My primary goal is for correct behavior first, and the optimization second. The are alot of problems that using joins causes, and even with alot of internal code fixes I'm not sure there's a clean solution using that approach.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 16th, 2009 @ 03:05 AM

    • Milestone set to 0.10.0
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) October 4th, 2009 @ 09:33 PM

    • Milestone changed from 0.10.0 to 0.10.2

    [project:id#20609 not-tagged:"0.10.0" milestone:id#51895 bulk edit command]

  • Tony Pitale

    Tony Pitale January 29th, 2010 @ 12:33 AM

    Hey Dan. Any word on this ticket?

  • Tony Pitale

    Tony Pitale January 29th, 2010 @ 01:52 AM

    I think the problem may have something to do with inverse or invert on relationship or in M:M. They're using the parent/child immediately. In reality, the inverse when :through many may need to check for the through relationship's child_model. Maybe? I'm not quite sure, and I couldn't get any of this to work properly.

  • Tony Pitale

    Tony Pitale January 29th, 2010 @ 10:19 PM

    This fixes the problem for me: http://gist.github.com/290404 I have no idea how to test this, or if there's a better way. All my tests pass now in my app.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) January 30th, 2010 @ 04:08 AM

    That gist passes fine with sqlite3. I am running it against the full set of adapters now.

    Before I push this I'll probably need to write up a simple failing spec. I presume I can base it off the gists that you've submitted.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) February 1st, 2010 @ 03:30 PM

    Ok, this fix appears to pass when run against all the adapters and all supported versions of Ruby.

    I am going to write up a failing spec, and push it to the repo. Thanks Tony for your help in isolating this bug and fix!

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) February 1st, 2010 @ 04:21 PM

    • State changed from “accepted” to “resolved”

    (from [f7aef8af9153960704311db19cbff723ed05d28e]) Make sure m:m joins join with the FK of the inner association properly

    [#903 state:resolved] http://github.com/datamapper/dm-core/commit/f7aef8af9153960704311db...

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

Referenced by