From 2b507ab996c4644da5d25475a11e1181f0e0c159 Mon Sep 17 00:00:00 2001 From: Shay Rojansky <roji@roji.org> Date: Sat, 30 Mar 2024 16:57:04 +0100 Subject: [PATCH] Fixes around identifier and type mappings for ValuesExpression Closes #33436 Closes #33438 --- ...yableMethodTranslatingExpressionVisitor.cs | 64 +++++++++++++------ .../RelationalTypeMappingPostprocessor.cs | 16 ++--- ...yableMethodTranslatingExpressionVisitor.cs | 2 - ...itiveCollectionsQueryRelationalTestBase.cs | 8 +++ .../PrimitiveCollectionsQueryTestBase.cs | 49 ++++++++++++++ .../TestUtilities/TestHelpers.cs | 14 +++- ...dPrimitiveCollectionsQuerySqlServerTest.cs | 24 +++++++ ...imitiveCollectionsQueryOldSqlServerTest.cs | 37 +++++++++++ .../PrimitiveCollectionsQuerySqlServerTest.cs | 37 +++++++++++ .../PrimitiveCollectionsQuerySqliteTest.cs | 24 +++++++ 10 files changed, 243 insertions(+), 32 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs index 7d0076c0598..d9ff7dce69b 100644 --- a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs +++ b/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,6 +382,12 @@ 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). @@ -387,41 +395,55 @@ JsonScalarExpression jsonScalar 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); diff --git a/src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs b/src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs index 3eb0976123e..03eaaefeb14 100644 --- a/src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs +++ b/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; diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs index 3bedb9a1008..a211a10ab16 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs +++ b/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; diff --git a/test/EFCore.Relational.Specification.Tests/Query/PrimitiveCollectionsQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/PrimitiveCollectionsQueryRelationalTestBase.cs index b8fb93c9377..e1cf534ae07 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/PrimitiveCollectionsQueryRelationalTestBase.cs +++ b/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); + } } diff --git a/test/EFCore.Specification.Tests/Query/PrimitiveCollectionsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/PrimitiveCollectionsQueryTestBase.cs index f60fe2f02f6..384a8db4a43 100644 --- a/test/EFCore.Specification.Tests/Query/PrimitiveCollectionsQueryTestBase.cs +++ b/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) diff --git a/test/EFCore.Specification.Tests/TestUtilities/TestHelpers.cs b/test/EFCore.Specification.Tests/TestUtilities/TestHelpers.cs index 132ed15072e..712c975573f 100644 --- a/test/EFCore.Specification.Tests/TestUtilities/TestHelpers.cs +++ b/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>( diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NonSharedPrimitiveCollectionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NonSharedPrimitiveCollectionsQuerySqlServerTest.cs index e35cd767d91..a956cb2131e 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NonSharedPrimitiveCollectionsQuerySqlServerTest.cs +++ b/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] diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQueryOldSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQueryOldSqlServerTest.cs index 6548d9f8b9e..80fdfacd6e1 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQueryOldSqlServerTest.cs +++ b/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); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerTest.cs index be85a4c5000..1c174b1ab8e 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerTest.cs +++ b/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); diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/PrimitiveCollectionsQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/PrimitiveCollectionsQuerySqliteTest.cs index bb9420b02a0..840fd16fed7 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/PrimitiveCollectionsQuerySqliteTest.cs +++ b/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,