This project is archived and is in readonly mode.

#4597 ✓committed
Adam Keys

Queries using limits and punctuation in order clauses generate invalid SQL

Reported by Adam Keys | May 14th, 2010 @ 10:34 PM | in 3.0.2

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.

Comments and changes to this ticket

  • Adam Keys

    Adam Keys May 14th, 2010 @ 10:36 PM

    This is occurring under rails-3.0.0.beta3.

  • Santiago Pastorino

    Santiago Pastorino May 14th, 2010 @ 11:19 PM

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

  • Adam Keys

    Adam Keys May 15th, 2010 @ 04:02 PM

    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.

  • Santiago Pastorino

    Santiago Pastorino May 15th, 2010 @ 05:01 PM

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

  • Santiago Pastorino

    Santiago Pastorino May 15th, 2010 @ 08:11 PM

    • 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

  • Santiago Pastorino

    Santiago Pastorino May 15th, 2010 @ 08:12 PM

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

  • Santiago Pastorino
  • Adam Keys

    Adam Keys May 15th, 2010 @ 08:49 PM

    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.

  • Adam Keys

    Adam Keys May 15th, 2010 @ 09:09 PM

    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.

  • Adam Keys

    Adam Keys May 18th, 2010 @ 03:54 PM

    • 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.

  • José Valim

    José Valim May 20th, 2010 @ 10:14 PM

    • State changed from “invalid” to “open”
  • Santiago Pastorino

    Santiago Pastorino May 21st, 2010 @ 04:53 AM

    • 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.

  • Santiago Pastorino

    Santiago Pastorino May 21st, 2010 @ 11:44 AM

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

  • Adam Keys

    Adam Keys May 21st, 2010 @ 08:59 PM

    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.

  • Santiago Pastorino

    Santiago Pastorino May 21st, 2010 @ 10:01 PM

    Adam you have to patch Arel too

  • Santiago Pastorino

    Santiago Pastorino May 22nd, 2010 @ 12:45 AM

    • Assigned user changed from “Santiago Pastorino” to “Jeremy Kemper”
  • Adam Keys

    Adam Keys May 23rd, 2010 @ 12:29 AM

    +1

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

    Thanks!

  • Santiago Pastorino

    Santiago Pastorino May 23rd, 2010 @ 02:01 AM

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

  • Jeremy Kemper

    Jeremy Kemper May 23rd, 2010 @ 09:20 PM

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

  • Rohit Arondekar

    Rohit Arondekar August 3rd, 2010 @ 08:01 AM

    • Importance changed from “” to “Low”

    Any updates here?

  • Santiago Pastorino

    Santiago Pastorino August 3rd, 2010 @ 04:44 PM

    • Assigned user changed from “Jeremy Kemper” to “Santiago Pastorino”
  • Jeremy Kemper
  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
  • Santiago Pastorino

    Santiago Pastorino November 4th, 2010 @ 10:08 PM

    • Assigned user changed from “Santiago Pastorino” to “Aaron Patterson”
  • Aaron Patterson

    Aaron Patterson November 4th, 2010 @ 10:27 PM

    @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.

  • Aaron Patterson

    Aaron Patterson November 4th, 2010 @ 10:27 PM

    • State changed from “open” to “needs-more-info”
  • Santiago Pastorino

    Santiago Pastorino November 4th, 2010 @ 11:28 PM

    • 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.

  • Repository

    Repository November 5th, 2010 @ 06:29 PM

    • 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 state:committed] https://github.com/rails/rails/commit/3146aa68fd03ea4392b45f1c87716...

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages