Skip to content
This repository

unique has_many association with pagination breaks #209

Closed
wants to merge 2 commits into from

4 participants

Christoph Ritler Ken Collins Donald Ball Klaus
Christoph Ritler

We are facing a problem using the latest version (master) of sqlserver-adapter and will_paginate. Our model has a unique has_many association. Paginating this relation results in invalid sql.

The following example is completely fictional and makes no sense at all but it should illustrate the situation. I can't use the real code and I had to replace the sql-statements so they really don't mean anything.

class Article
   has_many(:comment_authors,
           :order => 'created_at')

  has_many(:collaborators,
           :class_name => 'User',
           :through => :comment_authors,
           :uniq => true)
end

accessing the relation works as expected:

Article.find(200).collaborators.to_sql 
# => SELECT DISTINCT [collaborators].* FROM [collaborators] INNER JOIN [comment_authors] ON [collaborators].[article_id] = [comment_authors].[article_id] WHERE [comment_authors].[article_id] = 237

when I add pagination it breaks:

Article.find(200).collaborators.paginate(:page => 1).to_sql 
# => SELECT TOP (15) [__rnt].* FROM ( SELECT ROW_NUMBER() OVER (ORDER BY [collaborators].[article_id] ASC) AS [__rn], DISTINCT [collaborators].* FROM [collaborators] INNER JOIN [tblTeam] ON [collaborators].[article_id] = [comment_authors].[article_id] WHERE [comment_authors].[article_id] = 237 ) AS [__rnt] WHERE [__rnt].[__rn] > (0) ORDER BY [__rnt].[__rn] ASC

The code above results in the following exception:

ActiveRecord::StatementInvalid: TinyTds::Error: Incorrect syntax near the keyword 'DISTINCT'.: EXEC sp_executesql N'SELECT TOP (15) [__rnt].* FROM ( SELECT ROW_NUMBER() OVER (ORDER BY [collaborators].[article_id] ASC) AS [__rn], DISTINCT [collaborators].* FROM [collaborators] INNER JOIN [comment_authors] ON [collaborators].[article_id] = [comment_authors].[article_id] WHERE [comment_authors].[article_id] = 237 ) AS [__rnt] WHERE [__rnt].[__rn] > (0) ORDER BY [__rnt].[__rn] ASC'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:412:in `each'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:412:in `handle_to_names_and_values_dblib'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:401:in `handle_to_names_and_values'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:373:in `_raw_select'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:367:in `block in raw_select'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/connection_adapters/abstract_adapter.rb:280:in `block in log'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activesupport-3.2.6/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/connection_adapters/abstract_adapter.rb:275:in `log'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:367:in `raw_select'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:350:in `do_exec_query'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:24:in `exec_query'
    from /Users/cr/git_repositories/github/activerecord-sqlserver-adapter/lib/active_record/connection_adapters/sqlserver/database_statements.rb:293:in `select'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:18:in `select_all'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/connection_adapters/abstract/query_cache.rb:63:in `select_all'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/querying.rb:38:in `block in find_by_sql'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/explain.rb:40:in `logging_query_plan'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/querying.rb:37:in `find_by_sql'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/relation.rb:171:in `exec_queries'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/relation.rb:160:in `block in to_a'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/explain.rb:33:in `logging_query_plan'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/relation.rb:159:in `to_a'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/will_paginate-3.0.3/lib/will_paginate/active_record.rb:107:in `block in to_a'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/will_paginate-3.0.3/lib/will_paginate/collection.rb:96:in `create'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/will_paginate-3.0.3/lib/will_paginate/active_record.rb:106:in `to_a'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/activerecord-3.2.6/lib/active_record/relation.rb:498:in `inspect'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/railties-3.2.6/lib/rails/commands/console.rb:47:in `start'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/railties-3.2.6/lib/rails/commands/console.rb:8:in `start'
    from /Users/cr/.rvm/gems/ruby-1.9.3-p194@sfv_webclient/gems/railties-3.2.6/lib/rails/commands.rb:41:in `<top (required)>'
    from script/rails:5:in `require'
    from script/rails:5:in `<main>'1.9.3p194 :016 > 
Christoph Ritler

Please do not merge this now. This pull-request is meant to be the base of a discussion.

I've fixed the Issue we had.
(invalid query with MODEL.offset(3).uniq -> distinct tablename.* in subquery).
I'v changed the windowed query generation, by adding the disctinct to the top level of the query and not the subquery.

But now there is an other issue occurring (see written test case). And i think there will be others!

Could you take a look at it?

Thanks!

Ken Collins
Collaborator

Hmmm, tricky. I am not sure I have my head completely in the issue, but I always wonder if pagination issues are better solved by adding an order since at the core, this is what SQL Server needs. All other databases allow you to query and get non-deterministic results with limit and offset, but we had to add this to our visitor, so we always put something in the order (like a primary key) if you have not added one.

https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/lib/arel/visitors/sqlserver.rb#L361

Does adding an explicit order help in your situation? If not, is it related to how ActiveRecord adds the distinct due to eager loading associations?

Christoph Ritler

Since I don't know enough MS-SQL to solve the situation myself I tried to execute the same query using sequel. It could generate a valid query and I think the result should be accurate:

activerecord-sqlserver-adapter

SELECT TOP (5) [__rnt].* FROM ( SELECT ROW_NUMBER() OVER (ORDER BY id ASC) AS [__rn], DISTINCT R* FROM [books] ) AS [__rnt] WHERE [__rnt].[__rn] > (1) ORDER BY [__rnt].[__rn] ASC

Sequel

SELECT TOP (5) * FROM (SELECT DISTINCT *, ROW_NUMBER() OVER (ORDER BY [ID]) AS [X_SEQUEL_ROW_NUMBER_X] FROM [BOOKS]) AS [T1] WHERE ([X_SEQUEL_ROW_NUMBER_X] > 10) ORDER BY [X_SEQUEL_ROW_NUMBER_X]

The sequel query is executable and should return the right result. I really don't have enough experience to change the Arel-visitor-code to fix the issue though.

I hope this helps.

Ken Collins metaskills closed this in da2131d July 07, 2012
Ken Collins
Collaborator

The ActiveRecord Author test model is my play/test ground for this issue. It has tons of associations to use for this issue. The ones I focused on are the categorized_posts and comments. When doing so, I used the following fixtures.

david = authors(:david)
mary = authors(:mary)

The has many categorized_posts which also has the same association with a uniq named unique_categorized_posts. Using Mary's fixtures, here are some data points. Notice how she has a duplcate categorization via this association. This makes it a good target for the pagination test.

mary.categorized_posts.all.map(&:id)                  # => [2, 2]
mary.unique_categorized_posts.all.map(&:id)           # => [2]
mary.unique_categorized_posts.limit(2).offset(0).all  # => BOOM! Currently blows up per ticket.

Here are some various #to_sql output for later discussion. The last window where DISTINCT [posts].* occurs is our problem target.

-- mary.categorized_posts.to_sql
SELECT [posts].* 
FROM [posts] 
INNER JOIN [categorizations] ON [posts].[id] = [categorizations].[post_id] 
WHERE [categorizations].[author_id] = 2

-- mary.unique_categorized_posts.to_sql
SELECT DISTINCT [posts].* 
FROM [posts] 
INNER JOIN [categorizations] ON [posts].[id] = [categorizations].[post_id] 
WHERE [categorizations].[author_id] = 2

-- mary.unique_categorized_posts.limit(2).offset(0).to_sql
SELECT TOP (2) [__rnt].* 
FROM ( 
  SELECT ROW_NUMBER() OVER (ORDER BY [posts].[id] ASC) AS [__rn], DISTINCT [posts].* 
  FROM [posts] 
  INNER JOIN [categorizations] ON [posts].[id] = [categorizations].[post_id] 
  WHERE [categorizations].[author_id] = 2 
) AS [__rnt] 
WHERE [__rnt].[__rn] > (0) 
ORDER BY [__rnt].[__rn] ASC

After doing some more reading on window and ranking functions in SQL Server, I decided to use the DENSE_RANK() function to simulate a DISTINCT * in the inner query. The following links were helpful.

http://stackoverflow.com/questions/2635665/row-number-vs-distinct
http://msdn.microsoft.com/en-us/library/ms173825

To me this seemed the simpliest solution for the narrow problem of DISTINCT in pagination per how Rails currently does associations. An alternate solution would be to alter the INNER JOIN as described in the following link, but raw SQL alteration is just ugly and brittle.

http://stackoverflow.com/questions/3266264/inner-join-distinct-id

So the problematic SQL would end up now looking like this below. This has the effect of removing duplicate rows and turning the rank into the new row number. After this change, all tests continue to pass along with a few more I wrote using the fixtures mentioned above.

-- mary.unique_categorized_posts.limit(2).offset(0).to_sql
SELECT TOP (2) [__rnt].* 
FROM ( 
  SELECT DISTINCT DENSE_RANK() OVER (ORDER BY [posts].[id] ASC) AS [__rn], [posts].* 
  FROM [posts] 
  INNER JOIN [categorizations] ON [posts].[id] = [categorizations].[post_id] 
  WHERE [categorizations].[author_id] = 2 
) AS [__rnt] 
WHERE [__rnt].[__rn] > (0) 
ORDER BY [__rnt].[__rn] ASC

I have published the 3.2.6 gem which now includes this fix.

Donald Ball
dball commented July 08, 2012

I haven't yet reviewed the code, but on paper, this looks like a correct solution. Nice one!

Klaus

I didn't know about the DENSE_RANK function too, but it seems to be supported since SQL 2005 and it seems to be created for exactly the situation we have here in this ticket.

So: well done, Ken

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

Showing 2 unique commits by 1 author.

Jun 22, 2012
Test written which reproduces uniq offset bug 3c16221
Windowed (limit and offset), distinct queries are correctly created 79842c0
This page is out of date. Refresh to see the latest.
4  lib/arel/visitors/sqlserver.rb
@@ -154,7 +154,7 @@ def visit_Arel_Nodes_SelectStatementWithOutOffset(o, windowed=false)
154 154
           projections = projections.map { |x| projection_without_expression(x) }
155 155
         end
156 156
         [ ("SELECT" if !windowed),
157  
-          (visit(core.set_quantifier) if core.set_quantifier),
  157
+          (visit(core.set_quantifier) if core.set_quantifier && !windowed),
158 158
           (visit(o.limit) if o.limit && !windowed),
159 159
           (projections.map{ |x| v = visit(x); v == "1" ? "1 AS [__wrp]" : v }.join(', ')),
160 160
           (source_with_lock_for_select_statement(o)),
@@ -168,7 +168,9 @@ def visit_Arel_Nodes_SelectStatementWithOutOffset(o, windowed=false)
168 168
       def visit_Arel_Nodes_SelectStatementWithOffset(o)
169 169
         o.limit ||= Arel::Nodes::Limit.new(9223372036854775807)
170 170
         orders = rowtable_orders(o)
  171
+        core = o.cores.first
171 172
         [ "SELECT",
  173
+          (visit(core.set_quantifier) if core.set_quantifier),
172 174
           (visit(o.limit) if o.limit && !windowed_single_distinct_select_statement?(o)),
173 175
           (rowtable_projections(o).map{ |x| visit(x) }.join(', ')),
174 176
           "FROM (",
9  test/cases/offset_and_limit_test_sqlserver.rb
@@ -88,10 +88,17 @@ class OffsetAndLimitTestSqlserver < ActiveRecord::TestCase
88 88
       end
89 89
 
90 90
     end
  91
+
  92
+    context 'with uniq selection' do
  93
+
  94
+      should 'add the distinct clause at the correct position' do
  95
+        assert_equal 5, Book.limit(5).offset(1).uniq.size
  96
+      end
  97
+
  98
+    end
91 99
     
92 100
   end
93 101
   
94  
-  
95 102
   protected
96 103
   
97 104
   def create_10_books
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.