Skip to content

Commit bfcc13a

Browse files
committed
Arel: make Or nodes "Nary" like And
Fix: rails#51386 This significantly reduce the depth of the tree for large `OR` conditions. I was initially a bit on the fence about that fix, but given that `And` is already implemented this way, I see no reasons not to do the same. Amusingly, the reported repro script now makes SQLite fail: ```ruby SQLite3::SQLException: Expression tree is too large (maximum depth 1000) ```
1 parent d4c40b6 commit bfcc13a

File tree

15 files changed

+37
-36
lines changed

15 files changed

+37
-36
lines changed

activerecord/lib/active_record/relation/predicate_builder.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ def grouping_queries(queries)
142142
queries.first
143143
else
144144
queries.map! { |query| query.reduce(&:and) }
145-
queries = queries.reduce { |result, query| Arel::Nodes::Or.new(result, query) }
145+
queries = queries.reduce { |result, query| Arel::Nodes::Or.new([result, query]) }
146146
Arel::Nodes::Grouping.new(queries)
147147
end
148148
end

activerecord/lib/active_record/relation/query_methods.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1098,7 +1098,7 @@ def or!(other) # :nodoc:
10981098
raise ArgumentError, "Relation passed to #or must be structurally compatible. Incompatible values: #{incompatible_values}"
10991099
end
11001100

1101-
self.where_clause = self.where_clause.or(other.where_clause)
1101+
self.where_clause = where_clause.or(other.where_clause)
11021102
self.having_clause = having_clause.or(other.having_clause)
11031103
self.references_values |= other.references_values
11041104

activerecord/lib/active_record/relation/where_clause.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,11 @@ def or(other)
4747
right = right.ast
4848
right = right.expr if right.is_a?(Arel::Nodes::Grouping)
4949

50-
or_clause = Arel::Nodes::Or.new(left, right)
50+
or_clause = if left.is_a?(Arel::Nodes::Or)
51+
Arel::Nodes::Or.new(left.children + [right])
52+
else
53+
Arel::Nodes::Or.new([left, right])
54+
end
5155

5256
common.predicates << Arel::Nodes::Grouping.new(or_clause)
5357
common

activerecord/lib/arel/nodes.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@
4141
require "arel/nodes/regexp"
4242
require "arel/nodes/cte"
4343

44-
# nary
45-
require "arel/nodes/and"
44+
# nary (And and Or)
45+
require "arel/nodes/nary"
4646

4747
# function
4848
# FIXME: Function + Alias can be rewritten as a Function and Alias node.

activerecord/lib/arel/nodes/binary.rb

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,6 @@ def invert
111111
end
112112
end
113113

114-
class Or < Binary
115-
def fetch_attribute(&block)
116-
left.fetch_attribute(&block) && right.fetch_attribute(&block)
117-
end
118-
end
119-
120114
%w{
121115
Assignment
122116
Join

activerecord/lib/arel/nodes/and.rb renamed to activerecord/lib/arel/nodes/nary.rb

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
module Arel # :nodoc: all
44
module Nodes
5-
class And < Arel::Nodes::NodeExpression
5+
class Nary < Arel::Nodes::NodeExpression
66
attr_reader :children
77

88
def initialize(children)
@@ -23,7 +23,7 @@ def fetch_attribute(&block)
2323
end
2424

2525
def hash
26-
children.hash
26+
[self.class, children].hash
2727
end
2828

2929
def eql?(other)
@@ -32,5 +32,8 @@ def eql?(other)
3232
end
3333
alias :== :eql?
3434
end
35+
36+
And = Class.new(Nary)
37+
Or = Class.new(Nary)
3538
end
3639
end

activerecord/lib/arel/nodes/node.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ def not
127127
# Factory method to create a Nodes::Grouping node that has an Nodes::Or
128128
# node as a child.
129129
def or(right)
130-
Nodes::Grouping.new Nodes::Or.new(self, right)
130+
Nodes::Grouping.new Nodes::Or.new([self, right])
131131
end
132132

133133
###

activerecord/lib/arel/predications.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ def quoted_array(others)
232232
def grouping_any(method_id, others, *extras)
233233
nodes = others.map { |expr| send(method_id, expr, *extras) }
234234
Nodes::Grouping.new nodes.inject { |memo, node|
235-
Nodes::Or.new(memo, node)
235+
Nodes::Or.new([memo, node])
236236
}
237237
end
238238

activerecord/lib/arel/visitors/dot.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,7 @@ def visit__children(o)
191191
end
192192
end
193193
alias :visit_Arel_Nodes_And :visit__children
194+
alias :visit_Arel_Nodes_Or :visit__children
194195
alias :visit_Arel_Nodes_With :visit__children
195196

196197
def visit_String(o)

activerecord/lib/arel/visitors/to_sql.rb

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -622,18 +622,7 @@ def visit_Arel_Nodes_And(o, collector)
622622
end
623623

624624
def visit_Arel_Nodes_Or(o, collector)
625-
stack = [o.right, o.left]
626-
627-
while o = stack.pop
628-
if o.is_a?(Arel::Nodes::Or)
629-
stack.push o.right, o.left
630-
else
631-
visit o, collector
632-
collector << " OR " unless stack.empty?
633-
end
634-
end
635-
636-
collector
625+
inject_join o.children, collector, " OR "
637626
end
638627

639628
def visit_Arel_Nodes_Assignment(o, collector)

0 commit comments

Comments
 (0)