Skip to content

Commit 72f0b3e

Browse files
aidanharanaidanharan
committed
Cleaning up
1 parent 10f1d3b commit 72f0b3e

File tree

3 files changed

+108
-140
lines changed

3 files changed

+108
-140
lines changed

‎lib/arel/visitors/sqlserver.rb

+6-19
Original file line numberDiff line numberDiff line change
@@ -302,23 +302,22 @@ def select_statement_lock?
302302
@select_statement && @select_statement.lock
303303
end
304304

305-
# FETCH cannot be used without an order. If an order is not given then try to use the projections for the ordering.
306-
# If no suitable projection are present then fallback to using the primary key of the table.
305+
# FETCH cannot be used without an order. If no order is given, then try to use the projections for the ordering.
306+
# If no suitable projection is present, then fallback to using the primary key of the table.
307307
def make_Fetch_Possible_And_Deterministic(o)
308-
# binding.pry if $DEBUG
309-
310308
return if o.limit.nil? && o.offset.nil?
311309
return if o.orders.any?
312310

313311
if any_groupings?(o) || has_non_table_join_sources?(o)
314-
if projection = projection_to_order_by_for_fetch(o)
312+
if (projection = projection_to_order_by_for_fetch(o))
315313
o.orders = [projection.asc]
316314
return
317315
end
318316
end
319317

320-
pk = primary_Key_From_Table(table_From_Statement(o))
321-
o.orders = [pk.asc] if pk
318+
if (pk = primary_Key_From_Table(table_From_Statement(o)))
319+
o.orders = [pk.asc]
320+
end
322321
end
323322

324323
def any_groupings?(o)
@@ -329,18 +328,6 @@ def has_non_table_join_sources?(o)
329328
o.cores.none? { |core| core.source.left.is_a?(Arel::Table) }
330329
end
331330

332-
# TODO: Need this for "in the subquery the first projection is used for ordering if none provided" test.
333-
# TODO: rename
334-
# def xxx_has_join_sources?(o)
335-
# # binding.pry if $DEBUG
336-
#
337-
# return true
338-
#
339-
# # return false unless o.is_a?(Arel::Nodes::SelectStatement)
340-
# #
341-
# # o.cores.any? { |core| core.source.is_a?(Arel::Nodes::JoinSource) }
342-
# end
343-
344331
# Find the first projection or part of projection that can be used for ordering. Cannot use
345332
# projections with '*' or '1 AS one' in them.
346333
def projection_to_order_by_for_fetch(o)

‎test/cases/implicit_order_test_sqlserver.rb

+102
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
# frozen_string_literal: true
2+
3+
require "cases/helper_sqlserver"
4+
require "models/post"
5+
require "models/company"
6+
7+
class ImplicitOrderTestSQLServer < ActiveRecord::TestCase
8+
9+
10+
describe "GROUP queries" do
11+
12+
it "order by primary key if not a GROUP query" do
13+
assert_queries_match(/#{Regexp.escape("ORDER BY [posts].[id] ASC")}/i) do
14+
Post.pick(:title)
15+
end
16+
end
17+
18+
it "ordering not required if not using FETCH" do
19+
assert_queries_match(/^#{Regexp.escape("SELECT count(*) FROM [posts] GROUP BY [posts].[title]")}$/i) do
20+
Post.group(:title).select("count(*)").load
21+
end
22+
end
23+
24+
it "error if using `first` without primary key projection (as `find_nth_with_limit` adds primary key ordering)" do
25+
error = assert_raises(ActiveRecord::StatementInvalid) do
26+
Post.select(:title, "count(*)").group(:title).first(2)
27+
end
28+
assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message)
29+
end
30+
31+
32+
it "using `first` with primary key projection (as `find_nth_with_limit` adds primary key ordering)" do
33+
assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title]")}/i) do
34+
Post.select(:title, "count(*)").group(:title).order(:title).first(2)
35+
end
36+
end
37+
end
38+
39+
40+
41+
# describe "simple query containing limit" do
42+
# it "order by primary key if no projections" do
43+
# sql = Post.limit(5).to_sql
44+
#
45+
# assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
46+
# end
47+
#
48+
# it "use order provided" do
49+
# sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql
50+
#
51+
# assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
52+
# end
53+
#
54+
# end
55+
#
56+
# describe "query containing FROM and limit" do
57+
# it "uses the provided orderings" do
58+
# sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
59+
#
60+
# assert_queries_match(/#{Regexp.escape(sql)}/) do
61+
# result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
62+
# assert_equal result, [11, 5, 1]
63+
# end
64+
# end
65+
#
66+
# it "in the subquery the first projection is used for ordering if none provided" do
67+
# sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
68+
#
69+
# # $DEBUG = true
70+
#
71+
# assert_queries_match(/#{Regexp.escape(sql)}/) do
72+
# result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
73+
# assert_equal result, [10, 5, 0]
74+
# end
75+
# end
76+
#
77+
# it "in the subquery the primary key is used for ordering if none provided" do
78+
# sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
79+
#
80+
# assert_queries_match(/#{Regexp.escape(sql)}/) do
81+
# result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
82+
# assert_equal result, [10, 5, 0]
83+
# end
84+
# end
85+
# end
86+
#
87+
#
88+
# it "generates correct SQL" do
89+
#
90+
# # $DEBUG = true
91+
#
92+
# sql = "SELECT [posts].[title], [posts].[id] FROM [posts] ORDER BY [posts].[id] ASC"
93+
#
94+
# assert_queries_match(/#{Regexp.escape(sql)}/) do
95+
# Post.select(posts: [:title, :id]).take
96+
# end
97+
#
98+
# # assert_match /#{Regexp.escape(sql)}/, Post.select(posts: [:bar, :id]).to_sql
99+
#
100+
# end
101+
102+
end

‎test/cases/order_test_sqlserver.rb

-121
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@
66
class OrderTestSQLServer < ActiveRecord::TestCase
77
fixtures :posts
88

9-
10-
119
it "not mangel complex order clauses" do
1210
xyz_order = "CASE WHEN [title] LIKE N'XYZ%' THEN 0 ELSE 1 END"
1311
xyz_post = Post.create title: "XYZ Post", body: "Test cased orders."
@@ -152,123 +150,4 @@ class OrderTestSQLServer < ActiveRecord::TestCase
152150
sql = Post.order(:id).order("posts.id ASC").to_sql
153151
assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql
154152
end
155-
156-
157-
describe "grouping queries" do
158-
159-
# it "order by primary key by default if not a grouping query" do
160-
#
161-
# $DEBUG = true
162-
#
163-
# assert_queries_match(/ORDER BY \[posts\]\.\[id\] ASC/i) do
164-
# Post.pick(:title)
165-
# end
166-
# end
167-
168-
169-
it "ordering not required if not using FETCH" do
170-
# skip
171-
172-
assert_queries_match(/^#{Regexp.escape("SELECT count(*) FROM [posts] GROUP BY [posts].[title]")}$/i) do
173-
Post.group(:title).select("count(*)").load
174-
end
175-
end
176-
177-
it "error thrown if using FETCH without ordering and column not in select" do
178-
# skip
179-
180-
error = assert_raises(ActiveRecord::StatementInvalid) do
181-
Post.select(:title, "count(*)").group(:title).first(2)
182-
end
183-
assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message)
184-
end
185-
186-
it "ordering required if using FETCH" do
187-
188-
# ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload|
189-
# puts payload[:sql]
190-
# end
191-
192-
# $DEBUG = true
193-
194-
# TODO: failing when it should not fail. The ordering should be taken from the projection.
195-
196-
# `first` calls `find_nth_with_limit` which adds ordering.
197-
198-
assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY")}/i) do
199-
# error = assert_raises(ActiveRecord::StatementInvalid) do
200-
Post.select(:title, "count(*)").group(:title).order(:title).first(2)
201-
end
202-
203-
# assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because/, error.message)
204-
205-
# Post.group(:title).select("count(*)").first(2).load
206-
207-
# assert_match(/Column "posts\.id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause/, error.message)
208-
end
209-
210-
211-
#
212-
# it "order by primary key by default if not a grouping query" do
213-
# $DEBUG = true
214-
#
215-
# ActiveSupport::Notifications.subscribe('sql.active_record') do |_name, _start, _finish, _id, payload|
216-
# puts payload[:sql]
217-
# end
218-
#
219-
# Post.group(:title).select("count(*)").first(2).load
220-
# end
221-
222-
end
223-
224-
225-
226-
describe "simple query containing limit" do
227-
it "order by primary key if no projections" do
228-
sql = Post.limit(5).to_sql
229-
230-
assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
231-
end
232-
233-
it "use order provided" do
234-
sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql
235-
236-
assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql
237-
end
238-
239-
240-
241-
242-
end
243-
244-
describe "query containing FROM and limit" do
245-
it "uses the provided orderings" do
246-
sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
247-
248-
assert_queries_match(/#{Regexp.escape(sql)}/) do
249-
result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
250-
assert_equal result, [11, 5, 1]
251-
end
252-
end
253-
254-
it "in the subquery the first projection is used for ordering if none provided" do
255-
sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
256-
257-
# $DEBUG = true
258-
259-
assert_queries_match(/#{Regexp.escape(sql)}/) do
260-
result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
261-
assert_equal result, [10, 5, 0]
262-
end
263-
end
264-
265-
it "in the subquery the primary key is used for ordering if none provided" do
266-
sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY"
267-
268-
assert_queries_match(/#{Regexp.escape(sql)}/) do
269-
result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)"))
270-
assert_equal result, [10, 5, 0]
271-
end
272-
end
273-
end
274153
end

0 commit comments

Comments
 (0)