Merge pull request #39051 from kamipo/more_concise_or_ast

More concise Arel `Or` ast and make `Or` visitor non recursive

activerecord/lib/active_record/relation/where_clause.rb

@@ -39,10 +39,16 @@ def or(other)
         if left.empty? || right.empty?
           common
         else
-          or_clause = WhereClause.new(
-            [left.ast.or(right.ast)],
-          )
-          common + or_clause
+          left = left.ast
+          left = left.expr if left.is_a?(Arel::Nodes::Grouping)
+
+          right = right.ast
+          right = right.expr if right.is_a?(Arel::Nodes::Grouping)
+
+          or_clause = Arel::Nodes::Or.new(left, right)
+
+          common.predicates << Arel::Nodes::Grouping.new(or_clause)
+          common
         end
       end
 

activerecord/lib/arel/visitors/to_sql.rb

@@ -546,9 +546,18 @@ def visit_Arel_Nodes_And(o, collector)
         end
 
         def visit_Arel_Nodes_Or(o, collector)
-          collector = visit o.left, collector
-          collector << " OR "
-          visit o.right, collector
+          stack = [o.right, o.left]
+
+          while o = stack.pop
+            if o.is_a?(Arel::Nodes::Or)
+              stack.push o.right, o.left
+            else
+              visit o, collector
+              collector << " OR " unless stack.empty?
+            end
+          end
+
+          collector
         end
 
         def visit_Arel_Nodes_Assignment(o, collector)

activerecord/test/cases/relation/or_test.rb

@@ -4,11 +4,11 @@
 require "models/author"
 require "models/categorization"
 require "models/post"
+require "models/citation"
 
 module ActiveRecord
   class OrTest < ActiveRecord::TestCase
-    fixtures :posts
-    fixtures :authors, :author_addresses
+    fixtures :posts, :authors, :author_addresses
 
     def test_or_with_relation
       expected = Post.where("id = 1 or id = 2").to_a
@@ -151,4 +151,20 @@ def test_structurally_incompatible_values
       end
     end
   end
+
+  # The maximum expression tree depth is 1000 by default for SQLite3.
+  # https://www.sqlite.org/limits.html#max_expr_depth
+  unless current_adapter?(:SQLite3Adapter)
+    class TooManyOrTest < ActiveRecord::TestCase
+      fixtures :citations
+
+      def test_too_many_or
+        citations = 6000.times.map do |i|
+          Citation.where(id: i, book2_id: i * i)
+        end
+
+        assert_equal 6000, citations.inject(&:or).count
+      end
+    end
+  end
 end