Make it the responsibility of the connection to hold onto an ARel visitor for generating SQL. This improves the code architecture generally, and solves some problems with marshalling. Adapter authors please take note: you now need to define an Adapter.visitor_for method, but it degrades gracefully with a deprecation warning for now.

18 files changed, 103 insertions, 44 deletions

diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index f7ce70db1a..1f917f58f2 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -26,7 +26,7 @@ module ActiveRecord
             join_table[reflection.association_foreign_key] => record.id
           )
 
-          owner.connection.insert stmt.to_sql
+          owner.connection.insert stmt
         end
 
         record
@@ -46,7 +46,7 @@ module ActiveRecord
             stmt = relation.where(relation[reflection.foreign_key].eq(owner.id).
               and(relation[reflection.association_foreign_key].in(records.map { |x| x.id }.compact))
             ).compile_delete
-            owner.connection.delete stmt.to_sql
+            owner.connection.delete stmt
           end
         end
 
diff --git a/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb b/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb
index 24be279449..b77b667219 100644
--- a/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb
+++ b/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb
@@ -13,7 +13,7 @@ module ActiveRecord
         # access the aliased column on the join table
         def records_for(ids)
           scope = super
-          klass.connection.select_all(scope.arel.to_sql, 'SQL', scope.bind_values)
+          klass.connection.select_all(scope.arel, 'SQL', scope.bind_values)
         end
 
         def owner_key_name
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 4136868b39..06087642d4 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1409,9 +1409,8 @@ MSG
           attrs = expand_hash_conditions_for_aggregates(attrs)
 
           table = Arel::Table.new(table_name).alias(default_table_name)
-          viz = Arel::Visitors.for(arel_engine)
           PredicateBuilder.build_from_hash(arel_engine, attrs, table).map { |b|
-            viz.accept b
+            connection.visitor.accept b
           }.join(' AND ')
         end
         alias_method :sanitize_sql_hash, :sanitize_sql_hash_for_conditions
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
index ddfdb05297..61994d4a47 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -82,10 +82,11 @@ module ActiveRecord
         # default max pool size to 5
         @size = (spec.config[:pool] && spec.config[:pool].to_i) || 5
 
-        @connections = []
-        @checked_out = []
+        @connections         = []
+        @checked_out         = []
         @automatic_reconnect = true
-        @tables = {}
+        @tables              = {}
+        @visitor             = nil
 
         @columns     = Hash.new do |h, table_name|
           h[table_name] = with_connection do |conn|
@@ -298,8 +299,18 @@ module ActiveRecord
         :connected?, :disconnect!, :with => :@connection_mutex
 
       private
+
       def new_connection
-        ActiveRecord::Base.send(spec.adapter_method, spec.config)
+        connection = ActiveRecord::Base.send(spec.adapter_method, spec.config)
+
+        # TODO: This is a bit icky, and in the long term we may want to change the method
+        #       signature for connections. Also, if we switch to have one visitor per
+        #       connection (and therefore per thread), we can get rid of the thread-local
+        #       variable in Arel::Visitors::ToSql.
+        @visitor ||= connection.class.visitor_for(self)
+        connection.visitor = @visitor
+
+        connection
       end
 
       def current_connection_id #:nodoc:
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
index 777ef15dfc..2ae655e68d 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -1,30 +1,39 @@
 module ActiveRecord
   module ConnectionAdapters # :nodoc:
     module DatabaseStatements
+      # Converts an arel AST to SQL
+      def to_sql(arel)
+        if arel.respond_to?(:ast)
+          visitor.accept(arel.ast)
+        else
+          arel
+        end
+      end
+
       # Returns an array of record hashes with the column names as keys and
       # column values as values.
-      def select_all(sql, name = nil, binds = [])
-        select(sql, name, binds)
+      def select_all(arel, name = nil, binds = [])
+        select(to_sql(arel), name, binds)
       end
 
       # Returns a record hash with the column names as keys and column values
       # as values.
-      def select_one(sql, name = nil)
-        result = select_all(sql, name)
+      def select_one(arel, name = nil)
+        result = select_all(arel, name)
         result.first if result
       end
 
       # Returns a single value from a record
-      def select_value(sql, name = nil)
-        if result = select_one(sql, name)
+      def select_value(arel, name = nil)
+        if result = select_one(arel, name)
           result.values.first
         end
       end
 
       # Returns an array of the values of the first column in a select:
       #   select_values("SELECT id FROM companies LIMIT 3") => [1,2,3]
-      def select_values(sql, name = nil)
-        result = select_rows(sql, name)
+      def select_values(arel, name = nil)
+        result = select_rows(to_sql(arel), name)
         result.map { |v| v[0] }
       end
 
@@ -74,20 +83,20 @@ module ActiveRecord
       #
       # If the next id was calculated in advance (as in Oracle), it should be
       # passed in as +id_value+.
-      def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil, binds = [])
-        sql, binds = sql_for_insert(sql, pk, id_value, sequence_name, binds)
+      def insert(arel, name = nil, pk = nil, id_value = nil, sequence_name = nil, binds = [])
+        sql, binds = sql_for_insert(to_sql(arel), pk, id_value, sequence_name, binds)
         value      = exec_insert(sql, name, binds)
         id_value || last_inserted_id(value)
       end
 
       # Executes the update statement and returns the number of rows affected.
-      def update(sql, name = nil, binds = [])
-        exec_update(sql, name, binds)
+      def update(arel, name = nil, binds = [])
+        exec_update(to_sql(arel), name, binds)
       end
 
       # Executes the delete statement and returns the number of rows affected.
-      def delete(sql, name = nil, binds = [])
-        exec_delete(sql, name, binds)
+      def delete(arel, name = nil, binds = [])
+        exec_delete(to_sql(arel), name, binds)
       end
 
       # Checks whether there is currently no transaction active. This is done
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
index 093c30aa42..27ff13ad89 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
@@ -55,9 +55,10 @@ module ActiveRecord
         @query_cache.clear
       end
 
-      def select_all(sql, name = nil, binds = [])
+      def select_all(arel, name = nil, binds = [])
         if @query_cache_enabled
-          cache_sql(sql, binds) { super }
+          sql = to_sql(arel)
+          cache_sql(sql, binds) { super(sql, name, binds) }
         else
           super
         end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index bde31d1cda..88fd180fa5 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -2,6 +2,7 @@ require 'date'
 require 'bigdecimal'
 require 'bigdecimal/util'
 require 'active_support/core_ext/benchmark'
+require 'active_support/deprecation'
 
 # TODO: Autoload these files
 require 'active_record/connection_adapters/column'
@@ -38,6 +39,8 @@ module ActiveRecord
 
       define_callbacks :checkout, :checkin
 
+      attr_accessor :visitor
+
       def initialize(connection, logger = nil) #:nodoc:
         @active = nil
         @connection, @logger = connection, logger
@@ -45,6 +48,27 @@ module ActiveRecord
         @query_cache = Hash.new { |h,sql| h[sql] = {} }
         @open_transactions = 0
         @instrumenter = ActiveSupport::Notifications.instrumenter
+        @visitor = nil
+      end
+
+      # Returns a visitor instance for this adaptor, which conforms to the Arel::ToSql interface
+      def self.visitor_for(pool) # :nodoc:
+        adapter = pool.spec.config[:adapter]
+
+        if Arel::Visitors::VISITORS[adapter]
+          # TODO: Add a test for this
+
+          ActiveSupport::Deprecation.warn(
+            "Arel::Visitors::VISITORS is deprecated and will be removed. Database adapters " \
+            "should define a visitor_for method which returns the appropriate visitor for " \
+            "the database. For example, MysqlAdapter.visitor_for(pool) returns " \
+            "Arel::Visitors::MySQL.new(pool)."
+          )
+
+          Arel::Visitors::VISITORS[adapter].new(pool)
+        else
+          Arel::Visitors::ToSql.new(pool)
+        end
       end
 
       # Returns the human-readable name of the adapter. Use mixed case - one
diff --git a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
index f9602bbe77..18fdfa29ec 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
@@ -129,6 +129,10 @@ module ActiveRecord
         configure_connection
       end
 
+      def self.visitor_for(pool) # :nodoc:
+        Arel::Visitors::MySQL.new(pool)
+      end
+
       def adapter_name
         ADAPTER_NAME
       end
diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
index 9e6cb13cca..14b950dbb0 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
@@ -192,6 +192,10 @@ module ActiveRecord
         connect
       end
 
+      def self.visitor_for(pool) # :nodoc:
+        Arel::Visitors::MySQL.new(pool)
+      end
+
       def adapter_name #:nodoc:
         ADAPTER_NAME
       end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index aefe69f8ed..45c13bdcd6 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -265,6 +265,10 @@ module ActiveRecord
         @local_tz = execute('SHOW TIME ZONE', 'SCHEMA').first["TimeZone"]
       end
 
+      def self.visitor_for(pool) # :nodoc:
+        Arel::Visitors::PostgreSQL.new(pool)
+      end
+
       # Clears the prepared statements cache.
       def clear_cache!
         @statements.each_value do |value|
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
index ba65ff4357..486efc5ba0 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
@@ -53,6 +53,10 @@ module ActiveRecord
         @config = config
       end
 
+      def self.visitor_for(pool) # :nodoc:
+        Arel::Visitors::SQLite.new(pool)
+      end
+
       def adapter_name #:nodoc:
         'SQLite'
       end
diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb
index 4d387565d9..3c7defedac 100644
--- a/activerecord/lib/active_record/counter_cache.rb
+++ b/activerecord/lib/active_record/counter_cache.rb
@@ -33,7 +33,7 @@ module ActiveRecord
         stmt = unscoped.where(arel_table[primary_key].eq(object.id)).arel.compile_update({
           arel_table[counter_name] => object.send(association).count
         })
-        connection.update stmt.to_sql
+        connection.update stmt
       end
       return true
     end
diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb
index 6cfce6e573..d9ad7e4132 100644
--- a/activerecord/lib/active_record/locking/optimistic.rb
+++ b/activerecord/lib/active_record/locking/optimistic.rb
@@ -70,7 +70,7 @@ module ActiveRecord
 
           # If the locking column has no default value set,
           # start the lock version at zero. Note we can't use
-          # <tt>locking_enabled?</tt> at this point as 
+          # <tt>locking_enabled?</tt> at this point as
           # <tt>@attributes</tt> may not have been initialized yet.
 
           if result.key?(self.class.locking_column) && lock_optimistically
@@ -100,7 +100,7 @@ module ActiveRecord
               )
             ).arel.compile_update(arel_attributes_values(false, false, attribute_names))
 
-            affected_rows = connection.update stmt.to_sql
+            affected_rows = connection.update stmt
 
             unless affected_rows == 1
               raise ActiveRecord::StaleObjectError, "Attempted to update a stale object: #{self.class.name}"
diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
index fa1b303fc7..7166f1b82a 100644
--- a/activerecord/lib/active_record/migration.rb
+++ b/activerecord/lib/active_record/migration.rb
@@ -563,7 +563,7 @@ module ActiveRecord
 
       def get_all_versions
         table = Arel::Table.new(schema_migrations_table_name)
-        Base.connection.select_values(table.project(table['version']).to_sql).map{ |v| v.to_i }.sort
+        Base.connection.select_values(table.project(table['version'])).map{ |v| v.to_i }.sort
       end
 
       def current_version
@@ -720,11 +720,11 @@ module ActiveRecord
         if down?
           @migrated_versions.delete(version)
           stmt = table.where(table["version"].eq(version.to_s)).compile_delete
-          Base.connection.delete stmt.to_sql
+          Base.connection.delete stmt
         else
           @migrated_versions.push(version).sort!
           stmt = table.compile_insert table["version"] => version.to_s
-          Base.connection.insert stmt.to_sql
+          Base.connection.insert stmt
         end
       end
 
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index ebda3875cd..2dac9ea0fb 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -304,7 +304,7 @@ module ActiveRecord
       return 0 if attributes_with_values.empty?
       klass = self.class
       stmt = klass.unscoped.where(klass.arel_table[klass.primary_key].eq(id)).arel.compile_update(attributes_with_values)
-      klass.connection.update stmt.to_sql
+      klass.connection.update stmt
     end
 
     # Creates a record with values matching those of the instance attributes
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index fff0ad1b83..7e59eb4584 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -68,7 +68,7 @@ module ActiveRecord
       end
 
       conn.insert(
-        im.to_sql,
+        im,
         'SQL',
         primary_key,
         primary_key_value,
@@ -108,10 +108,10 @@ module ActiveRecord
 
       if default_scoped.equal?(self)
         @records = if @readonly_value.nil? && !@klass.locking_enabled?
-          eager_loading? ? find_with_associations : @klass.find_by_sql(arel.to_sql, @bind_values)
+          eager_loading? ? find_with_associations : @klass.find_by_sql(arel, @bind_values)
         else
           IdentityMap.without do
-            eager_loading? ? find_with_associations : @klass.find_by_sql(arel.to_sql, @bind_values)
+            eager_loading? ? find_with_associations : @klass.find_by_sql(arel, @bind_values)
           end
         end
 
@@ -224,7 +224,7 @@ module ActiveRecord
 
         stmt.order(*arel.orders)
         stmt.key = table[primary_key]
-        @klass.connection.update stmt.to_sql, 'SQL', bind_values
+        @klass.connection.update stmt, 'SQL', bind_values
       end
     end
 
@@ -341,8 +341,7 @@ module ActiveRecord
         where(conditions).delete_all
       else
         statement = arel.compile_delete
-        affected = @klass.connection.delete(
-          statement.to_sql, 'SQL', bind_values)
+        affected = @klass.connection.delete(statement, 'SQL', bind_values)
 
         reset
         affected
@@ -388,7 +387,7 @@ module ActiveRecord
     end
 
     def to_sql
-      @to_sql ||= arel.to_sql
+      @to_sql ||= klass.connection.to_sql(arel)
     end
 
     def where_values_hash
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 9a7ff87e88..af86771d2d 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -223,7 +223,7 @@ module ActiveRecord
         query_builder = relation.arel
       end
 
-      type_cast_calculated_value(@klass.connection.select_value(query_builder.to_sql), column_for(column_name), operation)
+      type_cast_calculated_value(@klass.connection.select_value(query_builder), column_for(column_name), operation)
     end
 
     def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
@@ -259,7 +259,7 @@ module ActiveRecord
       relation = except(:group).group(group.join(','))
       relation.select_values = select_values
 
-      calculated_data = @klass.connection.select_all(relation.to_sql)
+      calculated_data = @klass.connection.select_all(relation)
 
       if association
         key_ids     = calculated_data.collect { |row| row[group_aliases.first] }
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 8cef4e5554..73368aed18 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -193,8 +193,8 @@ module ActiveRecord
       else
         relation = relation.where(table[primary_key].eq(id)) if id
       end
-      
-      connection.select_value(relation.to_sql, "#{name} Exists") ? true : false
+
+      connection.select_value(relation, "#{name} Exists") ? true : false
     end
 
     protected
@@ -202,7 +202,7 @@ module ActiveRecord
     def find_with_associations
       join_dependency = construct_join_dependency_for_association_find
       relation = construct_relation_for_association_find(join_dependency)
-      rows = connection.select_all(relation.to_sql, 'SQL', relation.bind_values)
+      rows = connection.select_all(relation, 'SQL', relation.bind_values)
       join_dependency.instantiate(rows)
     rescue ThrowResult
       []