This project is archived and is in readonly mode.
Queries using limits and punctuation in order clauses generate invalid SQL
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.89 KB
-
Adam KeysThis is occurring under rails-3.0.0.beta3.
-
Santiago PastorinoCan you provide a test case?
And please try on master -
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.
-
testcase.diff
1002 Bytes
-
fail.txt
3.03 KB
-
-
Santiago Pastorino
Ok i'm going to try with your info, thx
-
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
-
Santiago Pastorino
Perhaps you misspeled order and tried to do another thing, please fix it if this is the case
-
Santiago Pastorino
- Tag set to bugmash
-
Adam Keys
The reason I believe this is a bug is that it works when there is no
limitclause. 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
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.45 KB
-
-
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.
-
José Valim- State changed from invalid to open
-
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.
-
Santiago Pastorino
The previous patch makes SQLite fail because it doesn't support LEAST function.
This is the right one. -
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.
-
Santiago Pastorino
Adam you have to patch Arel too
-
Santiago Pastorino
- Assigned user changed from Santiago Pastorino to Jeremy Kemper
-
Adam Keys
+1
I switched my bundle to use your fork of arel and, with your patch, everything is passing.
Thanks!
-
Santiago Pastorino
I've thought on other approaches less obtrusives than this, but need to talk to Jeremy Kemper about this.
-
Jeremy KemperPassing an array makes sense. What about deprecation in 2.3.7?
-
-
Santiago Pastorino
- Assigned user changed from Jeremy Kemper to Santiago Pastorino
-
-
-
Santiago Pastorino
- Assigned user changed from Santiago Pastorino to 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.
-
Aaron Patterson
- State changed from open to needs-more-info
-
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. -
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...