Skip to content

Commit f5e0c39

Browse files
committed
Find and fix uncorrelated joins. https://gist.github.com/780158
1 parent e8d09d7 commit f5e0c39

File tree

2 files changed

+62
-40
lines changed

2 files changed

+62
-40
lines changed

lib/arel/visitors/sqlserver.rb

Lines changed: 48 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -78,30 +78,42 @@ def visit_Arel_Nodes_LockWithSQLServer o
7878
# SQLServer ToSql/Visitor (Additions)
7979

8080
def visit_Arel_Nodes_SelectStatementWithOutOffset(o, windowed=false)
81+
find_and_fix_uncorrelated_joins_in_select_statement(o)
8182
core = o.cores.first
8283
projections = core.projections
84+
groups = core.groups
85+
orders = o.orders
8386
if windowed && !function_select_statement?(o)
8487
projections = projections.map { |x| projection_without_expression(x) }
85-
elsif eager_limiting_select?(o)
86-
# TODO visit_Arel_Nodes_SelectStatementWithOutOffset - eager_limiting_select
87-
raise 'visit_Arel_Nodes_SelectStatementWithOutOffset - eager_limiting_select'
88+
elsif eager_limiting_select_statement?(o)
89+
raise "visit_Arel_Nodes_SelectStatementWithOutOffset - eager_limiting_select_statement?"
90+
groups = projections.map { |x| projection_without_expression(x) }
91+
projections = projections.map { |x| projection_without_expression(x) }
92+
# TODO: Let's alter objects vs strings and make new order objects
93+
orders = orders.map do |x|
94+
Arel::SqlLiteral.new(x.split(',').reject(&:blank?).map do |c|
95+
max = c =~ /desc\s*/i
96+
c = clause_without_expression(c).sub(/(asc|desc)/i,'').strip
97+
max ? "MAX(#{c})" : "MIN(#{c})"
98+
end.join(', '))
99+
end
88100
end
89101
[ ("SELECT" if !windowed),
90102
(visit(o.limit) if o.limit && !windowed),
91103
(projections.map{ |x| visit(x) }.join(', ')),
92104
visit(core.source),
93105
(visit(o.lock) if o.lock),
94106
("WHERE #{core.wheres.map{ |x| visit(x) }.join ' AND ' }" unless core.wheres.empty?),
95-
("GROUP BY #{core.groups.map { |x| visit x }.join ', ' }" unless core.groups.empty?),
107+
("GROUP BY #{groups.map { |x| visit x }.join ', ' }" unless groups.empty?),
96108
(visit(core.having) if core.having),
97-
("ORDER BY #{o.orders.map{ |x| visit(x) }.join(', ')}" if !o.orders.empty? && !windowed)
109+
("ORDER BY #{orders.map{ |x| visit(x) }.join(', ')}" if !orders.empty? && !windowed)
98110
].compact.join ' '
99111
end
100112

101113
def visit_Arel_Nodes_SelectStatementWithOffset(o)
102114
orders = rowtable_orders(o)
103115
[ "SELECT",
104-
(visit(o.limit) if o.limit && !single_distinct_select?(o)),
116+
(visit(o.limit) if o.limit && !single_distinct_select_statement?(o)),
105117
(rowtable_projections(o).map{ |x| visit(x) }.join(', ')),
106118
"FROM (",
107119
"SELECT ROW_NUMBER() OVER (ORDER BY #{orders.map{ |x| visit(x) }.uniq.join(', ')}) AS [__rn],",
@@ -115,7 +127,7 @@ def visit_Arel_Nodes_SelectStatementForComplexCount(o)
115127
# joins = correlated_safe_joins
116128
core = o.cores.first
117129
orders = rowtable_orders(o)
118-
o.limit.expr = o.limit.expr + o.offset.expr if o.limit
130+
o.limit.expr = o.limit.expr + (o.offset ? o.offset.expr : 0) if o.limit
119131
[ "SELECT COUNT([count]) AS [count_id]",
120132
"FROM (",
121133
"SELECT",
@@ -140,36 +152,54 @@ def table_name_from_select_statement(o)
140152
o.cores.first.source.left.name
141153
end
142154

143-
def single_distinct_select?(o)
155+
def single_distinct_select_statement?(o)
144156
projections = o.cores.first.projections
145-
projections.size == 1 && projections.first.include?('DISTINCT')
157+
first_prjn = projections.first
158+
projections.size == 1 &&
159+
((first_prjn.respond_to?(:distinct) && first_prjn.distinct) || first_prjn.include?('DISTINCT'))
146160
end
147161

148162
def function_select_statement?(o)
149163
core = o.cores.first
150164
core.projections.any? { |x| Arel::Nodes::Function === x }
151165
end
152166

153-
def eager_limiting_select?(o)
154-
false
155-
# single_distinct_select?(o) && taken_only? && relation.group_clauses.blank?
167+
def eager_limiting_select_statement?(o)
168+
core = o.cores.first
169+
single_distinct_select_statement?(o) && (o.limit && !o.offset) && core.groups.empty?
170+
end
171+
172+
def join_in_select_statement?(o)
173+
core = o.cores.first
174+
core.source.right.any? { |x| Arel::Nodes::Join === x }
156175
end
157176

158177
def complex_count_sql?(o)
159178
core = o.cores.first
160179
core.projections.size == 1 &&
161180
Arel::Nodes::Count === core.projections.first &&
162181
(o.limit || !core.wheres.empty?) &&
163-
true # TODO: This was - relation.joins(self).blank?
164-
# Consider visit(core.source)
165-
# Consider core.from
166-
# Consider core.froms
182+
!join_in_select_statement?(o)
183+
end
184+
185+
def find_and_fix_uncorrelated_joins_in_select_statement(o)
186+
core = o.cores.first
187+
return if !join_in_select_statement?(o) || core.source.right.size != 2
188+
j1 = core.source.right.first
189+
j2 = core.source.right.second
190+
return unless Arel::Nodes::OuterJoin === j1 && Arel::Nodes::StringJoin === j2
191+
j1_tn = j1.left.name
192+
j2_tn = j2.left.match(/JOIN \[(.*)\].*ON/).try(:[],1)
193+
return unless j1_tn == j2_tn
194+
crltd_tn = "#{j1_tn}_crltd"
195+
j1.left.table_alias = crltd_tn
196+
j1.right.expr.left.relation.table_alias = crltd_tn
167197
end
168198

169199
def rowtable_projections(o)
170200
core = o.cores.first
171-
if single_distinct_select?(o)
172-
raise 'TODO: single_distinct_select'
201+
if single_distinct_select_statement?(o)
202+
raise 'TODO: single_distinct_select_statement'
173203
# ::Array.wrap(relation.select_clauses.first.dup.tap do |sc|
174204
# sc.sub! 'DISTINCT', "DISTINCT #{taken_clause if relation.taken.present?}".strip
175205
# sc.sub! table_name_from_select_clause(sc), '__rnt'
Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,28 @@
11
require 'cases/sqlserver_helper'
2+
require 'models/tag'
3+
require 'models/tagging'
24
require 'models/post'
5+
require 'models/item'
6+
require 'models/comment'
37
require 'models/author'
4-
require 'models/project'
5-
require 'models/developer'
8+
require 'models/category'
9+
require 'models/categorization'
10+
require 'models/vertex'
11+
require 'models/edge'
12+
require 'models/book'
13+
require 'models/citation'
614

715
class ScratchTestSqlserver < ActiveRecord::TestCase
816

9-
fixtures :posts, :authors, :projects, :developers
17+
self.use_transactional_fixtures = false
1018

19+
fixtures :posts, :authors, :categories, :categorizations, :comments, :tags, :taggings,
20+
:author_favorites, :vertices, :items, :books, :edges
1121

1222
should 'pass' do
13-
jack = Author.new :name => "Jack"
14-
post = jack.posts_with_callbacks.build :title => "Call me back!", :body => "Before you wake up and after you sleep"
15-
16-
callback_log = ["before_adding<new>", "after_adding#{jack.posts_with_callbacks.first.id}"]
17-
assert_equal callback_log, jack.post_log
18-
assert jack.save
19-
20-
# SELECT COUNT([count]) AS [count_id]
21-
# FROM (
22-
# SELECT ROW_NUMBER() OVER (ORDER BY [posts].[id] ASC) AS [__rn],
23-
# 1 AS [count]
24-
# FROM [posts]
25-
# WHERE ([posts].author_id = 3)
26-
# ) AS [__rnt] NULL
27-
28-
assert_equal 1, jack.posts_with_callbacks.count
29-
assert_equal callback_log, jack.post_log
23+
assert_equal 1, posts(:welcome).tags.count
3024
end
3125

3226

33-
34-
3527
end
3628

0 commit comments

Comments
 (0)