Skip to content

Conversation

@wbond
Copy link
Contributor

@wbond wbond commented May 14, 2014

Fixed selecting DISTINCT rows from a table when ordering by a column that is not part of the SELECT list where the join creates at least one extra row.

While previously the test suite combined with Rails 4.0.0 looked like it worked, the constructed SQL would return incorrect values for FinderTest#test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct. By using a GROUP BY and MAX()/MIN() in the ORDER BY with no DISTINCT call, the order of the returned rows was not necessarily correct.

With Rails 4.0.1, a related bug was fixed that introduces a new method, columns_for_distinct. Unfortunately by adding the columns from the ORDER BY clause to the SELECT list, the DISTINCT nature of the queries was altered. This combined with a test limiting the results to only 2 records, exposed the bug.

From testing various SQL constructs to solve the issue, I ended up determining that to properly return a single value from the related table used in the ORDER BY, a query with a subquery utilizing ROW_NUMBER() and DENSE_RANK() was needed. The ROW_NUMBER() was partitioned over the originally requested columns, and then the outer query would filter the results to just the first row of each partition. DENSE_RANK() was used to generate a proper ordering of the partitions in relation
to each other.

In the process I also determined that extra code, plus an extra test, was necessary to ensure that when such a construct was generated using both a limit and offset, that the results would be correct also.

See #306 for further discussion, plus example queries and results.

These changes have been tested against Rails:

  • 4.0.0
  • 4.0.1
  • 4.0.5

…that is not

part of the SELECT list where the join creates at least one extra row.

While previously the test suite combined with Rails 4.0.0 looked like it worked, the
constructed SQL would return incorrect values for
FinderTest#test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct.
By using a GROUP BY and MAX()/MIN() in the ORDER BY with no DISTINCT call, the order
of the returned rows was not necessarily correct.

With Rails 4.0.1, a related bug was fixed that introduces a new method,
columns_for_distinct. Unfortunately by adding the columns from the ORDER BY clause
to the SELECT list, the DISTINCT nature of the queries was altered. This combined
with a test limiting the results to only 2 records, exposed the bug.

From testing various SQL constructs to solve the issue, I ended up determining that
to properly return a single value from the related table used in the ORDER BY, a
query with a subquery utilizing ROW_NUMBER() and DENSE_RANK() was needed. The
ROW_NUMBER() was partitioned over the originally requested columns, and then the
outer query would filter the results to just the first row of each partition.
DENSE_RANK() was used to generate a proper ordering of the partitions in relation
to each other.

In the process I also determined that extra code, plus an extra test, was necessary
to ensure that when such a construct was generated using both a limit and offset,
that the results would be correct also.

See #306
for further discussion, plus example queries and results.
@wbond
Copy link
Contributor Author

wbond commented May 15, 2014

@annaswims @metaskills How do you two feel about me merging this and putting it in a branch named 4-0-stable? After that, I would move rails-4-1-0 to master.

@annaswims
Copy link
Contributor

@wbond You are AWESOME for doing this. The only thin I can think of that needs to be take care of before I'd say it's ready to be called stable is the stupid regex at the top of DatabaseStatements#exec_query, which I hope to take care of today.

annaswims added a commit that referenced this pull request May 15, 2014
Fix for Issue #306 - DISTINCT + TOP + ORDER BY Associated Table
@annaswims annaswims merged commit adfdf7e into rails-sqlserver:master May 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants