Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

build_arel causes counterintuitive behavior with group and order

#4545

Current version of Arel does not handle multiple calls to order and group as it would appear build_arel expects it to.

ruby-1.8.7-p249 > Article.order(:id, :created_by).to_sql
=> "SELECT     \"articles\".* FROM       \"articles\" ORDER BY  created_by, id"

ruby-1.8.7-p249 > Article.group(:id, :created_by).to_sql
=> "SELECT     \"articles\".* FROM       \"articles\" GROUP BY  created_by"

With order, the last added order is given precedence. With group_by, only one call is allowed. This patch calls both a single time with the entire usable list of order/group parameters and results in the expected behavior.

Thanks!

Reported by Ernie Miller · May 6th, 2010 @ 09:28 PM

State: committed
Milestone: none
Assigned to: Jeremy Kemper Jeremy Kemper
Importance: Low

Activity

  1. Ernie Miller
    Ernie Miller
    • Assigned user set to Jeremy Kemper

    Jeremy,

    As much as I hate "assigning" tickets since you and I talked about this in IRC and you described the desired behavior, I figure you're the guy to send it to. :)

    May 6th, 2010 @ 09:34 PM

  2. Jeremy Kemper
    Jeremy Kemper
    • State changed from new to open 
      1) Error:
test_find_keeps_multiple_group_values(BasicsTest):
ActiveRecord::StatementInvalid: PGError: ERROR:  column "developers.id" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: SELECT     "developers".* FROM       "developers" GROUP BY  ...
                   ^
: SELECT     "developers".* FROM       "developers" GROUP BY  developers.name, developers.salary

    May 6th, 2010 @ 10:53 PM

  3. Ernie Miller
    Ernie Miller

    Argh. CURSE YOU, POSTGRESQL!

    Fix forthcoming.

    May 6th, 2010 @ 11:02 PM

  4. Ernie Miller
    Ernie Miller

    Here you go!

    May 6th, 2010 @ 11:12 PM

  5. Jeremy Kemper
    Jeremy Kemper

    Thanks! Squashed and removed the extra commit. Looks like a good change but it needs its own test.

    May 7th, 2010 @ 12:06 AM

  6. Repository
    Repository
    • State changed from open to committed

    May 7th, 2010 @ 12:06 AM

  7. Ernie Miller
    Ernie Miller

    Works for me! I only added the last patch in because I inadvertently removed the SqlLiteral in the first one -- see the line:

    -        arel = arel.order(Arel::SqlLiteral.new(o.to_s)) if o.present?

    But still, it passes all tests either way, and I think in the case of order clauses the SqlLiteral is redundant if you're already doing a to_s:

    articles = Article.arel_table
articles.order(Arel::SqlLiteral.new('blah')).orders == articles.order('blah').orders
=> true

    May 7th, 2010 @ 01:36 AM

  8. Jeremy Kemper
    Jeremy Kemper

    Ha - missed that :)

    May 7th, 2010 @ 01:47 AM