Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect SQL generated for .last when ordering uses an SQL function #8919

Closed
wants to merge 1 commit into from

Conversation

vanyak
Copy link
Contributor

@vanyak vanyak commented Jan 13, 2013

I bumped into a problem with using COALESCE in sorting but it's relevant to any function like min or max.

When I have an order defined as COALESCE(t.field1, t.field2), all works great until .last is used. ActiveRecord was generating invalid SQL select whatever from t ORDER BY COALESCE(t.field1 DESC, t.field2 DESC) LIMIT 1.

@@ -1226,4 +1226,11 @@

*Aaron Patterson*

* Fix for reverting sorting order when SQL function is used in order clause
This was causing incorrect SQL generation when oder is not just a simple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean « order » here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks.
Fixed.

only outside of parentheses.
When splitting on any commas this causes generation of incorrect SQL if
any SQL functions are used inside sort order.
I.e for "order by max(t.field1, t.field2)" DESC would be inserted twice
@robin850
Copy link
Member

/cc @rafaelfranca @tenderlove

@joxxoxo
Copy link

joxxoxo commented Jan 16, 2013

it's an old issue, please see #8225, #8741, though your solution looks nicer

@vanyak
Copy link
Contributor Author

vanyak commented Jan 18, 2013

I tried to find if somebody has already fixed it before implementing myself. Apparently I did not search good enough.
Anyway I will be happy if any patch would be accepted.

@rafaelfranca
Copy link
Member

@vanyak looks good. Could you apply the tests cases from the other two patches on you pull request and see if all work with your fix?

@joxxoxo
Copy link

joxxoxo commented Jan 19, 2013

actually i think they won't pass:
1.9.3p194 :001 > s='users.id asc, sum_fun(users.name, sum_other_fun(a, b, c), users.surname)'
=> "users.id asc, sum_fun(users.name, sum_other_fun(a, b, c), users.surname)"
1.9.3p194 :002 > s.split(/,(?=(?:[^)]|([^)]))$)/)
=> ["users.id asc, sum_fun(users.name, sum_other_fun(a, b, c), users.surname)"]

my solution from #8741:
..
1.9.3p194 :015 > s.scan(order_values_regexp).map(&:first)
=> ["users.id asc", " sum_fun(users.name, sum_other_fun(a, b, c), users.surname)"]

@vanyak
Copy link
Contributor Author

vanyak commented Jan 20, 2013

@joxxoxo looks like you are right. There are actually some more cases when it won't work. Unfortunately Ruby does not support variable-length lookbehind assertions so the only way would be to use split like you did.
But I'll give it another try..

@vanyak
Copy link
Contributor Author

vanyak commented Jan 27, 2013

Have to admit that trying further I came up with solution quite similar to the one @joxxoxo proposed.
Closing this one in favor of #8741

@vanyak vanyak closed this Jan 27, 2013
@carlosantoniodasilva
Copy link
Member

@vanyak alright, thanks for reporting back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants