#2493 Eager Loading With :limit Returns Wrong Result Set (MySQL) - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2493 ✓resolved

Eager Loading With :limit Returns Wrong Result Set (MySQL)

Reported by Richard Head | April 14th, 2009 @ 09:39 PM | in 2.3.6

The models:


class Package < ActiveRecord::Base
 has_many :deliveries,
          :select=>'id, user, delivered, package_id'
end

class Delivery < ActiveRecord::Base
  belongs_to :package
end

The find:


 Package.find :all, :include=>:deliveries, :order=>'deliveries.delivered desc', :limit=>10

The 1st query:


SELECT DISTINCT `packages`.id FROM `packages` LEFT OUTER JOIN `deliveries` ON deliveries.package_id = packages.id ORDER BY deliveries.delivered desc LIMIT 10

Because DISTINCT is being used with a non-selected order by column, records aren't returned in the desired order, so the 2nd AR query:


select deliveries.* where deliveries.package_id in ( ... )

returns the wrong rows. Wrong meaning X.delivered > Y.delivered yet X is not in the result set.

MySQL doesn't impose the "order by columns are required in select if DISTINCT is used". I wonder how this query would be generated on say MSSQL.

Comments and changes to this ticket

CancelProfileIsBroken August 6th, 2009 @ 01:44 PM
- Tag set to “bugmash”

Wesley Moxam August 8th, 2009 @ 01:58 AM

Tested on 2-3-stable:


def test_eager_association_loading_with_belongs_to_and_limit_non_selected_order_by
  comments = Comment.find(:all, :include => :post, :limit => 5, :order => 'comments.post_id')
  puts comments.inspect
  assert_equal 5, comments.length
  assert_equal [1,2,3,5,6], comments.collect { |c| c.id }
end

got:

SELECT * FROM "comments" ORDER BY comments.post_id LIMIT 5;
SELECT * FROM "posts" WHERE ("posts"."id" IN (1,2,4));

You flagged this item as spam.
railsbob August 8th, 2009 @ 03:32 AM
Verified

I was able to test this using following scenario:
```
  class A < ActiveRecord::Base
    has_many :bs, :select => 'id, position, a_id'
  end

  class B < ActiveRecord::Base
    belongs_to :a
  end
```
Data in Table 'as':
+----+------+ | id | name | +----+------+ | 1 | A1 | | 2 | A2 | | 3 | A3 | +----+------+

Data in Table 'bs':
+----+------+----------+ | id | a_id | position | +----+------+----------+ | 1 | 1 | 1 | | 2 | 1 | 4 | | 3 | 2 | 6 | | 4 | 2 | 2 | | 5 | 2 | 5 | | 6 | 3 | 3 | +----+------+----------+

Finding without :limit option yields:
```
  >> A.find :all, :include => :bs, :order => 'bs.position desc'
  => [#<A id: 2 ... >, #<A id: 1 ... >, #<A id: 3 ... >]
```
(looks correct)

However, finding with :limit gives:
```
  >> A.find :all, :include => :bs, :order => 'bs.position desc', :limit => 2
  => [#<A id: 2 ... >, #<A id: 3 ... >]
```
Instead, it should give
```
  [#<A id: 2 ... >, #<A id: 1 ... >]
```

railsbob August 8th, 2009 @ 03:34 AM

Sorry for the formatting mess, the tables were supposed to be:


Data in Table 'as':

+----+------+
| id | name |
+----+------+
|  1 | A1   | 
|  2 | A2   | 
|  3 | A3   | 
+----+------+

Data in Table 'bs':
+----+------+----------+
| id | a_id | position |
+----+------+----------+
|  1 |    1 |        1 | 
|  2 |    1 |        4 | 
|  3 |    2 |        6 | 
|  4 |    2 |        2 | 
|  5 |    2 |        5 | 
|  6 |    3 |        3 | 
+----+------+----------+

You flagged this item as spam.
Wesley Moxam August 8th, 2009 @ 03:36 AM
Ok, I had that backwards. I've attached a corrected patch that fails on Mysql & Sqlite3. It passes on PostgresSQL.
- test_eager_association_loading_with_belongs_to_and_limit_non_selected_order_by.diff 1.2 KB
You flagged this item as spam.
Wesley Moxam August 9th, 2009 @ 01:41 AM
I've attached railsbob's example as a test
Wesley Moxam August 9th, 2009 @ 01:44 AM
- no changes were found...
- test_eager_association_loading_with_belongs_to_and_limit_non_selected_order_by.diff 5.9 KB
Jeremy Kemper August 9th, 2009 @ 01:46 AM
- State changed from “new” to “verified”
- Milestone changed from “2.x” to “2.3.4”
You flagged this item as spam.
Derander August 10th, 2009 @ 07:53 AM
+1 verified.

The patches above apply & fail on latest master.
Jeremy Kemper September 11th, 2009 @ 11:04 PM
- Milestone changed from “2.3.4” to “2.3.6”
[milestone:id#50064 bulk edit command]
You flagged this item as spam.
dira September 27th, 2009 @ 11:24 AM
Verified, +1

The preivious patch does not apply anymore. The attached one applies cleanly on master and demonstrates the bug
- 2493_master_test.diff 1.2 KB
You flagged this item as spam.
Greg Sterndale January 21st, 2010 @ 05:22 AM
-1

I don't believe that :limit or :select are really the problem here.

Unlike Postgres, MySQL orders null values first by default (ASC). So, in the Post has_many Comments example:
```
Post.find(:all, :include => :comments, :order => 'comments.post_id asc')
```
Because we're ordering by the foreign key, Posts without Comments are before those with Comments.
When limiting the Posts returned in the find() above to n, you get the first n records (as expected), nulls first in MySQL.

I've attached a patch to help illustrate.
- illustrate-mysql-null-order.diff 2.3 KB
Rizwan Reza February 12th, 2010 @ 12:46 PM
- Tag cleared.
[bulk edit]
Rizwan Reza May 16th, 2010 @ 02:41 AM
- Tag set to “bugmash”
[bulk edit]
Rizwan Reza May 16th, 2010 @ 02:43 AM
- Tag cleared.
- State changed from “verified” to “open”
Greg, do you mean that the ticket can be closed?
Rizwan Reza May 16th, 2010 @ 02:43 AM
- Tag set to “bugmash”
You flagged this item as spam.
Greg Sterndale May 16th, 2010 @ 02:26 PM
Yes. I'd say close it Rizwan.
You flagged this item as spam.
Bastes May 16th, 2010 @ 03:26 PM
Looks applied already.
Rizwan Reza May 17th, 2010 @ 03:26 PM
- Tag cleared.
- State changed from “open” to “resolved”
Ryan Bigg October 11th, 2010 @ 12:11 PM
- Importance changed from “” to “Low”
Automatic cleanup of spam.