Order by with expressions

[jrafanie]

This occurred while using the ancestry gem which creates scopes using case expressions in an order clause which works fine on postgres but fails on sqlserver due to the custom ordering code.

The second commit contains a failing test that is exacerbated by the fact that the custom ordering code splits on commas without regard for inline functions with parameters. We're not sure how to fix this particular test without rewriting the entire String conditional of the custom ordering code.

-- @jrafanie & @Fryguy

[metaskills]

A few thoughts on this.

We have an Arel visitor for Ordering which is deprecated and removed from Arel. The new Ascending/Descending should be used if you want to pass true Arel nodes down.

I am not sure how much longer ActiveRecord will allow and encourage dumping strings down to order to be visited. They should either be converted to Arel::Nodes::Ordering for us automatically or have some other logic solved higher up.

I have long told people they should be doing things like this no matter what in AR when the order is not a simple "column ASC" style.

order(Arel::Nodes::Ordering.new(Arel.sql('CASE WHEN scheduled_for IS NULL THEN 1 ELSE 0 END')))

This could totally be the fault of ActiveRecord for allowing such things.

[jrafanie]

We're confused as to how to proceed. Are you saying that the client, in this case the ancestry gem, should specify the order using the Arel::Nodes::Ordering notation and we should ignore the fact that ActiveRecord allows straight SQL? How do we handle future third party gems using the "wrong" approach? If this is the case, does the order method in the adapter go away or only the String leg of the case condition?

The SQL case statement in an order by works fine in the postgres adapter, so I would think it should work the same in the sqlserver adapter.

[metaskills]

I think I am just lamenting at a weak spot in ActiveRecord that should be doing the leg work for us. In all likelihood this will get merged in.

[workdreamer]

Hi i have a issue also on order by.

I want to Order with a case ex: ORDER BY ('CASE WHEN name = 'a' THEN 'immediate' WHEN name = 'b' THEN 'low-level' ELSE '') ASC

in my scope i build the string, the problem is this is scattered everywhere.

The problem is the adapater is droping me off the text, even if im putting the

 order(Arel::Nodes::Ordering.new(Arel.sql(my_builded_text)))

The adapter is cutting the case text. So, what can i do to avoid this? Thank you very much.

ex:

SELECT
    TOP(10)[__rnt].*
FROM
    (
        SELECT
            ROW_NUMBER()OVER(

                ORDER BY
                    (ASC)AS [__rn],
                    [development_reports].*
                FROM

[Fryguy]

I'm not sure if you typoed your example or not, but it's not valid SQL. Also, are you adding in the ORDER BY part as well? If so, then Arel is probably doubling that up on you, so you shouldn't do that.

Try this one:
(CASE WHEN name = 'a' THEN 'immediate' WHEN name = 'b' THEN 'low-level' ELSE '' END) ASC

[metaskills]

@Fryguy I think @workdreamer is reporting that the pagination window function is messing up the order, which could be the case. I'll have to take a look.

[workdreamer]

It is exactly when i want to do a pagination. Any solution? Thank you.

[jrafanie]

@metaskills, do you need anything else on this pull request to get it merged into master or at least a 3-1 master? If time is the issue, no problem. I'm just making sure you're not waiting on me.

[metaskills]

Working on the 3.2 adapter now. I will get to this soon, should be in the next few days.

[metaskills]

My first initial test run has this failure. I am starting to dig around but wanted to report back. Under 1.8.7 (REE) with DBLIB mode.

  1) Error:
test: Order by should support inline function with parameters. (OrderTestSqlserver):
ActiveRecord::StatementInvalid: TinyTds::Error: Incorrect syntax near the keyword 'ASC'.: EXEC sp_executesql N'SELECT TOP (1) [posts].* FROM [posts] ORDER BY SUBSTRING(title ASC, 1 ASC, 3) ASC'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:410:in `each'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:410:in `handle_to_names_and_values_dblib'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:399:in `handle_to_names_and_values'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:371:in `_raw_select'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:365:in `raw_select'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:365:in `raw_select'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:348:in `do_exec_query'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:24:in `exec_query'
    ./lib/active_record/connection_adapters/sqlserver/database_statements.rb:297:in `select'
    /Users/kencollins/Repositories/activerecord-sqlserver-adapter/test/cases/order_test_sqlserver.rb:99:in `__bind_1324990952_132541'

[workdreamer]

Thank you.

[metaskills]

Ah, reading your comment now about how this is known to fail. Let me think about that.

[workdreamer]

;) I really appreciate your feedback, it cheers me up.

[jrafanie]

Yeah, @metaskills. Order by with inline functions with multiple parameters was a known issue that our workaround fails to address. Since it wasn't a large problem for our needs and we couldn't find an uncomplicated solution, we left that as a known issue.

See our failing test: ManageIQ@1dfb775

[metaskills]

I will be pushing a 3.1.5 shortly. This commit is my changes.

55aa57f

A test with multiple functions with args/commas will still fail. But I think we can make the case that you may need to pass down an explicit ordering object at that point. Very good patch y'all. Thanks!