Fix sliced IN clauses to be grouped

Follow up of #35838.

And also this refactors `in_clause_length` handling is entirely
integrated in Arel visitor.

4 files changed, 50 insertions, 121 deletions

diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index 46532f651e..342d9e7a5a 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -36,13 +36,7 @@ module ActiveRecord
 
         def preloaded_records
           return @preloaded_records if defined?(@preloaded_records)
-          return [] if owner_keys.empty?
-          # Some databases impose a limit on the number of ids in a list (in Oracle it's 1000)
-          # Make several smaller queries if necessary or make one query if the adapter supports it
-          slices = owner_keys.each_slice(klass.connection.in_clause_length || owner_keys.size)
-          @preloaded_records = slices.flat_map do |slice|
-            records_for(slice)
-          end
+          @preloaded_records = owner_keys.empty? ? [] : records_for(owner_keys)
         end
 
         private
diff --git a/activerecord/lib/arel/visitors/oracle.rb b/activerecord/lib/arel/visitors/oracle.rb
index 500974dff5..f96bf65ee5 100644
--- a/activerecord/lib/arel/visitors/oracle.rb
+++ b/activerecord/lib/arel/visitors/oracle.rb
@@ -87,50 +87,6 @@ module Arel # :nodoc: all
           collector << " )"
         end
 
-        def visit_Arel_Nodes_In(o, collector)
-          if Array === o.right && !o.right.empty?
-            o.right.delete_if { |value| unboundable?(value) }
-          end
-
-          if Array === o.right && o.right.empty?
-            collector << "1=0"
-          else
-            first = true
-            o.right.each_slice(in_clause_length) do |sliced_o_right|
-              collector << " OR " unless first
-              first = false
-
-              collector = visit o.left, collector
-              collector << " IN ("
-              visit(sliced_o_right, collector)
-              collector << ")"
-            end
-          end
-          collector
-        end
-
-        def visit_Arel_Nodes_NotIn(o, collector)
-          if Array === o.right && !o.right.empty?
-            o.right.delete_if { |value| unboundable?(value) }
-          end
-
-          if Array === o.right && o.right.empty?
-            collector << "1=1"
-          else
-            first = true
-            o.right.each_slice(in_clause_length) do |sliced_o_right|
-              collector << " AND " unless first
-              first = false
-
-              collector = visit o.left, collector
-              collector << " NOT IN ("
-              visit(sliced_o_right, collector)
-              collector << ")"
-            end
-          end
-          collector
-        end
-
         def visit_Arel_Nodes_UpdateStatement(o, collector)
           # Oracle does not allow ORDER BY/LIMIT in UPDATEs.
           if o.orders.any? && o.limit.nil?
@@ -198,10 +154,6 @@ module Arel # :nodoc: all
           collector = visit [o.left, o.right, 0, 1], collector
           collector << ")"
         end
-
-        def in_clause_length
-          1000
-        end
     end
   end
 end
diff --git a/activerecord/lib/arel/visitors/oracle12.rb b/activerecord/lib/arel/visitors/oracle12.rb
index 8e0f07fca9..6269bc3907 100644
--- a/activerecord/lib/arel/visitors/oracle12.rb
+++ b/activerecord/lib/arel/visitors/oracle12.rb
@@ -8,11 +8,10 @@ module Arel # :nodoc: all
         def visit_Arel_Nodes_SelectStatement(o, collector)
           # Oracle does not allow LIMIT clause with select for update
           if o.limit && o.lock
-            raise ArgumentError, <<-MSG
-            'Combination of limit and lock is not supported.
-            because generated SQL statements
-            `SELECT FOR UPDATE and FETCH FIRST n ROWS` generates ORA-02014.`
-          MSG
+            raise ArgumentError, <<~MSG
+              Combination of limit and lock is not supported. Because generated SQL statements
+              `SELECT FOR UPDATE and FETCH FIRST n ROWS` generates ORA-02014.
+            MSG
           end
           super
         end
@@ -41,50 +40,6 @@ module Arel # :nodoc: all
           collector << " )"
         end
 
-        def visit_Arel_Nodes_In(o, collector)
-          if Array === o.right && !o.right.empty?
-            o.right.delete_if { |value| unboundable?(value) }
-          end
-
-          if Array === o.right && o.right.empty?
-            collector << "1=0"
-          else
-            first = true
-            o.right.each_slice(in_clause_length) do |sliced_o_right|
-              collector << " OR " unless first
-              first = false
-
-              collector = visit o.left, collector
-              collector << " IN ("
-              visit(sliced_o_right, collector)
-              collector << ")"
-            end
-          end
-          collector
-        end
-
-        def visit_Arel_Nodes_NotIn(o, collector)
-          if Array === o.right && !o.right.empty?
-            o.right.delete_if { |value| unboundable?(value) }
-          end
-
-          if Array === o.right && o.right.empty?
-            collector << "1=1"
-          else
-            first = true
-            o.right.each_slice(in_clause_length) do |sliced_o_right|
-              collector << " AND " unless first
-              first = false
-
-              collector = visit o.left, collector
-              collector << " NOT IN ("
-              visit(sliced_o_right, collector)
-              collector << ")"
-            end
-          end
-          collector
-        end
-
         def visit_Arel_Nodes_UpdateStatement(o, collector)
           # Oracle does not allow ORDER BY/LIMIT in UPDATEs.
           if o.orders.any? && o.limit.nil?
@@ -106,10 +61,6 @@ module Arel # :nodoc: all
           collector = visit [o.left, o.right, 0, 1], collector
           collector << ")"
         end
-
-        def in_clause_length
-          1000
-        end
     end
   end
 end
diff --git a/activerecord/lib/arel/visitors/to_sql.rb b/activerecord/lib/arel/visitors/to_sql.rb
index 277d553e6c..4740e6d94f 100644
--- a/activerecord/lib/arel/visitors/to_sql.rb
+++ b/activerecord/lib/arel/visitors/to_sql.rb
@@ -513,34 +513,66 @@ module Arel # :nodoc: all
         end
 
         def visit_Arel_Nodes_In(o, collector)
-          if Array === o.right && !o.right.empty?
+          unless Array === o.right
+            return collect_in_clause(o.left, o.right, collector)
+          end
+
+          unless o.right.empty?
             o.right.delete_if { |value| unboundable?(value) }
           end
 
-          if Array === o.right && o.right.empty?
-            collector << "1=0"
+          return collector << "1=0" if o.right.empty?
+
+          in_clause_length = @connection.in_clause_length
+
+          if !in_clause_length || o.right.length <= in_clause_length
+            collect_in_clause(o.left, o.right, collector)
           else
-            collector = visit o.left, collector
-            collector << " IN ("
-            visit(o.right, collector) << ")"
+            collector << "("
+            o.right.each_slice(in_clause_length).each_with_index do |right, i|
+              collector << " OR " unless i == 0
+              collect_in_clause(o.left, right, collector)
+            end
+            collector << ")"
           end
         end
 
+        def collect_in_clause(left, right, collector)
+          collector = visit left, collector
+          collector << " IN ("
+          visit(right, collector) << ")"
+        end
+
         def visit_Arel_Nodes_NotIn(o, collector)
-          if Array === o.right && !o.right.empty?
+          unless Array === o.right
+            return collect_not_in_clause(o.left, o.right, collector)
+          end
+
+          unless o.right.empty?
             o.right.delete_if { |value| unboundable?(value) }
           end
 
-          if Array === o.right && o.right.empty?
-            collector << "1=1"
+          return collector << "1=1" if o.right.empty?
+
+          in_clause_length = @connection.in_clause_length
+
+          if !in_clause_length || o.right.length <= in_clause_length
+            collect_not_in_clause(o.left, o.right, collector)
           else
-            collector = visit o.left, collector
-            collector << " NOT IN ("
-            collector = visit o.right, collector
-            collector << ")"
+            o.right.each_slice(in_clause_length).each_with_index do |right, i|
+              collector << " AND " unless i == 0
+              collect_not_in_clause(o.left, right, collector)
+            end
+            collector
           end
         end
 
+        def collect_not_in_clause(left, right, collector)
+          collector = visit left, collector
+          collector << " NOT IN ("
+          visit(right, collector) << ")"
+        end
+
         def visit_Arel_Nodes_And(o, collector)
           inject_join o.children, collector, " AND "
         end