Rails core - Jun 2006 - Patch for #3438: Eager loading doesn''t respect :order of association

Hello all,

I just submitted a patch for #3438
[http://dev.rubyonrails.org/ticket/3438], to make eagerly-loaded
associations have the same order as regularly-loaded associations. It
passes all existing MySQL tests, and I''ve added two more fairly
comprehensive tests which make sure that all eagerly-loaded
associations have the same order as non-eagerly-loaded ones.

Let me know if there''s anything I should change.

Thanks!

-- 
Coda Hale
http://blog.codahale.com

On 6/24/06, Coda Hale <coda.hale@gmail.com> wrote:
> Hello all,
>
> I just submitted a patch for #3438
> [http://dev.rubyonrails.org/ticket/3438], to make eagerly-loaded
> associations have the same order as regularly-loaded associations. It
> passes all existing MySQL tests, and I''ve added two more fairly
> comprehensive tests which make sure that all eagerly-loaded
> associations have the same order as non-eagerly-loaded ones.
>
> Let me know if there''s anything I should change.
>
> Thanks!
>
> --
> Coda Hale
> http://blog.codahale.com
This has annoyed me for so long, thx for the fix.

-- 
Corey
http://www.atmos.org/

Okay, so I''ve realized that just posting an "oi, patch over
here"
message doesn''t quite convince anyone to spend their time sorting
through the whole thing themselves, so here''s a run-down of the ticket
and my patch, in the hopes of convincing rails-core that the bug is
important and the patch is solid.

The problem:

  Author.find(1).posts != Author.find(1, :include => [:posts]).posts

if Author has_many :posts, :order => anything

The :order option of an association isn''t respected by eager loading,
which forces developers to either repeat the :order option for each
eagerly loaded find, or to actually sort things themselves. The
workarounds are neither pretty, DRY, nor necessary. This is not a
design decision nor a technical limitation; this is just functionality
which never got added in.

The solution:

My patch adds ~20 lines of code to
ActiveRecord::Associations::ClassMethods#construct_finder_sql_with_included_associations
which run through the various associations in the join dependency,
extract all :order options, expand them to their full form (e.g.,
''posts.published_at'' instead of
''published_at'', also minding table
aliases), and append them to the SQL''s ORDER BY clause. If there is no
existing :order option in the find itself, the ORDER BY clause begins
with the primary key of the model being searched, which is the default
SQL order.

This patch passes all existing unit tests without modification, and
I''ve added two units tests which have a combined total of 38
assertions which check that the eagerly-loaded and non-eagerly-loaded
associations both have the exact same order. The code is written to
standards, has comments to explain any tricky bits, and doesn''t rely
on high magic. It doesn''t break any existing code, works with
STI/explicit class names/primary keys, doesn''t overshadow the find
method''s :order options, and smells faintly of apple pie.

So please, take a look at this. I''m willing to go point on this bug,
and if the patch isn''t satisfying, I''m willing to do whatever
it takes
to get it in. Finally, I''m willing to resort to bribery/capitalism and
buy any member of rails-core in the San Francisco Bay Area a pint (or
equivalent) who either accepts this patch or fixes this bug.

The ticket: http://dev.rubyonrails.org/ticket/3438
My patch:
http://dev.rubyonrails.org/attachment/ticket/3438/eager_loading_order_patch_with_unit_tests.diff

Thanks!

-- 
Coda Hale
http://blog.codahale.com

On Jul 2, 2006, at 8:21 , Coda Hale wrote:
> If there is no
> existing :order option in the find itself, the ORDER BY clause begins
> with the primary key of the model being searched, which is the default
> SQL order.
I can''t speak to the correctness of the rest of the patch (though it  
seems pretty nifty to me :), this assumption is false. There is no  
guaranteed order in SQL without an explicit ORDER BY clause. Any  
ordering you may see in cases without an ORDER BY clause is an  
implementation artifact and can easily vary with the dbms and even  
the query itself.

I''m not familiar enough with the ActiveRecord code to know whether or  
not ActiveRecord adds a default ORDER BY #{table_name}.# 
{primary_key}, in which case what is specified by the SQL standard is  
pretty much moot so feel free to ignore all of this :). I''d be  
surprised if ActiveRecord *does* add a default ORDER BY as this could  
affect performance.

 From what I see, this is just a small part of the patch though, and  
it can easily be amended by removing c136-140.

Michael Glaesemann
grzm seespotcode net

On 7/1/06, Michael Glaesemann <grzm@seespotcode.net>
wrote:
> On Jul 2, 2006, at 8:21 , Coda Hale wrote:
> > If there is no
> > existing :order option in the find itself, the ORDER BY clause begins
> > with the primary key of the model being searched, which is the default
> > SQL order.
>
> I can''t speak to the correctness of the rest of the patch (though
it
> seems pretty nifty to me :), this assumption is false. There is no
> guaranteed order in SQL without an explicit ORDER BY clause. Any
> ordering you may see in cases without an ORDER BY clause is an
> implementation artifact and can easily vary with the dbms and even
> the query itself.
Well, I''m working with MySQL, which returns records by order of
primary key. (I think.) Does anyone know of any Rails-supported RDBMS
which doesn''t do this?
> I''m not familiar enough with the ActiveRecord code to know whether
or
> not ActiveRecord adds a default ORDER BY #{table_name}.#
> {primary_key}, in which case what is specified by the SQL standard is
> pretty much moot so feel free to ignore all of this :). I''d be
> surprised if ActiveRecord *does* add a default ORDER BY as this could
> affect performance.
It doesn''t add an ORDER BY clause unless one is provided, and
I''d
imagine that adding an ORDER BY clause to a JOIN statement would add
minimal overhead. For a straight SELECT, though, I totally agree.
>  From what I see, this is just a small part of the patch though, and
> it can easily be amended by removing c136-140.
This would introduce an unintended side effect, namely that the
following two pieces of code would not return the same results:

  Author.find(:all).map(&:id) != Author.find(:all, :include =>
[:posts]).map(&:id)

The array of Authors would be ordered by the default RDBMS order in
the first instance, and by the :order option of Author''s post
association in the second. Ideally, it would be nice to nail this down
so that both eagerly- and non-eagerly-loaded associations work the
same, but I''m personally in favor of trading the weirdness I have to
deal with for the one I can ignore. Some people may have a different
perspective. ;-)

Can any database ninjas within earshot give any advice about the most
polite way of doing this, or is prepending the first-order model''s
primary key to the ORDER BY clause a reasonable thing to do?

Also, can anyone tell me what this patch does off of MySQL, regarding
unit tests? I''d run PostgreSQL myself, but I''m humbled by my
inability
to configure the damn thing.

-- 
Coda Hale
http://blog.codahale.com

On Jul 2, 2006, at 17:15 , Coda Hale wrote:
> Well, I''m working with MySQL, which returns records by order of
> primary key. (I think.) Does anyone know of any Rails-supported RDBMS
> which doesn''t do this?
PostgreSQL follows the SQL-spec in this case.

Michael Glaesemann
grzm seespotcode net

Coda Hale wrote:
> Well, I''m working with MySQL, which returns records by order of
> primary key. (I think.) Does anyone know of any Rails-supported RDBMS
> which doesn''t do this?
Oracle doesn''t.

On 7/2/06, Michael Glaesemann <grzm@seespotcode.net>
wrote:
>
> On Jul 2, 2006, at 17:15 , Coda Hale wrote:
>
> > Well, I''m working with MySQL, which returns records by order
of
> > primary key. (I think.) Does anyone know of any Rails-supported RDBMS
> > which doesn''t do this?
>
> PostgreSQL follows the SQL-spec in this case.
On 7/2/06, Michael A. Schoen <schoenm@earthlink.net>
wrote:
> Oracle doesn''t [order by primary key].
Grah.

Okay, that''s compelling enough to ditch ordering by the primary key.

The unintended consequences of this is that if someone adds an
:include option with an ordered association in a find without an
:order option, the first-order results will appear in a different
order:

  Author.find(:all).map(&:id) != Author.find(:all, :include =>
[:posts]).map(&:id)

This is not particularly friendly behavior, but the buck for this can
be passed on to the SQL standard: if you want an order, specify an
order. Otherwise, don''t expect anything.

Does this seem like a valid tradeoff to everyone?

-- 
Coda Hale
http://blog.codahale.com

Not particularly friendly indeed, but you''re right - if you want order,
specify it. Like Ruby hashes, you never know what you''re gonna get :)

I don''t know about Oracle, but in Postgres it''s never smart to
rely on
''default'' ordering because when a complex query is altered or
Postgres
version chages, the database may return same records in different order. So
this trade-off -- surprising a few users (who didn''t explicitly
specified an
order) for an ActiveRecord enhancement -- seems to me like a plus. Thumbs up

-M

On 7/2/06, Coda Hale <coda.hale@gmail.com> wrote:
>
> The unintended consequences of this is that if someone adds an
> :include option with an ordered association in a find without an
> :order option, the first-order results will appear in a different
> order:
>
>   Author.find(:all).map(&:id) != Author.find(:all, :include =>
> [:posts]).map(&:id)
>
> This is not particularly friendly behavior, but the buck for this can
> be passed on to the SQL standard: if you want an order, specify an
> order. Otherwise, don''t expect anything.

_______________________________________________
Rails-core mailing list
Rails-core@lists.rubyonrails.org
http://lists.rubyonrails.org/mailman/listinfo/rails-core

On Jul 2, 2006, at 10:24 AM, Coda Hale wrote:
> This is not particularly friendly behavior, but the buck for this can
> be passed on to the SQL standard: if you want an order, specify an
> order. Otherwise, don''t expect anything.
>
> Does this seem like a valid tradeoff to everyone?
I think it''s best that way.  Even on DBs that default to order by id,  
that order can be less meaningful than you''d expect.  Some approaches  
to scaling involve allocating blocks of primary keys to separate DB  
servers, so the id order may not correspond to global creation order.

--
Josh Susser
http://blog.hasmanythrough.com

On 7/2/06, Josh Susser <josh@hasmanythrough.com>
wrote:
>
> On Jul 2, 2006, at 10:24 AM, Coda Hale wrote:
> > This is not particularly friendly behavior, but the buck for this can
> > be passed on to the SQL standard: if you want an order, specify an
> > order. Otherwise, don''t expect anything.
> >
> > Does this seem like a valid tradeoff to everyone?
>
> I think it''s best that way.  Even on DBs that default to order by
id,
> that order can be less meaningful than you''d expect.  Some
approaches
> to scaling involve allocating blocks of primary keys to separate DB
> servers, so the id order may not correspond to global creation order.
Yeah, that makes sense.

Okay, I''ve added another patch to the ticket
(http://dev.rubyonrails.org/attachment/ticket/3438/eager_loading_order_patch_sql_standard.diff)
 which no longer orders by primary key. It has full coverage with 2
unit tests (152 assertions now), and it passes all existing unit
tests.

I''d like to thank everyone for their input and help in making this
patch a lot stronger and for gently reminding me that MySQL is not the
center of the universe.

-- 
Coda Hale
http://blog.codahale.com