Incorrect SQL in Rails 4.0.1 when using Includes, Order and Limit

[annaswims]

FinderTest#test_coerced_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct does not pass

It generates different SQL in Rails 4.0.0 (good) and Rails 4.0.1-2 (failing).

Post.all.merge!(
      :includes => { :authors => :author_address }, 
      :order => 'author_addresses.id DESC ', 
      :limit => 2)

The SQL generated in Rails 4.0.0:

EXEC sp_executesql N'SELECT TOP (2) [posts].id FROM [posts] LEFT OUTER JOIN [categorizations] ON [categorizations].[category_id] = [posts].[id] LEFT OUTER JOIN [authors] ON [authors].[id] = [categorizations].[author_id] LEFT OUTER JOIN [author_addresses] ON [author_addresses].[id] = [authors].[author_address_id] GROUP BY [posts].id ORDER BY MAX(author_addresses.id)'

EXEC sp_executesql N'SELECT [posts].[id] AS t0_r0, [posts].[author_id] AS t0_r1, [posts].[title] AS t0_r2, [posts].[body] AS t0_r3, [posts].[type] AS t0_r4, [posts].[comments_count] AS t0_r5, [posts].[taggings_count] AS t0_r6, [posts].[taggings_with_delete_all_count] AS t0_r7, [posts].[taggings_with_destroy_count] AS t0_r8, [posts].[tags_count] AS t0_r9, [posts].[tags_with_destroy_count] AS t0_r10, [posts].[tags_with_nullify_count] AS t0_r11, [authors].[id] AS t1_r0, [authors].[name] AS t1_r1, [authors].[author_address_id] AS t1_r2, [authors].[author_address_extra_id] AS t1_r3, [authors].[organization_id] AS t1_r4, [authors].[owned_essay_id] AS t1_r5, [author_addresses].[id] AS t2_r0 FROM [posts] LEFT OUTER JOIN [categorizations] ON [categorizations].[category_id] = [posts].[id] LEFT OUTER JOIN [authors] ON [authors].[id] = [categorizations].[author_id] LEFT OUTER JOIN [author_addresses] ON [author_addresses].[id] = [authors].[author_address_id] WHERE [posts].[id] IN (3, 2) ORDER BY author_addresses.id DESC'

The SQL generated in Rails 4.0.1:

EXEC sp_executesql N'SELECT DISTINCT TOP (2) [posts].[id], author_addresses.id AS alias_0 FROM [posts] LEFT OUTER JOIN [categorizations] ON [categorizations].[category_id] = [posts].[id] LEFT OUTER JOIN [authors] ON [authors].[id] = [categorizations].[author_id] LEFT OUTER JOIN [author_addresses] ON [author_addresses].[id] = [authors].[author_address_id] ORDER BY author_addresses.id DESC'

EXEC sp_executesql N'SELECT [posts].[id] AS t0_r0, [posts].[author_id] AS t0_r1, [posts].[title] AS t0_r2, [posts].[body] AS t0_r3, [posts].[type] AS t0_r4, [posts].[comments_count] AS t0_r5, [posts].[taggings_count] AS t0_r6, [posts].[taggings_with_delete_all_count] AS t0_r7, [posts].[taggings_with_destroy_count] AS t0_r8, [posts].[tags_count] AS t0_r9, [posts].[tags_with_destroy_count] AS t0_r10, [posts].[tags_with_nullify_count] AS t0_r11, [authors].[id] AS t1_r0, [authors].[name] AS t1_r1, [authors].[author_address_id] AS t1_r2, [authors].[author_address_extra_id] AS t1_r3, [authors].[organization_id] AS t1_r4, [authors].[owned_essay_id] AS t1_r5, [author_addresses].[id] AS t2_r0 FROM [posts] LEFT OUTER JOIN [categorizations] ON [categorizations].[category_id] = [posts].[id] LEFT OUTER JOIN [authors] ON [authors].[id] = [categorizations].[author_id] LEFT OUTER JOIN [author_addresses] ON [author_addresses].[id] = [authors].[author_address_id] WHERE [posts].[id] IN (1, 1) ORDER BY author_addresses.id DESC'

It looks like this Arel pull request is where the problem originates.
rails/arel#211

[wbond]

I am seeing this test fail with 4.0.4, however, if I run the tests in isolation they run fine.

ACTIVERECORD_UNITTEST_HOST=win7-vm.local bundle exec rake test:dblib TEST_FILES=test/cases/finder_test_sqlserver.rb
/Users/wbond/.rbenv/versions/2.0.0-p247/bin/ruby -I"lib:test:/Users/wbond/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/bundler/gems/rails-2abe4b032d08/activerecord/test:/Users/wbond/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/gems/arel-4.0.2/test" -I"/Users/wbond/.rbenv/versions/2.0.0-p247/lib/ruby/2.0.0" "/Users/wbond/.rbenv/versions/2.0.0-p247/lib/ruby/2.0.0/rake/rake_test_loader.rb" "test/cases/sqlserver_helper.rb" "test/cases/finder_test_sqlserver.rb" 
Using dblib
Run options: --seed 12447

# Running tests:

....

Finished tests in 0.531390s, 7.5274 tests/s, 15.0549 assertions/s.

4 tests, 8 assertions, 0 failures, 0 errors, 0 skips
Coverage report generated for Unit Tests to /Users/wbond/dev/veracross/activerecord-sqlserver-adapter/coverage. 707 / 1243 LOC (56.88%) covered.

This makes me think there is another test that is changing the database and causing this test to break.

[wbond]

I've determined that adding , :categorizations to https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/test/cases/finder_test_sqlserver.rb#L13 causes the test to consistently fail.

[wbond]

Here is the formatted SQL that is generated with 4.0.0:

SELECT
    TOP (2)
    [posts].id
FROM
    [posts]
    LEFT OUTER JOIN [categorizations]
        ON [categorizations].[category_id] = [posts].[id]
    LEFT OUTER JOIN [authors]
        ON [authors].[id] = [categorizations].[author_id]
    LEFT OUTER JOIN [author_addresses]
        ON [author_addresses].[id] = [authors].[author_address_id]
GROUP BY
    [posts].id
ORDER BY
    MAX(author_addresses.id)

And here is 4.0.1:

SELECT
    DISTINCT TOP (2)
    [posts].[id],
    author_addresses.id AS alias_0
FROM
    [posts]
    LEFT OUTER JOIN [categorizations]
        ON [categorizations].[category_id] = [posts].[id]
    LEFT OUTER JOIN [authors]
        ON [authors].[id] = [categorizations].[author_id]
    LEFT OUTER JOIN [author_addresses]
        ON [author_addresses].[id] = [authors].[author_address_id]
ORDER BY
    author_addresses.id DESC

Those two SQL statements return the following results respectively:

| id |
+----+
| 3  |
| 2  |

| id | alias_0 |
+----+---------+
| 1  | 1       |
| 1  | NULL    |

So the issue is that SQL server is not returning a distinct set of values for
id, but the combination of id and the column used in ORDER BY. We will
need to alter how distinct statements are generated to fix this bug.

By comparison, here is the SQLite statement:

SELECT
    DISTINCT
    "posts".id
FROM
    "posts"
    LEFT OUTER JOIN "categorizations"
        ON "categorizations"."category_id" = "posts"."id"
    LEFT OUTER JOIN "authors"
        ON "authors"."id" = "categorizations"."author_id"
    LEFT OUTER JOIN "author_addresses"
        ON "author_addresses"."id" = "authors"."author_address_id"
ORDER BY
    author_addresses.id DESC
LIMIT 2

And it returns:

| id |
+----+
| 1  |
| 2  |

Once we remove the entries from the categorizations table, SQL Server returns
the same:

| id | alias_0 |
+----+---------+
| 1  | NULL    |
| 2  | NULL    |

[annaswims]

I've done some digging and this is what I know so far:
This marceloeloelo/rails@12ab37d#diff-1cabaf53664d448a5cbfcf396cd842b1 in activerecord 4.0.1 seems to be the root of the problem. When construct_limited_ids_condition changed from @klass.connection.distinct to @klass.connection.columns_for_distinct it no longer returns an Arel::SqlLiteral with 'DISTINCT'. That means that p1.include?('DISTINCT') is no longer true in Arel::Visitors::SQLServer#single_distinct_select_statement? (I think). Which means that the first if and elsif in Arel::Visitors::SQLServer#rowtable_projections aren't ever true (at least in any of the test suite).

So, basically, I know the problem but not the solution :)

[annaswims]

Oh, and this pull request seemed relevant too - rails/arel#211

[wbond]

Well, if determining the Arel object is a distinct statement were the only problem, then placing:

        sq = o.cores.first.set_quantifier
        return true if sq and sq.class.to_s =~ /Distinct/

at https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/lib/arel/visitors/sqlserver.rb#L162 would solve the problem. However the new columns_for_distinct is placing the extra author_addresses.id AS alias_0 in the GROUP BY, which is invalid SQL:

EXEC sp_executesql N'
    SELECT
        DISTINCT TOP (2)
        [posts].[id],
        author_addresses.id AS alias_0
    FROM
        [posts]
        LEFT OUTER JOIN [categorizations]
            ON [categorizations].[category_id] = [posts].[id]
        LEFT OUTER JOIN [authors]
            ON [authors].[id] = [categorizations].[author_id]
        LEFT OUTER JOIN [author_addresses]
            ON [author_addresses].[id] = [authors].[author_address_id]
    GROUP BY
        [posts].[id],
        author_addresses.id AS alias_0
    ORDER BY
        MAX(author_addresses.id)
'

All that said, the originally constructed SQL seems to return the wrong result (3, 2). Instead, it should be returning (1, 2). I believe the issue is that instead of generating the following like in 4.0.0:

SELECT
    TOP (2)
    [posts].id
FROM
    [posts]
    LEFT OUTER JOIN [categorizations]
        ON [categorizations].[category_id] = [posts].[id]
    LEFT OUTER JOIN [authors]
        ON [authors].[id] = [categorizations].[author_id]
    LEFT OUTER JOIN [author_addresses]
        ON [author_addresses].[id] = [authors].[author_address_id]
GROUP BY
    [posts].id
ORDER BY
    MAX(author_addresses.id)

or the following like now:

SELECT
    DISTINCT TOP (2)
    [posts].[id],
    author_addresses.id AS alias_0
FROM
    [posts]
    LEFT OUTER JOIN [categorizations]
        ON [categorizations].[category_id] = [posts].[id]
    LEFT OUTER JOIN [authors]
        ON [authors].[id] = [categorizations].[author_id]
    LEFT OUTER JOIN [author_addresses]
        ON [author_addresses].[id] = [authors].[author_address_id]
ORDER BY
    author_addresses.id DESC

we need to generate something with a subquery and row_number():

SELECT
    DISTINCT TOP (2)
    [__sq].[id]
FROM
    (
    SELECT
        [posts].[id],
        ROW_NUMBER() OVER (PARTITION BY [posts].[id] ORDER BY [author_addresses].[id]) AS [__row_num]
    FROM
        [posts]
        LEFT OUTER JOIN [categorizations]
            ON [categorizations].[category_id] = [posts].[id]
        LEFT OUTER JOIN [authors]
            ON [authors].[id] = [categorizations].[author_id]
        LEFT OUTER JOIN [author_addresses]
            ON [author_addresses].[id] = [authors].[author_address_id]
    ) AS [__sq]
WHERE
    [__row_num] = 1

[wbond]

Upon further investigation using authors.name as an example, I believe the following is the correct structure to return the correct Post records:

SELECT
    DISTINCT TOP(2)
    id,
    __order
FROM
    (
    SELECT
        [posts].id,
        DENSE_RANK() OVER (ORDER BY authors.name DESC) AS __order,
        ROW_NUMBER() OVER (PARTITION BY posts.id ORDER BY authors.name DESC) AS __row_num
    FROM
        [posts]
        LEFT OUTER JOIN [categorizations]
            ON [categorizations].[category_id] = [posts].[id]
        LEFT OUTER JOIN [authors]
            ON [authors].[id] = [categorizations].[author_id]
        LEFT OUTER JOIN [author_addresses]
            ON [author_addresses].[id] = [authors].[author_address_id]
    ) AS sq
WHERE
    __row_num = 1
ORDER BY
    __order

[wbond]

The Oracle adapter uses FIRST_VALUE() https://github.com/rsim/oracle-enhanced/blob/328037150c16d11e8e0eb9094d49562ae029b775/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb#L1393-L1407 to accomplish the appropriate ordering with distinct.

Unfortunately FIRST_VALUE() was only just introduced in SQL Server 2012.