Address review comments

Author: roji

src/EFCore.SqlServer/Query/Internal/SqlServerAggregateFunctionExpression.cs

@@ -11,7 +11,7 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;
 ///     any release. You should only use it directly in your code with extreme caution and knowing that
 ///     doing so can result in application failures when updating to a new Entity Framework Core release.
 /// </summary>
-public class SqlServerAggregateFunctionExpression : SqlExpression, IEquatable<SqlServerAggregateFunctionExpression>
+public class SqlServerAggregateFunctionExpression : SqlExpression
 {
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -37,12 +37,18 @@ public SqlServerAggregateFunctionExpression(
     }
 
     /// <summary>
-    ///     The name of the function.
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual string Name { get; }
 
     /// <summary>
-    ///     The list of arguments of this function.
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual IReadOnlyList<SqlExpression> Arguments { get; }
 
@@ -55,12 +61,18 @@ public SqlServerAggregateFunctionExpression(
     public virtual IReadOnlyList<OrderingExpression> Orderings { get; }
 
     /// <summary>
-    ///     A bool value indicating if the function can return null result.
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual bool IsNullable { get; }
 
     /// <summary>
-    ///     A list of bool values indicating whether individual argument propagate null to the result.
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual IReadOnlyList<bool> ArgumentsPropagateNullability { get; }
 
@@ -125,10 +137,11 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
     }
 
     /// <summary>
-    ///     Applies supplied type mapping to this expression.
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    /// <param name="typeMapping">A relational type mapping to apply.</param>
-    /// <returns>A new expression which has supplied type mapping.</returns>
     public virtual SqlServerAggregateFunctionExpression ApplyTypeMapping(RelationalTypeMapping? typeMapping)
         => new(
             Name,
@@ -181,8 +194,7 @@ protected override void Print(ExpressionPrinter expressionPrinter)
     public override bool Equals(object? obj)
         => obj is SqlServerAggregateFunctionExpression sqlServerFunctionExpression && Equals(sqlServerFunctionExpression);
 
-    /// <inheritdoc />
-    public virtual bool Equals(SqlServerAggregateFunctionExpression? other)
+    private bool Equals(SqlServerAggregateFunctionExpression? other)
         => ReferenceEquals(this, other)
             || other is not null
             && base.Equals(other)

src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs

@@ -101,7 +101,7 @@ protected override void GenerateLimitOffset(SelectExpression selectExpression)
     ///     any release. You should only use it directly in your code with extreme caution and knowing that
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    protected virtual Expression VisitAggregateFunction(SqlServerAggregateFunctionExpression aggregateFunctionExpression)
+    protected virtual Expression VisitSqlServerAggregateFunction(SqlServerAggregateFunctionExpression aggregateFunctionExpression)
     {
         Sql.Append(aggregateFunctionExpression.Name);
 
@@ -193,7 +193,7 @@ when tableExpression.FindAnnotation(SqlServerAnnotationNames.TemporalOperationTy
             }
 
             case SqlServerAggregateFunctionExpression aggregateFunctionExpression:
-                return VisitAggregateFunction(aggregateFunctionExpression);
+                return VisitSqlServerAggregateFunction(aggregateFunctionExpression);
         }
 
         return base.VisitExtension(extensionExpression);

src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs

@@ -98,8 +98,10 @@ protected virtual SqlExpression VisitAggregateFunction(
             }
         }
 
-        return aggregateFunctionExpression.Update(
-            arguments ?? aggregateFunctionExpression.Arguments,
-            orderings ?? aggregateFunctionExpression.Orderings);
+        return arguments is not null || orderings is not null
+            ? aggregateFunctionExpression.Update(
+                arguments ?? aggregateFunctionExpression.Arguments,
+                orderings ?? aggregateFunctionExpression.Orderings)
+            : aggregateFunctionExpression;
     }
 }