#4597 Queries using limits and punctuation in order clauses generate invalid SQL - Ruby on Rails

This project is archived and is in readonly mode.

#4597 ✓committed

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.

bug.txt 4.9 KB

Comments and changes to this ticket

You flagged this item as spam.
Adam Keys May 14th, 2010 @ 10:36 PM
This is occurring under rails-3.0.0.beta3.
Santiago Pastorino May 14th, 2010 @ 11:19 PM
Can you provide a test case?
And please try on master
You flagged this item as spam.
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.
- testcase.diff 1002 Bytes
- fail.txt 3 KB
Santiago Pastorino May 15th, 2010 @ 05:01 PM
Ok i'm going to try with your info, thx
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 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 May 15th, 2010 @ 08:41 PM
- Tag set to “bugmash”

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.

You flagged this item as spam.
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.
- testcase.diff 1.4 KB
You flagged this item as spam.
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 May 20th, 2010 @ 10:14 PM
- State changed from “invalid” to “open”
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.
- limits_and_punctiation_in_order.patch 6 KB
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.
- limits_and_punctuation_in_order.patch 6.2 KB

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 May 21st, 2010 @ 10:01 PM
Adam you have to patch Arel too
Santiago Pastorino May 22nd, 2010 @ 12:45 AM
- Assigned user changed from “Santiago Pastorino” to “Jeremy Kemper”
You flagged this item as spam.
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 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 May 23rd, 2010 @ 09:20 PM
Passing an array makes sense. What about deprecation in 2.3.7?
Rohit Arondekar August 3rd, 2010 @ 08:01 AM
- Importance changed from “” to “Low”
Any updates here?
Santiago Pastorino August 3rd, 2010 @ 04:44 PM
- Assigned user changed from “Jeremy Kemper” to “Santiago Pastorino”
Jeremy Kemper August 30th, 2010 @ 04:10 AM
- Milestone cleared.
[bulk edit]
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]
Santiago Pastorino November 4th, 2010 @ 10:08 PM
- Assigned user changed from “Santiago Pastorino” to “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 November 4th, 2010 @ 10:27 PM
- State changed from “open” to “needs-more-info”
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 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...