Skip to content

Commit

Permalink
Rolled back ColumnExpression changes
Browse files Browse the repository at this point in the history 
Disabled support for siblings of differing entity types, dotnet#16298
  • Loading branch information
@roji
roji committed Jun 27, 2019
1 parent 96bb180 commit 918ce1a
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 130 deletions.
8 changes: 2 additions & 6 deletions src/EFCore.Relational/Query/Pipeline/QuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 6 additions & 2 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressions/ColumnExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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;
Expand Down Expand Up @@ -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}";
}
}
123 changes: 15 additions & 108 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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)
{
Expand All @@ -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");
}
}

Expand Down Expand Up @@ -443,99 +442,25 @@ 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(
IProperty property,
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(
Expand All @@ -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;
}
}

Expand Down Expand Up @@ -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)
{
Expand Down
10 changes: 0 additions & 10 deletions test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.ResultOperators.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
6 changes: 3 additions & 3 deletions test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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())
Expand All @@ -525,7 +525,7 @@ public virtual void OfType_Union_OfType()
}
}

[ConditionalFact]
[ConditionalFact(Skip = "#16298")]
public virtual void Union_entity_equality()
{
using (var context = CreateContext())
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Customer>(isAsync, cs => cs
Expand Down

0 comments on commit 918ce1a

Please sign in to comment.