Translate COALESCE as ISNULL in more cases

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

@@ -213,37 +213,62 @@ protected override Expression VisitSqlFunction(SqlFunctionExpression sqlFunction
         if (sqlFunctionExpression is { IsBuiltIn: true, Arguments: not null }
             && string.Equals(sqlFunctionExpression.Name, "COALESCE", StringComparison.OrdinalIgnoreCase))
         {
-            var type = sqlFunctionExpression.Type;
-            var typeMapping = sqlFunctionExpression.TypeMapping;
-            var defaultTypeMapping = _typeMappingSource.FindMapping(type);
-
             // ISNULL always return a value having the same type as its first
             // argument. Ideally we would convert the argument to have the
             // desired type and type mapping, but currently EFCore has some
             // trouble in computing types of non-homogeneous expressions
-            // (tracked in https://github.com/dotnet/efcore/issues/15586). To
-            // stay on the safe side we only use ISNULL if:
-            //  - all sub-expressions have the same type as the expression
-            //  - all sub-expressions have the same type mapping as the expression
-            //  - the expression is using the default type mapping (combined
-            //    with the two above, this implies that all of the expressions
-            //    are using the default type mapping of the type)
-            if (defaultTypeMapping == typeMapping
-                && sqlFunctionExpression.Arguments.All(a => a.Type == type && a.TypeMapping == typeMapping)) {
-
-                var head = sqlFunctionExpression.Arguments[0];
-                sqlFunctionExpression = (SqlFunctionExpression)sqlFunctionExpression
-                    .Arguments
-                    .Skip(1)
-                    .Aggregate(head, (l, r) => new SqlFunctionExpression(
-                        "ISNULL",
-                        arguments: [l, r],
-                        nullable: true,
-                        argumentsPropagateNullability: [false, false],
-                        sqlFunctionExpression.Type,
-                        sqlFunctionExpression.TypeMapping
-                    ));
+            // (tracked in https://github.com/dotnet/efcore/issues/15586).
+            //
+            // The main issue is the sizing of the type. Since sometimes the
+            // computed size is wrong, stay on the safe side by expanding to the
+            // maximum supported size with an approach similar to that used in
+            // SqlServerStringAggregateMethodTranslator. This might result in
+            // unneeded conversions, but should produce the correct results.
+            var forceCast = sqlFunctionExpression.TypeMapping?.StoreTypeNameBase is
+                "nvarchar" or "varchar" or "varbinary";
+
+            var typeMapping = sqlFunctionExpression.TypeMapping switch
+            {
+                { StoreTypeNameBase: "nvarchar", Size: >= 0 and < 4000 } => _typeMappingSource.FindMapping(
+                    typeof(string),
+                    sqlFunctionExpression.TypeMapping.StoreTypeNameBase,
+                    unicode: true,
+                    size: 4000),
+                { StoreTypeNameBase: "varchar" or "varbinary", Size: >= 0 and < 8000 } => _typeMappingSource.FindMapping(
+                    typeof(string),
+                    sqlFunctionExpression.TypeMapping.StoreTypeNameBase,
+                    unicode: false,
+                    size: 8000),
+                var t => t,
+            };
+
+            var result = sqlFunctionExpression.Arguments[0];
+            if (forceCast || result.TypeMapping?.StoreType != typeMapping?.StoreType)
+            {
+                result = new SqlUnaryExpression(
+                    ExpressionType.Convert,
+                    result,
+                    sqlFunctionExpression.Type,
+                    typeMapping
+                );
             }
+
+            var length = sqlFunctionExpression.Arguments.Count;
+            for (var i = 1; i < length; i++)
+            {
+                // propagate type and type mapping from the first argument,
+                // nullability from COALESCE
+                result = new SqlFunctionExpression(
+                    "ISNULL",
+                    arguments: [result, sqlFunctionExpression.Arguments[i]],
+                    nullable: i == length - 1 ? sqlFunctionExpression.IsNullable : true,
+                    argumentsPropagateNullability: [false, false],
+                    result.Type,
+                    result.TypeMapping
+                );
+            }
+
+            sqlFunctionExpression = (SqlFunctionExpression)result;
         }
 
         return base.VisitSqlFunction(sqlFunctionExpression);

test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs

@@ -3269,6 +3269,38 @@ public override Task Coalesce_Correct_TypeMapping_String(bool async)
 SELECT VALUE ((c["Region"] != null) ? c["Region"] : "no region specified")
 FROM root c
 ORDER BY c["id"]
+""");
+            });
+
+    public override Task Coalesce_Correct_TypeMapping_String_Sum(bool async)
+        => Fixture.NoSyncTest(
+            async, async a =>
+            {
+                await base.Coalesce_Correct_TypeMapping_String_Sum(async);
+
+                AssertSql(
+                    """
+SELECT VALUE ((((c["Region"] != null) ? ("R" || c["Region"]) : null) != null) ? ((c["Region"] != null) ? ("R" || c["Region"]) : null) : "no region specified")
+FROM root c
+ORDER BY c["id"]
+""");
+            });
+
+    public override Task Coalesce_Correct_TypeMapping_String_Join(bool async)
+        => Fixture.NoSyncTest(
+            async, async a =>
+            {
+                await base.Coalesce_Correct_TypeMapping_String_Join(async);
+
+                AssertSql(
+                    """
+SELECT VALUE
+{
+    "c" : (c["Region"] != null),
+    "c0" : ["R", c["Region"]]
+}
+FROM root c
+ORDER BY c["id"]
 """);
             });
 
@@ -3390,7 +3422,7 @@ public override async Task SelectMany_primitive_select_subquery(bool async)
         // Cosmos client evaluation. Issue #17246.
         Assert.Equal(
             CoreStrings.ExpressionParameterizationExceptionSensitive(
-                "value(Microsoft.EntityFrameworkCore.Query.NorthwindMiscellaneousQueryTestBase`1+<>c__DisplayClass175_0[Microsoft.EntityFrameworkCore.Query.NorthwindQueryCosmosFixture`1[Microsoft.EntityFrameworkCore.TestUtilities.NoopModelCustomizer]]).ss.Set().Any()"),
+                "value(Microsoft.EntityFrameworkCore.Query.NorthwindMiscellaneousQueryTestBase`1+<>c__DisplayClass177_0[Microsoft.EntityFrameworkCore.Query.NorthwindQueryCosmosFixture`1[Microsoft.EntityFrameworkCore.TestUtilities.NoopModelCustomizer]]).ss.Set().Any()"),
             (await Assert.ThrowsAsync<InvalidOperationException>(
                 () => base.SelectMany_primitive_select_subquery(async))).Message);
 

test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs

@@ -748,6 +748,23 @@ public virtual Task Coalesce_Correct_TypeMapping_String(bool async)
             ss => ss.Set<Customer>().OrderBy(c => c.CustomerID).Select(c => c.Region ?? "no region specified"),
             assertOrder: true);
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Coalesce_Correct_TypeMapping_String_Sum(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Customer>().OrderBy(c => c.CustomerID).Select(c => (c.Region != null ? "R" + c.Region : null) ?? "no region specified"),
+            assertOrder: true);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Coalesce_Correct_TypeMapping_String_Join(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Customer>().OrderBy(c => c.CustomerID)
+                .Select(c => (c.Region != null ? string.Join("|", new[] { "R", c.Region }) : null) ?? "no region specified"),
+            assertOrder: true);
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Null_Coalesce_Short_Circuit(bool async)

test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs

@@ -98,7 +98,7 @@ public override async Task Update_non_owned_property_on_entity_with_owned2(bool
         AssertSql(
             """
 UPDATE [o]
-SET [o].[Title] = COALESCE([o].[Title], N'') + N'_Suffix'
+SET [o].[Title] = ISNULL(CAST([o].[Title] AS nvarchar(max)), N'') + N'_Suffix'
 FROM [Owner] AS [o]
 """);
     }
@@ -125,7 +125,7 @@ public override async Task Update_owned_and_non_owned_properties_with_table_shar
         AssertSql(
             """
 UPDATE [o]
-SET [o].[Title] = COALESCE(CONVERT(varchar(11), [o].[OwnedReference_Number]), ''),
+SET [o].[Title] = ISNULL(CAST(CONVERT(varchar(11), [o].[OwnedReference_Number]) AS varchar(8000)), ''),
     [o].[OwnedReference_Number] = CAST(LEN([o].[Title]) AS int)
 FROM [Owner] AS [o]
 """);
@@ -190,7 +190,7 @@ public override async Task Update_with_alias_uniquification_in_setter_subquery(b
             """
 UPDATE [o]
 SET [o].[Total] = (
-    SELECT COALESCE(SUM([o0].[Amount]), 0)
+    SELECT ISNULL(SUM([o0].[Amount]), 0)
     FROM [OrderProduct] AS [o0]
     WHERE [o].[Id] = [o0].[OrderId])
 FROM [Orders] AS [o]

test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NorthwindBulkUpdatesSqlServerTest.cs

@@ -1110,7 +1110,7 @@ public override async Task Update_Where_set_property_plus_constant(bool async)
         AssertExecuteUpdateSql(
             """
 UPDATE [c]
-SET [c].[ContactName] = COALESCE([c].[ContactName], N'') + N'Abc'
+SET [c].[ContactName] = ISNULL(CAST([c].[ContactName] AS nvarchar(4000)), N'') + N'Abc'
 FROM [Customers] AS [c]
 WHERE [c].[CustomerID] LIKE N'F%'
 """);
@@ -1125,7 +1125,7 @@ public override async Task Update_Where_set_property_plus_parameter(bool async)
 @value='Abc' (Size = 4000)
 
 UPDATE [c]
-SET [c].[ContactName] = COALESCE([c].[ContactName], N'') + @value
+SET [c].[ContactName] = ISNULL(CAST([c].[ContactName] AS nvarchar(4000)), N'') + @value
 FROM [Customers] AS [c]
 WHERE [c].[CustomerID] LIKE N'F%'
 """);
@@ -1138,7 +1138,7 @@ public override async Task Update_Where_set_property_plus_property(bool async)
         AssertExecuteUpdateSql(
             """
 UPDATE [c]
-SET [c].[ContactName] = COALESCE([c].[ContactName], N'') + [c].[CustomerID]
+SET [c].[ContactName] = ISNULL(CAST([c].[ContactName] AS nvarchar(4000)), N'') + [c].[CustomerID]
 FROM [Customers] AS [c]
 WHERE [c].[CustomerID] LIKE N'F%'
 """);
@@ -1560,11 +1560,11 @@ public override async Task Update_Where_Join_set_property_from_joined_single_res
         AssertExecuteUpdateSql(
             """
 UPDATE [c]
-SET [c].[City] = COALESCE(CONVERT(varchar(11), DATEPART(year, (
+SET [c].[City] = ISNULL(CAST(CONVERT(varchar(11), DATEPART(year, (
     SELECT TOP(1) [o].[OrderDate]
     FROM [Orders] AS [o]
     WHERE [c].[CustomerID] = [o].[CustomerID]
-    ORDER BY [o].[OrderDate] DESC))), '')
+    ORDER BY [o].[OrderDate] DESC))) AS varchar(8000)), '')
 FROM [Customers] AS [c]
 WHERE [c].[CustomerID] LIKE N'F%'
 """);
@@ -1595,11 +1595,11 @@ public override async Task Update_Where_Join_set_property_from_joined_single_res
         AssertExecuteUpdateSql(
             """
 UPDATE [c]
-SET [c].[City] = COALESCE(CONVERT(varchar(11), DATEPART(year, (
+SET [c].[City] = ISNULL(CAST(CONVERT(varchar(11), DATEPART(year, (
     SELECT TOP(1) [o].[OrderDate]
     FROM [Orders] AS [o]
     WHERE [c].[CustomerID] = [o].[CustomerID]
-    ORDER BY [o].[OrderDate] DESC))), '')
+    ORDER BY [o].[OrderDate] DESC))) AS varchar(8000)), '')
 FROM [Customers] AS [c]
 WHERE [c].[CustomerID] LIKE N'F%'
 """);

test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs

@@ -2202,7 +2202,7 @@ public override async Task Enum_with_value_converter_matching_take_value(bool as
 @orderItemType='MyType1' (Nullable = false) (Size = 4000)
 @p='1'
 
-SELECT [o1].[Id], COALESCE((
+SELECT [o1].[Id], ISNULL((
     SELECT TOP(1) [o3].[Price]
     FROM [OrderItems] AS [o3]
     WHERE [o1].[Id] = [o3].[OrderId] AND [o3].[Type] = @orderItemType), 0.0E0) AS [SpecialSum]
@@ -2284,8 +2284,8 @@ public override async Task Group_by_aggregate_in_subquery_projection_after_group
 
         AssertSql(
             """
-SELECT [t].[Value] AS [A], COALESCE(SUM([t].[Id]), 0) AS [B], COALESCE((
-    SELECT TOP(1) COALESCE(SUM([t].[Id]), 0) + COALESCE(SUM([t0].[Id]), 0)
+SELECT [t].[Value] AS [A], ISNULL(SUM([t].[Id]), 0) AS [B], ISNULL((
+    SELECT TOP(1) ISNULL(SUM([t].[Id]), 0) + ISNULL(SUM([t0].[Id]), 0)
     FROM [Tables] AS [t0]
     GROUP BY [t0].[Value]
     ORDER BY (SELECT 1)), 0) AS [C]

test/EFCore.SqlServer.FunctionalTests/Query/AdHocNavigationsQuerySqlServerTest.cs

@@ -337,14 +337,14 @@ FROM [CompetitionSeasons] AS [c1]
 SELECT [a].[Id], [a].[ActivityTypeId], [a].[DateTime], [a].[Points], (
     SELECT TOP(1) [c].[Id]
     FROM [CompetitionSeasons] AS [c]
-    WHERE [c].[StartDate] <= [a].[DateTime] AND [a].[DateTime] < [c].[EndDate]) AS [CompetitionSeasonId], COALESCE([a].[Points], (
+    WHERE [c].[StartDate] <= [a].[DateTime] AND [a].[DateTime] < [c].[EndDate]) AS [CompetitionSeasonId], ISNULL(ISNULL([a].[Points], (
     SELECT TOP(1) [a1].[Points]
     FROM [ActivityTypePoints] AS [a1]
     INNER JOIN [CompetitionSeasons] AS [c0] ON [a1].[CompetitionSeasonId] = [c0].[Id]
     WHERE [a0].[Id] = [a1].[ActivityTypeId] AND [c0].[Id] = (
         SELECT TOP(1) [c1].[Id]
         FROM [CompetitionSeasons] AS [c1]
-        WHERE [c1].[StartDate] <= [a].[DateTime] AND [a].[DateTime] < [c1].[EndDate])), 0) AS [Points]
+        WHERE [c1].[StartDate] <= [a].[DateTime] AND [a].[DateTime] < [c1].[EndDate]))), 0) AS [Points]
 FROM [Activities] AS [a]
 INNER JOIN [ActivityType] AS [a0] ON [a].[ActivityTypeId] = [a0].[Id]
 """);

test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServer160Test.cs

@@ -1099,7 +1099,7 @@ public override async Task Result_operator_nav_prop_reference_optional_Sum(bool
 
         AssertSql(
             """
-SELECT COALESCE(SUM([l0].[Level1_Required_Id]), 0)
+SELECT ISNULL(SUM([l0].[Level1_Required_Id]), 0)
 FROM [LevelOne] AS [l]
 LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id]
 """);
@@ -1171,7 +1171,7 @@ public override async Task Result_operator_nav_prop_reference_optional_via_Defau
 
         AssertSql(
             """
-SELECT COALESCE(SUM(CASE
+SELECT ISNULL(SUM(CASE
     WHEN [l0].[Id] IS NULL THEN 0
     ELSE [l0].[Level1_Required_Id]
 END), 0)
@@ -2031,7 +2031,7 @@ public override async Task Select_join_with_key_selector_being_a_subquery(bool a
             """
 SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id], [l0].[Id], [l0].[Date], [l0].[Level1_Optional_Id], [l0].[Level1_Required_Id], [l0].[Name], [l0].[OneToMany_Optional_Inverse2Id], [l0].[OneToMany_Optional_Self_Inverse2Id], [l0].[OneToMany_Required_Inverse2Id], [l0].[OneToMany_Required_Self_Inverse2Id], [l0].[OneToOne_Optional_PK_Inverse2Id], [l0].[OneToOne_Optional_Self2Id]
 FROM [LevelOne] AS [l]
-INNER JOIN [LevelTwo] AS [l0] ON [l].[Id] = COALESCE((
+INNER JOIN [LevelTwo] AS [l0] ON [l].[Id] = ISNULL((
     SELECT TOP(1) [l1].[Id]
     FROM [LevelTwo] AS [l1]
     ORDER BY [l1].[Id]), 0)
@@ -2943,10 +2943,10 @@ public override async Task Select_optional_navigation_property_string_concat(boo
 
         AssertSql(
             """
-SELECT COALESCE([l].[Name], N'') + N' ' + COALESCE(CASE
+SELECT ISNULL(CAST([l].[Name] AS nvarchar(max)), N'') + N' ' + ISNULL(CAST(CASE
     WHEN [l1].[Id] IS NOT NULL THEN [l1].[Name]
     ELSE N'NULL'
-END, N'')
+END AS nvarchar(max)), N'')
 FROM [LevelOne] AS [l]
 LEFT JOIN (
     SELECT [l0].[Id], [l0].[Name], [l0].[OneToMany_Optional_Inverse2Id]
@@ -3807,7 +3807,7 @@ public override async Task Sum_with_selector_cast_using_as(bool async)
 
         AssertSql(
             """
-SELECT COALESCE(SUM([l].[Id]), 0)
+SELECT ISNULL(SUM([l].[Id]), 0)
 FROM [LevelOne] AS [l]
 """);
     }
@@ -3821,7 +3821,7 @@ public override async Task Sum_with_filter_with_include_selector_cast_using_as(b
 SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
 FROM [LevelOne] AS [l]
 WHERE [l].[Id] > (
-    SELECT COALESCE(SUM([l0].[Id]), 0)
+    SELECT ISNULL(SUM([l0].[Id]), 0)
     FROM [LevelTwo] AS [l0]
     WHERE [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id])
 """);
@@ -3965,7 +3965,7 @@ FROM [LevelOne] AS [l]
 LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Id]
 GROUP BY [l1].[Name]
 HAVING (
-    SELECT MIN(COALESCE([l5].[Id], 0) + COALESCE([l5].[Id], 0))
+    SELECT MIN(ISNULL([l5].[Id], 0) + ISNULL([l5].[Id], 0))
     FROM [LevelOne] AS [l2]
     LEFT JOIN [LevelTwo] AS [l3] ON [l2].[Id] = [l3].[Id]
     LEFT JOIN [LevelThree] AS [l4] ON [l3].[Id] = [l4].[Id]
@@ -4106,7 +4106,7 @@ public override async Task Composite_key_join_on_groupby_aggregate_projecting_on
 SELECT [l2].[Key]
 FROM [LevelOne] AS [l]
 INNER JOIN (
-    SELECT [l1].[Key], COALESCE(SUM([l1].[Id]), 0) AS [Sum]
+    SELECT [l1].[Key], ISNULL(SUM([l1].[Id]), 0) AS [Sum]
     FROM (
         SELECT [l0].[Id], [l0].[Id] % 3 AS [Key]
         FROM [LevelTwo] AS [l0]
@@ -4125,7 +4125,7 @@ public override async Task Composite_key_join_on_groupby_aggregate_projecting_on
 SELECT [l2].[Key]
 FROM [LevelOne] AS [l]
 INNER JOIN (
-    SELECT [l1].[Key], COALESCE(SUM([l1].[Id]), 0) AS [Sum]
+    SELECT [l1].[Key], ISNULL(SUM([l1].[Id]), 0) AS [Sum]
     FROM (
         SELECT [l0].[Id], [l0].[Id] % 3 AS [Key]
         FROM [LevelTwo] AS [l0]
@@ -4341,7 +4341,7 @@ WHERE [l].[Id] < 5
 CROSS APPLY (
     SELECT [l1].[Id], [l1].[Date], [l1].[Name], [l1].[OneToMany_Optional_Self_Inverse1Id], [l1].[OneToMany_Required_Self_Inverse1Id], [l1].[OneToOne_Optional_Self1Id]
     FROM [LevelOne] AS [l1]
-    WHERE [l1].[Id] <> COALESCE([l0].[Level1_Required_Id], 0)
+    WHERE [l1].[Id] <> ISNULL([l0].[Level1_Required_Id], 0)
 ) AS [l2]
 """);
     }
@@ -4449,7 +4449,7 @@ FROM [LevelOne] AS [l]
 LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Id]
 GROUP BY [l1].[Name]
 HAVING (
-    SELECT MIN(COALESCE([l5].[Id], 0))
+    SELECT MIN(ISNULL([l5].[Id], 0))
     FROM [LevelOne] AS [l2]
     LEFT JOIN [LevelTwo] AS [l3] ON [l2].[Id] = [l3].[Id]
     LEFT JOIN [LevelThree] AS [l4] ON [l3].[Id] = [l4].[Id]
@@ -4831,7 +4831,7 @@ ORDER BY [l].[Id]
 ) AS [l4]
 LEFT JOIN (
     SELECT [l0].[Id], [l1].[Id] AS [Id0], [l2].[Id] AS [Id1], CASE
-        WHEN COALESCE((
+        WHEN ISNULL((
             SELECT MAX([l3].[Id])
             FROM [LevelFour] AS [l3]
             WHERE [l1].[Id] IS NOT NULL AND [l1].[Id] = [l3].[OneToMany_Optional_Inverse4Id]), 0) > 1 THEN CAST(1 AS bit)