Fixes around identifier and type mappings for ValuesExpression (#33439)

Closes #33436
Closes #33438

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -366,10 +366,12 @@ JsonScalarExpression jsonScalar
     {
         var elementType = inlineQueryRootExpression.ElementType;
 
-        var rowExpressions = new List<RowValueExpression>();
         var encounteredNull = false;
         var intTypeMapping = _typeMappingSource.FindMapping(typeof(int), RelationalDependencies.Model);
+        RelationalTypeMapping? inferredTypeMaping = null;
+        var sqlExpressions = new SqlExpression[inlineQueryRootExpression.Values.Count];
 
+        // Do a first pass, translating the elements and inferring type mappings/nullability.
         for (var i = 0; i < inlineQueryRootExpression.Values.Count; i++)
         {
             // Note that we specifically don't apply the default type mapping to the translation, to allow it to get inferred later based
@@ -380,48 +382,68 @@ JsonScalarExpression jsonScalar
                 return null;
             }
 
+            // Infer the type mapping from the different inline elements, applying the type mapping of a column to constants/parameters, and
+            // also to the projection of the VALUES expression as a whole.
+            // TODO: This currently picks up the first type mapping; we can do better once we have a type compatibility chart (#15586)
+            // TODO: See similarity with SqlExpressionFactory.ApplyTypeMappingOnIn()
+            inferredTypeMaping ??= translatedValue.TypeMapping;
+
             // TODO: Poor man's null semantics: in SqlNullabilityProcessor we don't fully handle the nullability of SelectExpression
             // projections. Whether the SelectExpression's projection is nullable or not is determined here in translation, but at this
             // point we don't know how to properly calculate nullability (and can't look at parameters).
             // So for now, we assume the projected column is nullable if we see anything but non-null constants and non-nullable columns.
             encounteredNull |=
                 translatedValue is not SqlConstantExpression { Value: not null } and not ColumnExpression { IsNullable: false };
 
-            rowExpressions.Add(
+            sqlExpressions[i] = translatedValue;
+        }
+
+        // Second pass: create the VALUES expression's row value expressions.
+        var rowExpressions = new RowValueExpression[sqlExpressions.Length];
+        for (var i = 0; i < sqlExpressions.Length; i++)
+        {
+            var sqlExpression = sqlExpressions[i];
+
+            rowExpressions[i] =
                 new RowValueExpression(
                     new[]
                     {
                         // Since VALUES may not guarantee row ordering, we add an _ord value by which we'll order.
                         _sqlExpressionFactory.Constant(i, intTypeMapping),
-                        // Note that for the actual value, we must leave the type mapping null to allow it to get inferred later based on usage
-                        translatedValue
-                    }));
+                        // If no type mapping was inferred (i.e. no column in the inline collection), it's left null, to allow it to get
+                        // inferred later based on usage. Note that for the element in the VALUES expression, we'll also apply an explicit
+                        // CONVERT to make sure the database gets the right type (see
+                        // RelationalTypeMappingPostprocessor.ApplyTypeMappingsOnValuesExpression)
+                        sqlExpression.TypeMapping is null && inferredTypeMaping is not null
+                            ? _sqlExpressionFactory.ApplyTypeMapping(sqlExpression, inferredTypeMaping)
+                            : sqlExpression
+                    });
         }
 
         var alias = _sqlAliasManager.GenerateTableAlias("values");
         var valuesExpression = new ValuesExpression(alias, rowExpressions, new[] { ValuesOrderingColumnName, ValuesValueColumnName });
 
         // Note: we leave the element type mapping null, to allow it to get inferred based on queryable operators composed on top.
+        var valueColumn = new ColumnExpression(
+            ValuesValueColumnName,
+            alias,
+            elementType.UnwrapNullableType(),
+            typeMapping: inferredTypeMaping,
+            nullable: encounteredNull);
+        var orderingColumn = new ColumnExpression(
+            ValuesOrderingColumnName,
+            alias,
+            typeof(int),
+            typeMapping: intTypeMapping,
+            nullable: false);
+
         var selectExpression = new SelectExpression(
             [valuesExpression],
-            new ColumnExpression(
-                ValuesValueColumnName,
-                alias,
-                elementType.UnwrapNullableType(),
-                typeMapping: null,
-                nullable: encounteredNull),
-            identifier: [],
+            valueColumn,
+            identifier: [(orderingColumn, orderingColumn.TypeMapping!.Comparer)],
             _sqlAliasManager);
 
-        selectExpression.AppendOrdering(
-            new OrderingExpression(
-                selectExpression.CreateColumnExpression(
-                    valuesExpression,
-                    ValuesOrderingColumnName,
-                    typeof(int),
-                    intTypeMapping,
-                    columnNullable: false),
-                ascending: true));
+        selectExpression.AppendOrdering(new OrderingExpression(orderingColumn, ascending: true));
 
         Expression shaperExpression = new ProjectionBindingExpression(
             selectExpression, new ProjectionMember(), encounteredNull ? elementType.MakeNullable() : elementType);

src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs

@@ -176,17 +176,17 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp
 
                 var value = rowValue.Values[j];
 
-                var inferredTypeMapping = inferredTypeMappings[j];
-                if (inferredTypeMapping is not null && value.TypeMapping is null)
+                if (value.TypeMapping is null
+                    && inferredTypeMappings[j] is RelationalTypeMapping inferredTypeMapping)
                 {
                     value = _sqlExpressionFactory.ApplyTypeMapping(value, inferredTypeMapping);
+                }
 
-                    // We currently add explicit conversions on the first row, to ensure that the inferred types are properly typed.
-                    // See #30605 for removing that when not needed.
-                    if (i == 0)
-                    {
-                        value = new SqlUnaryExpression(ExpressionType.Convert, value, value.Type, value.TypeMapping);
-                    }
+                // We currently add explicit conversions on the first row (but not to the _ord column), to ensure that the inferred types
+                // are properly typed. See #30605 for removing that when not needed.
+                if (i == 0 && j > 0 && value is not ColumnExpression)
+                {
+                    value = new SqlUnaryExpression(ExpressionType.Convert, value, value.Type, value.TypeMapping);
                 }
 
                 newValues[j - (stripOrdering ? 1 : 0)] = value;

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

@@ -2,12 +2,10 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics.CodeAnalysis;
-using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;
 using Microsoft.EntityFrameworkCore.SqlServer.Internal;
 using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal;
-using Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal;
 
 namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;
 

test/EFCore.Relational.Specification.Tests/Query/PrimitiveCollectionsQueryRelationalTestBase.cs

@@ -43,4 +43,12 @@ public override async Task Parameter_collection_in_subquery_Union_another_parame
 
         Assert.Equal(RelationalStrings.SetOperationsRequireAtLeastOneSideWithValidTypeMapping("Union"), message);
     }
+
+    public override async Task Project_inline_collection_with_Concat(bool async)
+    {
+        var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+            () => base.Project_inline_collection_with_Concat(async))).Message;
+
+        Assert.Equal(RelationalStrings.InsufficientInformationToIdentifyElementOfCollectionJoin, message);
+    }
 }

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

@@ -928,6 +928,55 @@ public virtual Task Project_primitive_collections_element(bool async)
             },
             assertOrder: true);
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Project_inline_collection(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<PrimitiveCollectionsEntity>().Select(x => new[] { x.String, "foo" }),
+            elementAsserter: (e, a) => AssertCollection(e, a, ordered: true),
+            assertOrder: true);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Project_inline_collection_with_Union(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<PrimitiveCollectionsEntity>()
+                .Select(
+                    x => new
+                    {
+                        x.Id,
+                        Values = new[] { x.String }.Union(ss.Set<PrimitiveCollectionsEntity>().OrderBy(xx => xx.Id).Select(xx => xx.String)).ToList()
+                    })
+                .OrderBy(x => x.Id),
+            elementAsserter: (e, a) =>
+            {
+                Assert.Equal(e.Id, a.Id);
+                AssertCollection(e.Values, a.Values, ordered: false);
+            },
+            assertOrder: true);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Project_inline_collection_with_Concat(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<PrimitiveCollectionsEntity>()
+                .Select(
+                    x => new
+                    {
+                        x.Id,
+                        Values = new[] { x.String }.Concat(ss.Set<PrimitiveCollectionsEntity>().OrderBy(xx => xx.Id).Select(xx => xx.String)).ToList()
+                    })
+                .OrderBy(x => x.Id),
+            elementAsserter: (e, a) =>
+            {
+                Assert.Equal(e.Id, a.Id);
+                AssertCollection(e.Values, a.Values, ordered: false);
+            },
+            assertOrder: true);
+
     [ConditionalTheory] // #32208, #32215
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Nested_contains_with_Lists_and_no_inferred_type_mapping(bool async)

test/EFCore.Specification.Tests/TestUtilities/TestHelpers.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections;
 using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
 using Microsoft.EntityFrameworkCore.Design.Internal;
 using Microsoft.EntityFrameworkCore.Internal;
@@ -259,10 +260,21 @@ private static int AssertResults<T>(IList<T> expected, IList<T> actual)
         {
             Assert.True(
                 actual.Contains(expectedItem),
-                $"\r\nExpected item: [{expectedItem}] not found in results: [{string.Join(", ", actual.Take(10))}]...");
+                $"\r\nExpected item: '{Render(expectedItem)}' not found in results: '{string.Join(", ", actual.Take(10).Select(Render))}'...");
         }
 
         return actual.Count;
+
+        static object? Render(T element)
+        {
+            if (element is not string and IList list)
+            {
+                var start = '[' + string.Join(", ", list.ToList<object>().Take(4));
+                return list.Count > 4 ? start + "...]" : start + ']';
+            }
+
+            return element;
+        }
     }
 
     public static int AssertResults<T>(

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

@@ -871,6 +871,30 @@ public virtual async Task Same_collection_with_conflicting_type_mappings_not_sup
         Assert.Equal(RelationalStrings.ConflictingTypeMappingsInferredForColumn("value"), exception.Message);
     }
 
+    [ConditionalFact]
+    public virtual async Task Infer_inline_collection_type_mapping()
+    {
+        var contextFactory = await InitializeAsync<TestContext>(
+            onModelCreating: mb => mb.Entity<TestEntity>(b => b.Property<DateTime>("DateTime").HasColumnType("datetime")));
+
+        await using var context = contextFactory.CreateContext();
+
+        _ = await context.Set<TestEntity>()
+            .Where(b => new[] { new DateTime(2020, 1, 1), EF.Property<DateTime>(b, "DateTime") }[0] == new DateTime(2020, 1, 1))
+            .ToArrayAsync();
+
+        AssertSql(
+            """
+SELECT [t].[Id], [t].[DateTime], [t].[Ints]
+FROM [TestEntity] AS [t]
+WHERE (
+    SELECT [v].[Value]
+    FROM (VALUES (0, CAST('2020-01-01T00:00:00.000' AS datetime)), (1, [t].[DateTime])) AS [v]([_ord], [Value])
+    ORDER BY [v].[_ord]
+    OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY) = '2020-01-01T00:00:00.000'
+""");
+    }
+
     #endregion Type mapping inference
 
     [ConditionalFact]

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

@@ -792,6 +792,43 @@ ORDER BY [p].[Id]
 """);
     }
 
+    public override async Task Project_inline_collection(bool async)
+    {
+        await base.Project_inline_collection(async);
+
+        AssertSql(
+            """
+SELECT [p].[String]
+FROM [PrimitiveCollectionsEntity] AS [p]
+""");
+    }
+
+    public override async Task Project_inline_collection_with_Union(bool async)
+    {
+        await base.Project_inline_collection_with_Union(async);
+
+        AssertSql(
+            """
+SELECT [p].[Id], [u].[Value]
+FROM [PrimitiveCollectionsEntity] AS [p]
+OUTER APPLY (
+    SELECT [v].[Value]
+    FROM (VALUES ([p].[String])) AS [v]([Value])
+    UNION
+    SELECT [p0].[String] AS [Value]
+    FROM [PrimitiveCollectionsEntity] AS [p0]
+) AS [u]
+ORDER BY [p].[Id]
+""");
+    }
+
+    public override async Task Project_inline_collection_with_Concat(bool async)
+    {
+        await base.Project_inline_collection_with_Concat(async);
+
+        AssertSql();
+    }
+
     public override async Task Nested_contains_with_Lists_and_no_inferred_type_mapping(bool async)
     {
         await base.Nested_contains_with_Lists_and_no_inferred_type_mapping(async);

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

@@ -1470,6 +1470,43 @@ ORDER BY [p].[Id]
 """);
     }
 
+    public override async Task Project_inline_collection(bool async)
+    {
+        await base.Project_inline_collection(async);
+
+        AssertSql(
+            """
+SELECT [p].[String]
+FROM [PrimitiveCollectionsEntity] AS [p]
+""");
+    }
+
+    public override async Task Project_inline_collection_with_Union(bool async)
+    {
+        await base.Project_inline_collection_with_Union(async);
+
+        AssertSql(
+            """
+SELECT [p].[Id], [u].[Value]
+FROM [PrimitiveCollectionsEntity] AS [p]
+OUTER APPLY (
+    SELECT [v].[Value]
+    FROM (VALUES ([p].[String])) AS [v]([Value])
+    UNION
+    SELECT [p0].[String] AS [Value]
+    FROM [PrimitiveCollectionsEntity] AS [p0]
+) AS [u]
+ORDER BY [p].[Id]
+""");
+    }
+
+    public override async Task Project_inline_collection_with_Concat(bool async)
+    {
+        await base.Project_inline_collection_with_Concat(async);
+
+        AssertSql();
+    }
+
     public override async Task Nested_contains_with_Lists_and_no_inferred_type_mapping(bool async)
     {
         await base.Nested_contains_with_Lists_and_no_inferred_type_mapping(async);

test/EFCore.Sqlite.FunctionalTests/Query/PrimitiveCollectionsQuerySqliteTest.cs

@@ -1333,6 +1333,30 @@ ORDER BY "p"."Id"
 """);
     }
 
+    public override async Task Project_inline_collection(bool async)
+    {
+        await base.Project_inline_collection(async);
+
+        AssertSql(
+            """
+SELECT "p"."String"
+FROM "PrimitiveCollectionsEntity" AS "p"
+""");
+    }
+
+    public override async Task Project_inline_collection_with_Union(bool async)
+        => Assert.Equal(
+            SqliteStrings.ApplyNotSupported,
+            (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => base.Project_inline_collection_with_Union(async))).Message);
+
+    public override async Task Project_inline_collection_with_Concat(bool async)
+    {
+        await base.Project_inline_collection_with_Concat(async);
+
+        AssertSql();
+    }
+
     public override async Task Project_empty_collection_of_nullables_and_collection_only_containing_nulls(bool async)
         => Assert.Equal(
             SqliteStrings.ApplyNotSupported,