From 918ce1a56619898a77139d618a8032496228134e Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Thu, 27 Jun 2019 18:23:00 +0200 Subject: [PATCH] Rolled back ColumnExpression changes Disabled support for siblings of differing entity types, #16298 --- .../Query/Pipeline/QuerySqlGenerator.cs | 8 +- .../SqlExpressions/ColumnExpression.cs | 8 +- .../SqlExpressions/SelectExpression.cs | 123 +++--------------- .../SimpleQueryCosmosTest.ResultOperators.cs | 10 -- .../Query/InheritanceTestBase.cs | 6 +- .../SimpleQueryTestBase.SetOperations.cs | 2 +- 6 files changed, 27 insertions(+), 130 deletions(-) diff --git a/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs index fe3937fce86..ea528ba5e10 100644 --- a/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs +++ b/src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs @@ -236,13 +236,9 @@ protected override Expression VisitSqlFunction(SqlFunctionExpression sqlFunction protected override Expression VisitColumn(ColumnExpression columnExpression) { - if (columnExpression.Table.Alias != null) - { - _relationalCommandBuilder - .Append(_sqlGenerationHelper.DelimitIdentifier(columnExpression.Table.Alias)) - .Append("."); - } _relationalCommandBuilder + .Append(_sqlGenerationHelper.DelimitIdentifier(columnExpression.Table.Alias)) + .Append(".") .Append(_sqlGenerationHelper.DelimitIdentifier(columnExpression.Name)); return columnExpression; diff --git a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/ColumnExpression.cs b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/ColumnExpression.cs index 9ee29ddd44c..eb9e8a5e366 100644 --- a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/ColumnExpression.cs +++ b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/ColumnExpression.cs @@ -7,6 +7,7 @@ using Microsoft.EntityFrameworkCore.Metadata; using Microsoft.EntityFrameworkCore.Query.Internal; using Microsoft.EntityFrameworkCore.Storage; +using Microsoft.EntityFrameworkCore.Utilities; namespace Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.SqlExpressions { @@ -27,6 +28,10 @@ internal ColumnExpression(ProjectionExpression subqueryProjection, TableExpressi private ColumnExpression(string name, TableExpressionBase table, Type type, RelationalTypeMapping typeMapping, bool nullable) : base(type, typeMapping) { + Check.NotEmpty(name, nameof(name)); + Check.NotNull(table, nameof(table)); + Check.NotEmpty(table.Alias, $"{nameof(table)}.{nameof(table.Alias)}"); + Name = name; Table = table; Nullable = nullable; @@ -71,7 +76,6 @@ private bool Equals(ColumnExpression columnExpression) public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), Name, Table, Nullable); - private string DebuggerDisplay() - => Table.Alias == null ? Name : $"{Table.Alias}.{Name}"; + private string DebuggerDisplay() => $"{Table.Alias}.{Name}"; } } diff --git a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs index 711a983bb01..162c396a36a 100644 --- a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs @@ -373,7 +373,7 @@ public Expression ApplySetOperation( if (_projection.Any()) { - throw new NotSupportedException("Can't process set operations after client evaluation, consider moving the operation before the last Select() call (see issue #16243)"); + throw new InvalidOperationException("Can't process set operations after client evaluation, consider moving the operation before the last Select() call (see issue #16243)"); } else { @@ -389,7 +389,6 @@ public Expression ApplySetOperation( kv => kv.Key, (kv1, kv2) => (kv1.Key, Value1: kv1.Value, Value2: kv2.Value))) { - if (joinedMapping.Value1 is EntityProjectionExpression entityProjection1 && joinedMapping.Value2 is EntityProjectionExpression entityProjection2) { @@ -407,7 +406,7 @@ public Expression ApplySetOperation( continue; } - throw new NotSupportedException("Non-matching or unknown projection mapping type in set operation"); + throw new InvalidOperationException("Non-matching or unknown projection mapping type in set operation"); } } @@ -443,63 +442,7 @@ void HandleEntityMapping( return; } - // We're doing a set operation over two different entity types (within the same hierarchy). - // Since both sides of the set operations must produce the same result shape, find the - // closest common ancestor and load all the columns for that, adding null projections where - // necessary. Note this means we add null projections for properties which neither sibling - // actually needs, since the shaper doesn't know that only those sibling types will be coming - // back. - var commonParentEntityType = projection1.EntityType.GetClosestCommonParent(projection2.EntityType); - - if (commonParentEntityType == null) - { - throw new NotSupportedException(RelationalStrings.SetOperationNotWithinEntityTypeHierarchy); - } - - var properties1 = GetAllPropertiesInHierarchy(projection1.EntityType).ToArray(); - var properties2 = GetAllPropertiesInHierarchy(projection2.EntityType).ToArray(); - - foreach (var property in properties1.Intersect(properties2)) - { - propertyExpressions[property] = AddSetOperationColumnProjections( - property, - select1, projection1.GetProperty(property), - select2, projection2.GetProperty(property)); - } - - foreach (var property in properties1.Except(properties2)) - { - propertyExpressions[property] = AddSetOperationColumnProjections( - property, - select1,projection1.GetProperty(property), - select2, null); - } - - foreach (var property in properties2.Except(properties1)) - { - propertyExpressions[property] = AddSetOperationColumnProjections( - property, - select1, null, - select2, projection2.GetProperty(property)); - } - - foreach (var property in GetAllPropertiesInHierarchy(commonParentEntityType) - .Except(properties1).Except(properties2)) - { - propertyExpressions[property] = AddSetOperationColumnProjections( - property, - select1, null, - select2, null); - } - - _projectionMapping[projectionMember] = new EntityProjectionExpression(commonParentEntityType, propertyExpressions); - - if (commonParentEntityType != projection1.EntityType) - { - // The first source has been up-cast by the set operation, so we also need to change the shaper expression. - // The EntityShaperExpression may be buried under Convert nodes produced by Cast operators, preserve those. - shaperExpression = UpdateEntityShaperEntityType(shaperExpression, commonParentEntityType); - } + throw new InvalidOperationException("Set operations over different entity types are currently unsupported (see #16298)"); } ColumnExpression AddSetOperationColumnProjections( @@ -507,35 +450,17 @@ ColumnExpression AddSetOperationColumnProjections( SelectExpression select1, ColumnExpression column1, SelectExpression select2, ColumnExpression column2) { - var columnName = column1?.Name ?? column2?.Name ?? property.Name; - var baseColumnName = columnName; - var counter = 0; - while (select1._projection.Any(pe => string.Equals(pe.Alias, columnName, StringComparison.OrdinalIgnoreCase))) - { - columnName = $"{baseColumnName}{counter++}"; - } - - var typeMapping = column1?.TypeMapping ?? column2?.TypeMapping ?? property.FindRelationalMapping(); - - select1._projection.Add(new ProjectionExpression(column1 != null - ? (SqlExpression)column1 - : new SqlConstantExpression(Constant(null), typeMapping), - columnName)); + var columnName = column1.Name; - select2._projection.Add(new ProjectionExpression(column2 != null - ? (SqlExpression)column2 - : new SqlConstantExpression(Constant(null), typeMapping), - columnName)); - - var projectionExpression = select1._projection[select1._projection.Count - 1]; - var outerColumn = new ColumnExpression(projectionExpression, select1, IsNullableProjection(projectionExpression)); + select1._projection.Add(new ProjectionExpression(column1, columnName)); + select2._projection.Add(new ProjectionExpression(column2, columnName)); if (select1._identifyingProjection.Contains(column1)) { - _identifyingProjection.Add(outerColumn); + _identifyingProjection.Add(column1); } - return outerColumn; + return column1; } void HandleColumnMapping( @@ -546,29 +471,9 @@ void HandleColumnMapping( // The actual columns may actually be different, but we don't care as long as the type and alias // coming out of the two operands are the same var alias = projectionMember.LastMember?.Name; - var index = select1.AddToProjection(innerColumn1, alias); - var projectionExpression1 = select1._projection[index]; + select1.AddToProjection(innerColumn1, alias); select2.AddToProjection(innerColumn2, alias); - var outerColumn = new ColumnExpression(projectionExpression1, select1, IsNullableProjection(projectionExpression1)); - _projectionMapping[projectionMember] = outerColumn; - } - - static Expression UpdateEntityShaperEntityType(Expression shaperExpression, IEntityType newEntityType) - { - switch (shaperExpression) - { - case EntityShaperExpression entityShaperExpression: - return new EntityShaperExpression(newEntityType, entityShaperExpression.ValueBufferExpression, entityShaperExpression.Nullable); - case UnaryExpression unary when unary.NodeType == ExpressionType.Convert: - var newShaperExpression = UpdateEntityShaperEntityType(unary.Operand, newEntityType); - return newShaperExpression.Type == unary.Type - ? newShaperExpression - : Convert(newShaperExpression, unary.Type); - case UnaryExpression unary when unary.NodeType == ExpressionType.ConvertChecked: - return ConvertChecked(UpdateEntityShaperEntityType(unary.Operand, newEntityType), unary.Type); - default: - throw new Exception($"Unexpected expression type {shaperExpression.GetType().Name} encountered in {nameof(UpdateEntityShaperEntityType)}"); - } + _projectionMapping[projectionMember] = innerColumn1; } } @@ -901,10 +806,12 @@ private SqlBinaryExpression ValidateKeyComparison(SelectExpression inner, SqlBin return null; } + // We treat a set operation as a transparent wrapper over its left operand (the ColumnExpression projection mappings + // found on a set operation SelectExpression are actually those of its left operand). private bool ContainsTableReference(TableExpressionBase table) - { - return _tables.Any(te => ReferenceEquals(te is JoinExpressionBase jeb ? jeb.Table : te, table)); - } + => IsSetOperation + ? ((SelectExpression)Tables[0]).ContainsTableReference(table) + : Tables.Any(te => ReferenceEquals(te is JoinExpressionBase jeb ? jeb.Table : te, table)); public void AddInnerJoin(SelectExpression innerSelectExpression, SqlExpression joinPredicate, Type transparentIdentifierType) { diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.ResultOperators.cs b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.ResultOperators.cs index 1a9910ce14b..dc65f700d6f 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.ResultOperators.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.ResultOperators.cs @@ -12,16 +12,6 @@ namespace Microsoft.EntityFrameworkCore.Cosmos.Query { public partial class SimpleQueryCosmosTest { - public override async Task Union_with_custom_projection(bool isAsync) - { - await base.Union_with_custom_projection(isAsync); - - AssertSql( - @"SELECT c -FROM root c -WHERE (c[""Discriminator""] = ""Customer"")"); - } - [ConditionalTheory(Skip = "Issue#14935")] public override void Select_All() { diff --git a/test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs b/test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs index 45cc674cba3..27580a03185 100644 --- a/test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs @@ -478,7 +478,7 @@ protected virtual void UseTransaction(DatabaseFacade facade, IDbContextTransacti { } - [ConditionalFact] + [ConditionalFact(Skip = "#16298")] public virtual void Union_siblings_with_duplicate_property_in_subquery() { // Coke and Tea both have CaffeineGrams, which both need to be projected out on each side and so @@ -498,7 +498,7 @@ public virtual void Union_siblings_with_duplicate_property_in_subquery() } } - [ConditionalFact] + [ConditionalFact(Skip = "#16298")] public virtual void OfType_Union_subquery() { using (var context = CreateContext()) @@ -525,7 +525,7 @@ public virtual void OfType_Union_OfType() } } - [ConditionalFact] + [ConditionalFact(Skip = "#16298")] public virtual void Union_entity_equality() { using (var context = CreateContext()) diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs index d27beac45a0..caf56aa4569 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs @@ -235,7 +235,7 @@ public virtual Task Select_Union_unrelated(bool isAsync) .OrderBy(x => x), assertOrder: true); - [ConditionalTheory] + [ConditionalTheory(Skip = "Very similar to #16298")] [MemberData(nameof(IsAsyncData))] public virtual Task Select_Union_different_fields_in_anonymous_with_subquery(bool isAsync) => AssertQuery(isAsync, cs => cs