From d295b2e18cb1581fc67be57756a82d26642952fd Mon Sep 17 00:00:00 2001 From: Ken Collins Date: Tue, 3 Aug 2010 00:02:05 -0400 Subject: [PATCH] [Rails3] Fix scoped counts. --- RAILS3_NOTES | 6 --- .../sql/compilers/sqlserver_compiler.rb | 39 ++++++++++++++++++- test/cases/offset_and_limit_test_sqlserver.rb | 34 ++++++++++------ .../pessimistic_locking_test_sqlserver.rb | 10 ++--- 4 files changed, 64 insertions(+), 25 deletions(-) diff --git a/RAILS3_NOTES b/RAILS3_NOTES index 1e620820a..8625c2612 100644 --- a/RAILS3_NOTES +++ b/RAILS3_NOTES @@ -65,12 +65,6 @@ = Arel Todo - 338 examples, 0 failures, 3 pending - - * unit/relations - • skip_spec.rb (try adding more, maybe with order) - • take_spec.rb (try adding more, maybe with order) - * Review/remove our long winded add_limit_offset! diff --git a/lib/arel/engines/sql/compilers/sqlserver_compiler.rb b/lib/arel/engines/sql/compilers/sqlserver_compiler.rb index 8cdaa5433..05293c983 100644 --- a/lib/arel/engines/sql/compilers/sqlserver_compiler.rb +++ b/lib/arel/engines/sql/compilers/sqlserver_compiler.rb @@ -12,7 +12,13 @@ module SqlCompiler class SQLServerCompiler < GenericCompiler def select_sql - relation.skipped ? select_sql_with_skipped : select_sql_without_skipped + if complex_count_sql? + select_sql_with_complex_count + elsif relation.skipped + select_sql_with_skipped + else + select_sql_without_skipped + end end def delete_sql @@ -25,6 +31,12 @@ def delete_sql protected + def complex_count_sql? + projections = relation.projections + Count === projections.first && projections.size == 1 && + (relation.taken.present? || relation.wheres.present?) && relation.joins(self).blank? + end + def taken_only? relation.taken.present? && relation.skipped.blank? end @@ -43,6 +55,31 @@ def all_select_clauses_aliased? end end + def select_sql_with_complex_count + joins = relation.joins(self) + wheres = relation.where_clauses + groups = relation.group_clauses + havings = relation.having_clauses + orders = relation.order_clauses + taken = relation.taken.to_i + skipped = relation.skipped.to_i + top_clause = "TOP (#{taken+skipped}) " if relation.taken.present? + build_query \ + "SELECT COUNT([count]) AS [count_id]", + "FROM (", + "SELECT #{top_clause}ROW_NUMBER() OVER (ORDER BY #{rowtable_order_clauses.join(', ')}) AS [rn],", + "1 AS [count]", + "FROM #{relation.from_clauses}", + (locked unless locked.blank?), + (joins unless joins.blank?), + ("WHERE #{wheres.join(' AND ')}" unless wheres.blank?), + ("GROUP BY #{groups.join(', ')}" unless groups.blank?), + ("HAVING #{havings.join(' AND ')}" unless havings.blank?), + ("ORDER BY #{orders.join(', ')}" unless orders.blank?), + ") AS [_rnt]", + "WHERE [_rnt].[rn] > #{relation.skipped.to_i}" + end + def select_sql_without_skipped(windowed=false) selects = relation.select_clauses joins = relation.joins(self) diff --git a/test/cases/offset_and_limit_test_sqlserver.rb b/test/cases/offset_and_limit_test_sqlserver.rb index 58648ff2c..c982bdd28 100644 --- a/test/cases/offset_and_limit_test_sqlserver.rb +++ b/test/cases/offset_and_limit_test_sqlserver.rb @@ -1,9 +1,10 @@ require 'cases/sqlserver_helper' require 'models/book' +require 'models/post' class OffsetAndLimitTestSqlserver < ActiveRecord::TestCase - class Account < ActiveRecord::Base; end + fixtures :posts setup :create_10_books teardown :destroy_all_books @@ -49,19 +50,27 @@ class Account < ActiveRecord::Base; end assert_sql(sql) { Book.limit(3).offset(5).all } end - should_eventually 'add locks to deepest sub select in limit offset sql that has a limited tally' do - options = { :limit => 3, :offset => 5, :lock => 'WITH (NOLOCK)' } - select_sql = 'SELECT * FROM books' - expected_sql = "SELECT * FROM (SELECT TOP 3 * FROM (SELECT TOP 8 * FROM books WITH (NOLOCK)) AS tmp1) AS tmp2" - @connection.add_limit_offset! select_sql, options - assert_equal expected_sql, @connection.add_lock!(select_sql,options) + should 'add locks to deepest sub select' do + pattern = /FROM \[books\] WITH \(NOLOCK\)/ + assert_sql(pattern) { Book.all :limit => 3, :offset => 5, :lock => 'WITH (NOLOCK)' } + assert_sql(pattern) { Book.count :limit => 3, :offset => 5, :lock => 'WITH (NOLOCK)' } end - should_eventually 'not create invalid SQL with subquery SELECTs with TOP' do - options = { :limit => 5, :offset => 1 } - subquery_select_sql = 'SELECT *, (SELECT TOP (1) [id] FROM [books]) AS [book_id] FROM [books]' - expected_sql = "SELECT * FROM (SELECT TOP 5 * FROM (SELECT TOP 6 *, (SELECT TOP 1 id FROM books) AS book_id FROM books) AS tmp1) AS tmp2" - assert_equal expected_sql, @connection.add_limit_offset!(subquery_select_sql,options) + context 'with count' do + + should 'pass a gauntlet of window tests' do + assert_equal 7, Post.count + assert_equal 1, Post.limit(1).offset(1).size + assert_equal 1, Post.limit(1).offset(5).size + assert_equal 1, Post.limit(1).offset(6).size + assert_equal 0, Post.limit(1).offset(7).size + assert_equal 3, Post.limit(3).offset(4).size + assert_equal 2, Post.limit(3).offset(5).size + assert_equal 1, Post.limit(3).offset(6).size + assert_equal 0, Post.limit(3).offset(7).size + assert_equal 0, Post.limit(3).offset(8).size + end + end end @@ -70,6 +79,7 @@ class Account < ActiveRecord::Base; end protected def create_10_books + Book.delete_all @books = (1..10).map {|i| Book.create!} end diff --git a/test/cases/pessimistic_locking_test_sqlserver.rb b/test/cases/pessimistic_locking_test_sqlserver.rb index 153f14b25..aae65e9fa 100644 --- a/test/cases/pessimistic_locking_test_sqlserver.rb +++ b/test/cases/pessimistic_locking_test_sqlserver.rb @@ -64,12 +64,10 @@ def setup 20.times { |n| Person.create!(:first_name => "Thing_#{n}") } end - should_eventually 'cope with un-locked paginated results' do - tally_not_locked = %r|SELECT count\(\*\) as TotalRows from \(SELECT TOP 1000000000 \* FROM \[people\]\s+WITH \(NOLOCK\) \) tally| - inner_tmp_not_locked = %r|SELECT TOP 15 \* FROM \[people\] WITH \(NOLOCK\)| - # Currently association limiting is not locked like the parent. - association_limiting_not_locked = %r|SELECT \[readers\]\.\* FROM \[readers\] WITH \(NOLOCK\) WHERE \(\[readers\]\.person_id IN \(1,2,3,4,5\)\)| - assert_sql(tally_not_locked,inner_tmp_not_locked) do + should 'cope with eager loading un-locked paginated' do + eager_ids_sql = /SELECT DISTINCT TOP \(5\).*FROM \[people\] WITH \(NOLOCK\)/ + loader_sql = /FROM \[people\] WITH \(NOLOCK\).*WHERE \(\[people\].\[id\] IN/ + assert_sql(eager_ids_sql,loader_sql) do Person.all(:include => :readers, :lock => 'WITH (NOLOCK)', :limit => 5, :offset => 10) end end