Skip to content

Commit 6c9aa73

Browse files
committed
[Rails3] Note SQL Server specific cases. Experiment with changing limit/offset.
1 parent 3302818 commit 6c9aa73

File tree

3 files changed

+84
-63
lines changed

3 files changed

+84
-63
lines changed

RAILS3_NOTES

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,16 @@ on a local branch of our remote tracking branch.
3636

3737
2384 tests, 9524 assertions, 30 failures, 37 errors
3838

39+
* SQL Server Test Cases
40+
x adapter_test_sqlserver.rb (8) Limit offset stuff.
41+
x eager_association_test_sqlserver.rb (2) Limit offset stuff.
42+
x inheritance_test_sqlserver.rb (1) Recheck orig.
43+
x method_scoping_test_sqlserver.rb (1) Recheck orig.
44+
√ named_scope_test_sqlserver.rb (0) Recheck orig.
45+
x offset_and_limit_test_sqlserver.rb (3) Offset/Limit stuff.
46+
x pessimistic_locking_test_sqlserver.rb (7) Tons of lock problems.
47+
x table_name_test_sqlserver.rb (1) Query pattern.
48+
3949
* Verify coerced tests
4050
Undefined coerced test: AdapterTest#test_add_limit_offset_should_sanitize_sql_injection_for_limit_without_comas
4151
Undefined coerced test: AdapterTest#test_add_limit_offset_should_sanitize_sql_injection_for_limit_with_comas
@@ -62,7 +72,7 @@ on a local branch of our remote tracking branch.
6272

6373
= Arel Todo
6474

65-
338 examples, 6 failures, 3 pending
75+
338 examples, 4 failures, 3 pending
6676

6777
* integration/joins/
6878
√ with_adjacency_spec.rb

lib/active_record/connection_adapters/sqlserver/database_statements.rb

Lines changed: 70 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -43,67 +43,77 @@ def rollback_to_savepoint
4343
end
4444

4545
def add_limit_offset!(sql, options)
46-
# Validate and/or convert integers for :limit and :offets options.
47-
if options[:offset]
48-
raise ArgumentError, "offset should have a limit" unless options[:limit]
49-
unless options[:offset].kind_of?(Integer)
50-
if options[:offset] =~ /^\d+$/
51-
options[:offset] = options[:offset].to_i
52-
else
53-
raise ArgumentError, "offset should be an integer"
54-
end
55-
end
56-
end
57-
if options[:limit] && !(options[:limit].kind_of?(Integer))
58-
if options[:limit] =~ /^\d+$/
59-
options[:limit] = options[:limit].to_i
60-
else
61-
raise ArgumentError, "limit should be an integer"
62-
end
63-
end
64-
# The business of adding limit/offset
65-
if options[:limit] and options[:offset]
66-
tally_sql = "SELECT count(*) as TotalRows from (#{sql.sub(/\bSELECT(\s+DISTINCT)?\b/i, "SELECT#{$1} TOP 1000000000")}) tally"
67-
add_lock! tally_sql, options
68-
total_rows = select_value(tally_sql).to_i
69-
if (options[:limit] + options[:offset]) >= total_rows
70-
options[:limit] = (total_rows - options[:offset] >= 0) ? (total_rows - options[:offset]) : 0
71-
end
72-
# Make sure we do not need a special limit/offset for association limiting. http://gist.github.com/25118
73-
add_limit_offset_for_association_limiting!(sql,options) and return if sql_for_association_limiting?(sql)
74-
# Wrap the SQL query in a bunch of outer SQL queries that emulate proper LIMIT,OFFSET support.
75-
sql.sub!(/^\s*SELECT(\s+DISTINCT)?/i, "SELECT * FROM (SELECT TOP #{options[:limit]} * FROM (SELECT#{$1} TOP #{options[:limit] + options[:offset]}")
76-
sql << ") AS tmp1"
77-
if options[:order]
78-
order = options[:order].split(',').map do |field|
79-
order_by_column, order_direction = field.split(" ")
80-
order_by_column = quote_column_name(order_by_column)
81-
# Investigate the SQL query to figure out if the order_by_column has been renamed.
82-
if sql =~ /#{Regexp.escape(order_by_column)} AS (t\d+_r\d+)/
83-
# Fx "[foo].[bar] AS t4_r2" was found in the SQL. Use the column alias (ie 't4_r2') for the subsequent orderings
84-
order_by_column = $1
85-
elsif order_by_column =~ /\w+\.\[?(\w+)\]?/
86-
order_by_column = $1
87-
else
88-
# It doesn't appear that the column name has been renamed as part of the query. Use just the column
89-
# name rather than the full identifier for the outer queries.
90-
order_by_column = order_by_column.split('.').last
91-
end
92-
# Put the column name and eventual direction back together
93-
[order_by_column, order_direction].join(' ').strip
94-
end.join(', ')
95-
sql << " ORDER BY #{change_order_direction(order)}) AS tmp2 ORDER BY #{order}"
96-
else
97-
sql << ") AS tmp2"
98-
end
99-
elsif options[:limit] && sql !~ /^\s*SELECT (@@|COUNT\()/i
100-
if md = sql.match(/^(\s*SELECT)(\s+DISTINCT)?(.*)/im)
101-
sql.replace "#{md[1]}#{md[2]} TOP #{options[:limit]}#{md[3]}"
102-
else
103-
# Account for building SQL fragments without SELECT yet. See #update_all and #limited_update_conditions.
104-
sql.replace "TOP #{options[:limit]} #{sql}"
105-
end
46+
limit = options[:limit]
47+
offset = options[:offset]
48+
49+
50+
if limit && !offset
51+
sql
52+
elsif limit || offset
53+
sql
10654
end
55+
56+
# options[:offset] ||= 0
57+
# if options[:offset] > 0
58+
# options[:order] ||= if order_by = sql.match(/ORDER BY (.*$)/i)
59+
# order_by[1]
60+
# else
61+
# table_name = sql.match('FROM ([\[\]a-zA-Z0-9_\.]+)')[1]
62+
#
63+
# find_table_primary_key_columns(table_name)
64+
# end
65+
# sql.sub!(/ORDER BY.*$/i, '')
66+
# sql.sub!(/SELECT/i, "SELECT * FROM ( SELECT ROW_NUMBER() OVER( ORDER BY #{options[:order] } ) AS row_num, ")
67+
# sql << ") AS t WHERE row_num > #{options[:offset]}"
68+
# end
69+
# sql.sub!(/^SELECT/i, "SELECT TOP #{options[:limit]}") if options[:limit]
70+
# sql
71+
72+
73+
74+
# # The business of adding limit/offset
75+
# if options[:limit] and options[:offset]
76+
# tally_sql = "SELECT count(*) as TotalRows from (#{sql.sub(/\bSELECT(\s+DISTINCT)?\b/i, "SELECT#{$1} TOP 1000000000")}) tally"
77+
# add_lock! tally_sql, options
78+
# total_rows = select_value(tally_sql).to_i
79+
# if (options[:limit] + options[:offset]) >= total_rows
80+
# options[:limit] = (total_rows - options[:offset] >= 0) ? (total_rows - options[:offset]) : 0
81+
# end
82+
# # Make sure we do not need a special limit/offset for association limiting. http://gist.github.com/25118
83+
# add_limit_offset_for_association_limiting!(sql,options) and return if sql_for_association_limiting?(sql)
84+
# # Wrap the SQL query in a bunch of outer SQL queries that emulate proper LIMIT,OFFSET support.
85+
# sql.sub!(/^\s*SELECT(\s+DISTINCT)?/i, "SELECT * FROM (SELECT TOP #{options[:limit]} * FROM (SELECT#{$1} TOP #{options[:limit] + options[:offset]}")
86+
# sql << ") AS tmp1"
87+
# if options[:order]
88+
# order = options[:order].split(',').map do |field|
89+
# order_by_column, order_direction = field.split(" ")
90+
# order_by_column = quote_column_name(order_by_column)
91+
# # Investigate the SQL query to figure out if the order_by_column has been renamed.
92+
# if sql =~ /#{Regexp.escape(order_by_column)} AS (t\d+_r\d+)/
93+
# # Fx "[foo].[bar] AS t4_r2" was found in the SQL. Use the column alias (ie 't4_r2') for the subsequent orderings
94+
# order_by_column = $1
95+
# elsif order_by_column =~ /\w+\.\[?(\w+)\]?/
96+
# order_by_column = $1
97+
# else
98+
# # It doesn't appear that the column name has been renamed as part of the query. Use just the column
99+
# # name rather than the full identifier for the outer queries.
100+
# order_by_column = order_by_column.split('.').last
101+
# end
102+
# # Put the column name and eventual direction back together
103+
# [order_by_column, order_direction].join(' ').strip
104+
# end.join(', ')
105+
# sql << " ORDER BY #{change_order_direction(order)}) AS tmp2 ORDER BY #{order}"
106+
# else
107+
# sql << ") AS tmp2"
108+
# end
109+
# elsif options[:limit] && sql !~ /^\s*SELECT (@@|COUNT\()/i
110+
# if md = sql.match(/^(\s*SELECT)(\s+DISTINCT)?(.*)/im)
111+
# sql.replace "#{md[1]}#{md[2]} TOP #{options[:limit]}#{md[3]}"
112+
# else
113+
# # Account for building SQL fragments without SELECT yet. See #update_all and #limited_update_conditions.
114+
# sql.replace "TOP #{options[:limit]} #{sql}"
115+
# end
116+
# end
107117
end
108118

109119
def empty_insert_statement_value

test/cases/method_scoping_test_sqlserver.rb

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,11 @@ class NestedScopingTest < ActiveRecord::TestCase
1212

1313
fixtures :developers
1414

15+
1516
def test_coerced_test_merged_scoped_find
1617
poor_jamis = developers(:poor_jamis)
17-
Developer.with_scope(:find => { :conditions => "salary < 100000" }) do
18-
Developer.with_scope(:find => { :offset => 1, :order => 'id asc' }) do
18+
Developer.send(:with_scope, :find => { :conditions => "salary < 100000" }) do
19+
Developer.send(:with_scope, :find => { :offset => 1, :order => 'id asc' }) do
1920
assert_sql /ORDER BY id ASC/ do
2021
assert_equal(poor_jamis, Developer.find(:first, :order => 'id asc'))
2122
end

0 commit comments

Comments
 (0)