From 699b64ab6322a549a84c7d116fe18cbb522ce64f Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Sat, 25 Apr 2020 16:56:09 +0900
Subject: [PATCH] Improve `WhereClause#ast` to make concise Arel ast
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If only one Arel node exist, wrapping a node by `And` node is obviously
redundant, make concise Arel ast will improve performance for visiting
the ast (about 20% faster for complex ast case).

```ruby
class Post < ActiveRecord::Base
end

posts = (0..500).map { |i| Post.where(id: i) }

Benchmark.ips do |x|
  x.report("inject scopes") { posts.inject { |res, scope| res.or(scope) }.to_sql }
end
```

Before:

```
Warming up --------------------------------------
      where with ids     8.000  i/100ms
Calculating -------------------------------------
      where with ids     80.416  (± 2.5%) i/s -    408.000  in   5.078118s
```

After:

```
Warming up --------------------------------------
      where with ids     9.000  i/100ms
Calculating -------------------------------------
      where with ids     96.126  (± 2.1%) i/s -    486.000  in   5.058960s
```
---
 .../associations/join_dependency/join_association.rb  | 11 +++++++----
 .../lib/active_record/relation/where_clause.rb        |  3 ++-
 .../test/cases/scoping/default_scoping_test.rb        |  7 +++----
 activerecord/test/models/vehicle.rb                   |  9 ---------
 4 files changed, 12 insertions(+), 18 deletions(-)
 delete mode 100644 activerecord/test/models/vehicle.rb

diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
index 9cdaa1a918..3c66ce1866 100644
--- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb
+++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -36,13 +36,15 @@ module ActiveRecord
             arel = join_scope.arel(alias_tracker.aliases)
             nodes = arel.constraints.first
 
-            others = nodes.children.extract! do |node|
-              !Arel.fetch_attribute(node) { |attr| attr.relation.name == table.name }
+            if nodes.is_a?(Arel::Nodes::And)
+              others = nodes.children.extract! do |node|
+                !Arel.fetch_attribute(node) { |attr| attr.relation.name == table.name }
+              end
             end
 
             joins << table.create_join(table, table.create_on(nodes), join_type)
 
-            unless others.empty?
+            if others && !others.empty?
               joins.concat arel.join_sources
               append_constraints(joins.last, others)
             end
@@ -77,7 +79,8 @@ module ActiveRecord
               join_string = table.create_and(constraints.unshift(join.left))
               join.left = Arel.sql(base_klass.connection.visitor.compile(join_string))
             else
-              join.right.expr.children.concat(constraints)
+              right = join.right
+              right.expr = Arel::Nodes::And.new(constraints.unshift right.expr)
             end
           end
       end
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
index 6029682fd8..a881a3b51b 100644
--- a/activerecord/lib/active_record/relation/where_clause.rb
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -62,7 +62,8 @@ module ActiveRecord
       end
 
       def ast
-        Arel::Nodes::And.new(predicates_with_wrapped_sql_literals)
+        predicates = predicates_with_wrapped_sql_literals
+        predicates.one? ? predicates.first : Arel::Nodes::And.new(predicates)
       end
 
       def ==(other)
diff --git a/activerecord/test/cases/scoping/default_scoping_test.rb b/activerecord/test/cases/scoping/default_scoping_test.rb
index b8d74e54b0..e14ae8a6d4 100644
--- a/activerecord/test/cases/scoping/default_scoping_test.rb
+++ b/activerecord/test/cases/scoping/default_scoping_test.rb
@@ -6,7 +6,6 @@ require "models/comment"
 require "models/developer"
 require "models/project"
 require "models/computer"
-require "models/vehicle"
 require "models/cat"
 require "concurrent/atomic/cyclic_barrier"
 
@@ -499,13 +498,13 @@ class DefaultScopingTest < ActiveRecord::TestCase
 
   test "a scope can remove the condition from the default scope" do
     scope = DeveloperCalledJamis.david2
-    assert_equal 1, scope.where_clause.ast.children.length
+    assert_instance_of Arel::Nodes::Equality, scope.where_clause.ast
     assert_equal Developer.where(name: "David").map(&:id), scope.map(&:id)
   end
 
   def test_with_abstract_class_where_clause_should_not_be_duplicated
-    scope = Bus.all
-    assert_equal scope.where_clause.ast.children.length, 1
+    scope = Lion.all
+    assert_instance_of Arel::Nodes::Equality, scope.where_clause.ast
   end
 
   def test_sti_conditions_are_not_carried_in_default_scope
diff --git a/activerecord/test/models/vehicle.rb b/activerecord/test/models/vehicle.rb
deleted file mode 100644
index c9b3338522..0000000000
--- a/activerecord/test/models/vehicle.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-# frozen_string_literal: true
-
-class Vehicle < ActiveRecord::Base
-  self.abstract_class = true
-  default_scope -> { where("tires_count IS NOT NULL") }
-end
-
-class Bus < Vehicle
-end