Ensure function aliases allow retrying queries

Previously, function aliases (ex. combining a `#group` and `#count`)
would make queries non-retryable because `Function#as` would always wrap
a given alias in a non-retryable `SqlLiteral`.

This could be addressed by making `Function#as` behave similarly to
`AliasPredication` (retry everything). However, `Function#as` can also
just be removed so that `Function` uses `AliasPredication#as` instead.
This behaves slightly differently because `AliasPredication#as` will now
return the `Function` wrapped in an `Alias` (meaning the return value
must be used). Only a single place in Active Record needed to be changed
and all others already did the correct thing.

This also resolves a long standing FIXME suggesting removing the custom
`Function#as` in favor of using `Alias` nodes. The other half of the
comment suggesting `Function` could inherit from `Unary` does not make
sense because SQL functions could potentially take multiple parameters,
so the existing superclass should not be changed.

Author: skipkayhil

activerecord/lib/active_record/relation/calculations.rb

@@ -545,7 +545,7 @@ def execute_grouped_calculation(operation, column_name, distinct) # :nodoc:
           column = relation.aggregate_column(column_name)
           column_alias = column_alias_tracker.alias_for("#{operation} #{column_name.to_s.downcase}")
           select_value = operation_over_aggregate_column(column, operation, distinct)
-          select_value.as(model.adapter_class.quote_column_name(column_alias))
+          select_value = select_value.as(model.adapter_class.quote_column_name(column_alias))
 
           select_values = [select_value]
           select_values += self.select_values unless having_clause.empty?

activerecord/lib/arel/nodes.rb

@@ -45,8 +45,6 @@
 require "arel/nodes/nary"
 
 # function
-# FIXME: Function + Alias can be rewritten as a Function and Alias node.
-# We should make Function a Unary node and deprecate the use of "aliaz"
 require "arel/nodes/function"
 require "arel/nodes/count"
 require "arel/nodes/extract"

activerecord/lib/arel/nodes/count.rb

@@ -3,8 +3,8 @@
 module Arel # :nodoc: all
   module Nodes
     class Count < Arel::Nodes::Function
-      def initialize(expr, distinct = false, aliaz = nil)
-        super(expr, aliaz)
+      def initialize(expr, distinct = false)
+        super(expr)
         @distinct = distinct
       end
     end

activerecord/lib/arel/nodes/function.rb

@@ -5,28 +5,22 @@ module Nodes
     class Function < Arel::Nodes::NodeExpression
       include Arel::WindowPredications
       include Arel::FilterPredications
-      attr_accessor :expressions, :alias, :distinct
 
-      def initialize(expr, aliaz = nil)
+      attr_accessor :expressions, :distinct
+
+      def initialize(expr)
         super()
         @expressions = expr
-        @alias       = aliaz && SqlLiteral.new(aliaz)
         @distinct    = false
       end
 
-      def as(aliaz)
-        self.alias = SqlLiteral.new(aliaz)
-        self
-      end
-
       def hash
-        [@expressions, @alias, @distinct].hash
+        [@expressions, @distinct].hash
       end
 
       def eql?(other)
         self.class == other.class &&
           self.expressions == other.expressions &&
-          self.alias == other.alias &&
           self.distinct == other.distinct
       end
       alias :== :eql?

activerecord/lib/arel/nodes/named_function.rb

@@ -5,8 +5,8 @@ module Nodes
     class NamedFunction < Arel::Nodes::Function
       attr_accessor :name
 
-      def initialize(name, expr, aliaz = nil)
-        super(expr, aliaz)
+      def initialize(name, expr)
+        super(expr)
         @name = name
       end
 

activerecord/lib/arel/visitors/dot.rb

@@ -34,7 +34,6 @@ def accept(object, collector)
         def visit_Arel_Nodes_Function(o)
           visit_edge o, "expressions"
           visit_edge o, "distinct"
-          visit_edge o, "alias"
         end
 
         def visit_Arel_Nodes_Unary(o)
@@ -108,14 +107,12 @@ def visit__no_edges(o)
 
         def visit_Arel_Nodes_Extract(o)
           visit_edge o, "expressions"
-          visit_edge o, "alias"
         end
 
         def visit_Arel_Nodes_NamedFunction(o)
           visit_edge o, "name"
           visit_edge o, "expressions"
           visit_edge o, "distinct"
-          visit_edge o, "alias"
         end
 
         def visit_Arel_Nodes_InsertStatement(o)

activerecord/lib/arel/visitors/to_sql.rb

@@ -75,13 +75,7 @@ def visit_Arel_Nodes_InsertStatement(o, collector)
 
         def visit_Arel_Nodes_Exists(o, collector)
           collector << "EXISTS ("
-          collector = visit(o.expressions, collector) << ")"
-          if o.alias
-            collector << " AS "
-            visit o.alias, collector
-          else
-            collector
-          end
+          visit(o.expressions, collector) << ")"
         end
 
         def visit_Arel_Nodes_Casted(o, collector)
@@ -388,13 +382,7 @@ def visit_Arel_Nodes_NamedFunction(o, collector)
           collector << o.name
           collector << "("
           collector << "DISTINCT " if o.distinct
-          collector = inject_join(o.expressions, collector, ", ") << ")"
-          if o.alias
-            collector << " AS "
-            visit o.alias, collector
-          else
-            collector
-          end
+          inject_join(o.expressions, collector, ", ") << ")"
         end
 
         def visit_Arel_Nodes_Extract(o, collector)
@@ -998,13 +986,7 @@ def aggregate(name, o, collector)
           if o.distinct
             collector << "DISTINCT "
           end
-          collector = inject_join(o.expressions, collector, ", ") << ")"
-          if o.alias
-            collector << " AS "
-            visit o.alias, collector
-          else
-            collector
-          end
+          inject_join(o.expressions, collector, ", ") << ")"
         end
 
         def is_distinct_from(o, collector)

activerecord/test/cases/adapter_test.rb

@@ -709,9 +709,10 @@ def teardown
           assert_predicate Post, :exists?
           a.books.to_a
           Author.select(:status).joins(:books).group(:status).to_a
+          Author.group(:name).count
         end.select { |n| n.payload[:name] != "SCHEMA" }
 
-        assert_equal 7, notifications.length
+        assert_equal 8, notifications.length
 
         notifications.each do |n|
           assert n.payload[:allow_retry], "#{n.payload[:sql]} was not retryable"
@@ -729,9 +730,10 @@ def teardown
           assert_predicate Post, :exists?
           a.books.to_a
           Author.select(:status).joins(:books).group(:status).to_a
+          Author.group(:name).count
         end.select { |n| n.payload[:name] != "SCHEMA" }
 
-        assert_equal 7, notifications.length
+        assert_equal 8, notifications.length
 
         notifications.each do |n|
           assert n.payload[:allow_retry], "#{n.payload[:sql]} was not retryable"

activerecord/test/cases/arel/nodes/homogeneous_in_test.rb

@@ -36,7 +36,7 @@ class TypedNode < Arel::Nodes::NamedFunction
       attr_reader :type_caster
 
       def initialize(name, expr, type)
-        super(name, expr, nil)
+        super(name, expr)
         @type_caster = type
       end
     end

activerecord/test/cases/arel/nodes/named_function_test.rb

@@ -11,35 +11,18 @@ def test_construct
         assert_equal "zomg", function.expressions
       end
 
-      def test_function_alias
-        function = NamedFunction.new "omg", "zomg"
-        function = function.as("wth")
-        assert_equal "omg", function.name
-        assert_equal "zomg", function.expressions
-        assert_kind_of SqlLiteral, function.alias
-        assert_equal "wth", function.alias
-      end
-
-      def test_construct_with_alias
-        function = NamedFunction.new "omg", "zomg", "wth"
-        assert_equal "omg", function.name
-        assert_equal "zomg", function.expressions
-        assert_kind_of SqlLiteral, function.alias
-        assert_equal "wth", function.alias
-      end
-
       def test_equality_with_same_ivars
         array = [
-          NamedFunction.new("omg", "zomg", "wth"),
-          NamedFunction.new("omg", "zomg", "wth")
+          NamedFunction.new("omg", "zomg"),
+          NamedFunction.new("omg", "zomg")
         ]
         assert_equal 1, array.uniq.size
       end
 
       def test_inequality_with_different_ivars
         array = [
-          NamedFunction.new("omg", "zomg", "wth"),
-          NamedFunction.new("zomg", "zomg", "wth")
+          NamedFunction.new("omg", "zomg"),
+          NamedFunction.new("zomg", "zomg")
         ]
         assert_equal 2, array.uniq.size
       end