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.

Queries using limits and punctuation in order clauses generate invalid SQL

#4597

If one creates a query that uses both limits and order clauses that contain punctuation, column aliases are put in the wrong place and an invalid query is generated.

For example:

# Works
Trip.includes(:spots).offset(0).limit(20).order("LEAST(COS(1)*COS(-1)*COS(RADIANS(spots.lat)))").all
# Fails (note the comma)
Trip.includes(:spots).offset(0).limit(20).order("LEAST(1,COS(1)*COS(-1)*COS(RADIANS(spots.lat)))").all

The correct query looks like this:

 SELECT * FROM (SELECT DISTINCT ON ("trips".id) "trips".id, LEAST(COS(1)*COS(-1)*COS(RADIANS(spots.lat))) AS alias_0 FROM "trips" LEFT OUTER JOIN "waypoints" ON "trips"."id" = "waypoints"."trip_id" LEFT OUTER JOIN "spots" ON "spots"."id" = "waypoints"."spot_id") AS id_list ORDER BY id_list.alias_0 LIMIT 20 OFFSET 0

The incorrect query ends up like this (note the placement of the first AS):

SELECT * FROM (SELECT DISTINCT ON ("trips".id) "trips".id, LEAST(1 AS alias_0, COS(1)*COS(-1)*COS(RADIANS(spots.lat))) AS alias_1 FROM "trips" LEFT OUTER JOIN "waypoints" ON "trips"."id" = "waypoints"."trip_id" LEFT OUTER JOIN "spots" ON "spots"."id" = "waypoints"."spot_id") AS id_list ORDER BY id_list.alias_0 , id_list.alias_1  LIMIT 20 OFFSET 0

If the limit is removed, the query is generated properly, regardless of order clause.

This script shows a couple working order clauses and one that doesn't.

logger = Logger.new(STDOUT)
ActiveRecord::Base.logger = logger
ActiveRecord::Base.colorize_logging = false

working = ["LEAST(COS(1)*COS(-1)*COS(RADIANS(spots.lat)))",
  "LEAST(1,COS(1)*COS(-1)*COS(RADIANS(lat)))"]
not_working = ["LEAST(1,COS(1)*COS(-1)*COS(RADIANS(spots.lat)))"]

(working + not_working).each do |order|
  begin
    # Spots can belong to multiple trips. Each spot has a lat/lng pair.
    Trip.includes(:spots).offset(0).limit(20).order(order).all
    logger.info "Good: #{order}"
  rescue => e
    logger.info "Bad: #{order}"
    logger.debug e
  end
end

The output of said script is attached.

Reported by Adam Keys · May 14th, 2010 @ 10:34 PM

State: committed
Milestone: 3.0.2
Assigned to: Aaron Patterson Aaron Patterson
Importance: Low

Activity

  1. Adam Keys
    Adam Keys

    This is occurring under rails-3.0.0.beta3.

    May 14th, 2010 @ 10:36 PM

  2. Santiago Pastorino
    Santiago Pastorino

    Can you provide a test case?
    And please try on master

    May 14th, 2010 @ 11:19 PM

  3. Adam Keys
    Adam Keys

    Sure. Attached is a patch to add the test case and the result I get when run against PostgreSQL. I ran this against a recent clone of master.

    May 15th, 2010 @ 04:02 PM

  4. Santiago Pastorino
    Santiago Pastorino

    Ok i'm going to try with your info, thx

    May 15th, 2010 @ 05:01 PM

  5. Santiago Pastorino
    Santiago Pastorino
    • State changed from new to invalid

    Adam Keys the wrong thing here is that you are doing ...

    Trip.includes(:spots).offset(0).limit(20).order("LEAST(COS(1)COS(-1)COS(RADIANS(spots.lat)))").all

    and expecting the order method to add things on the select part of the query

    SELECT * FROM (SELECT DISTINCT ON ("trips".id) "trips".id, LEAST(COS(1)COS(-1)COS(RADIANS(spots.lat))) AS alias_0 FROM "trips" LEFT OUTER JOIN "waypoints" ON "trips"."id" = "waypoints"."trip_id" LEFT OUTER JOIN "spots" ON "spots"."id" = "waypoints"."spot_id") AS id_list ORDER BY id_list.alias_0 LIMIT 20 OFFSET 0

    May 15th, 2010 @ 08:11 PM

  6. Santiago Pastorino
    Santiago Pastorino

    Perhaps you misspeled order and tried to do another thing, please fix it if this is the case

    May 15th, 2010 @ 08:12 PM

  7. Santiago Pastorino
    Santiago Pastorino
    • Tag set to bugmash

    May 15th, 2010 @ 08:41 PM

  8. Adam Keys
    Adam Keys

    The reason I believe this is a bug is that it works when there is no limit clause. Removing it produces this valid query:

    SELECT     "tags"."id" AS t0_r0, "tags"."name" AS t0_r1, "tags"."taggings_count" AS t0_r2, "taggings"."id" AS t1_r0, "taggings"."tag_id" AS t1_r1, "taggings"."super_tag_id" AS t1_r2, "taggings"."taggable_type" AS t1_r3, "taggings"."taggable_id" AS t1_r4 FROM       "tags" LEFT OUTER JOIN "taggings" ON "taggings"."tag_id" = "tags"."id" ORDER BY  LEAST(1,COS(1)*COS(-1)*COS(RADIANS(taggings.taggings_count)))

    The query I have in the test case isn't exactly right for the test schema, but I'm certain that generating this bit of SQL is not correct:

    SELECT DISTINCT ON ("trips".id) "trips".id, LEAST(1 AS alias_0,

    The alias inside the function call is how this bug manifests itself.

    May 15th, 2010 @ 08:49 PM

  9. Adam Keys
    Adam Keys

    Here's a better testcase. Remove the limit and the query is fine but the assert fails. Leave it in and the query is invalid.

    May 15th, 2010 @ 09:09 PM

  10. Adam Keys
    Adam Keys
    • Assigned user set to Santiago Pastorino

    Santiago, could you please review the most recent testcase attached above and clear the invalid flag on this ticket? I believe this is a bonafide bug.

    May 18th, 2010 @ 03:54 PM

  11. José Valim
    José Valim
    • State changed from invalid to open

    May 20th, 2010 @ 10:14 PM

  12. Santiago Pastorino
    Santiago Pastorino
    • Milestone cleared.
    • Tag changed from bugmash to activerecord, arel, limit, order, patch, postgresql

    First of all sorry for closing the ticket, i hadn't understood it really well.

    The issue here is .order(str) and str containing comma inside of expressions when used with limit
    as in order("LEAST(1,COS(1)COS(-1)COS(RADIANS(spots.lat)))").limit(1)

       this comma  ^^^

    I propose to deprecate/remove the following use order("column1, column2") in favor of order(["column1", "column2"]) which is also a cleaner usage.
    If not trying to deal with both cases order("column1, column2") and order("LEAST(1,COS(1)COS(-1)COS(RADIANS(spots.lat)))") the split by comma done in activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:853 couldn't be and we should manage with a StringScanner taking care about things like ((,)),((,)) in we only want to split by the middle comma.

    With this API change accepted we have to do a thsi minimal patch for Arel see here http://github.com/spastorino/arel/commit/6e4b421ac394a4c18d8d35d6a6...

    And apply this patch on master.

    May 21st, 2010 @ 04:53 AM

  13. Santiago Pastorino
    Santiago Pastorino

    The previous patch makes SQLite fail because it doesn't support LEAST function.
    This is the right one.

    May 21st, 2010 @ 11:44 AM

  14. Adam Keys
    Adam Keys

    I applied the patch above and got one failure when I ran the PostgreSQL relation tests:

      1) Error:
test_finding_with_complex_order_and_limit(RelationTest):
ActiveRecord::StatementInvalid: PGError: ERROR:  column id_list.alias_1 does not exist
LINE 1: ...tags"."id") AS id_list ORDER BY id_list.alias_0 , id_list.al...
                                                             ^
: SELECT * FROM (SELECT DISTINCT ON ("tags".id) "tags".id, LEAST(1,COS(1)*COS(-1)*COS(RADIANS(taggings.super_tag_id))) AS alias_0 FROM "tags" LEFT OUTER JOIN "taggings" ON "taggings"."tag_id" = "tags"."id") AS id_list ORDER BY id_list.alias_0 , id_list.alias_1  LIMIT 1
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:210:in `log'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:464:in `execute_without_query_record'
    ./test/cases/helper.rb:40:in `execute'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:954:in `select_raw'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:941:in `select'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:7:in `select_all'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:56:in `select_all'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/base.rb:431:in `find_by_sql'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/relation.rb:64:in `to_a'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/relation.rb:12:in `collect'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/relation/finder_methods.rb:221:in `construct_limited_ids_condition'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/relation/finder_methods.rb:208:in `apply_join_dependency'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/relation/finder_methods.rb:197:in `construct_relation_for_association_find'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/relation/finder_methods.rb:182:in `find_with_associations'
    /Users/adam/Desktop/In/rails/activerecord/lib/active_record/relation.rb:64:in `to_a'
    ./test/cases/relations_test.rb:102:in `test_finding_with_complex_order_and_limit'
    /Users/adam/Desktop/In/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `__send__'
    /Users/adam/Desktop/In/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `run'
    /Users/adam/Desktop/In/rails/activesupport/lib/active_support/callbacks.rb:414:in `_run_setup_callbacks'
    /Users/adam/Desktop/In/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run'

    Will give it a look tomorrow.

    May 21st, 2010 @ 08:59 PM

  15. Santiago Pastorino
    Santiago Pastorino

    Adam you have to patch Arel too

    May 21st, 2010 @ 10:01 PM

  16. Santiago Pastorino
    Santiago Pastorino
    • Assigned user changed from Santiago Pastorino to Jeremy Kemper

    May 22nd, 2010 @ 12:45 AM

  17. Adam Keys
    Adam Keys

    +1

    I switched my bundle to use your fork of arel and, with your patch, everything is passing.

    Thanks!

    May 23rd, 2010 @ 12:29 AM

  18. Santiago Pastorino
    Santiago Pastorino

    I've thought on other approaches less obtrusives than this, but need to talk to Jeremy Kemper about this.

    May 23rd, 2010 @ 02:01 AM

  19. Jeremy Kemper
    Jeremy Kemper

    Passing an array makes sense. What about deprecation in 2.3.7?

    May 23rd, 2010 @ 09:20 PM

  20. Rohit Arondekar
    Rohit Arondekar
    • Importance changed from to Low

    Any updates here?

    August 3rd, 2010 @ 08:01 AM

  21. Santiago Pastorino
    Santiago Pastorino
    • Assigned user changed from Jeremy Kemper to Santiago Pastorino

    August 3rd, 2010 @ 04:44 PM

  22. Jeremy Kemper
    Jeremy Kemper
    • Milestone cleared.

    August 30th, 2010 @ 04:10 AM

  23. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2

    October 15th, 2010 @ 11:01 PM

  24. Santiago Pastorino
    Santiago Pastorino
    • Assigned user changed from Santiago Pastorino to Aaron Patterson

    November 4th, 2010 @ 10:08 PM

  25. Aaron Patterson
    Aaron Patterson

    @Adam can you try this again against 3-0-stable with ARel 2.0.0? I think this is fixed, but I don't know that the number of return rows is correct.

    By "fixed", I mean valid SQL is returned.

    November 4th, 2010 @ 10:27 PM

  26. Aaron Patterson
    Aaron Patterson
    • State changed from open to needs-more-info

    November 4th, 2010 @ 10:27 PM

  27. Santiago Pastorino
    Santiago Pastorino
    • State changed from needs-more-info to open

    I've tried again and is not working. The problem is in the distinct method of postgresql.
    We are going to try some things with Aaron and get back to you.

    November 4th, 2010 @ 11:28 PM

  28. Repository
    Repository
    • State changed from open to committed

    (from [3146aa68fd03ea4392b45f1c8771675a9c850471]) Fixes queries using limits and punctuation in order, removes order("col1, col2") usage in favor of order(["col1", "col2"})

    [#4597 Queries using limits and punctuation in order clauses generate invalid SQL state:committed] https://github.com/rails/rails/commit/3146aa68fd03ea4392b45f1c87716...

    November 5th, 2010 @ 06:29 PM