Skip to content

Commit

Permalink
Work on set operations and inheritance
Browse files Browse the repository at this point in the history
Added support for set operations over different entity types.

When performing a set operation, if two properties in the entity
inheritance hierarchy were mapped to the same database column, that
column was projected twice.

Closes dotnet#16298
Fixes dotnet#18832
  • Loading branch information
roji committed Nov 26, 2019
1 parent 93d5bfb commit 54620ca
Show file tree
Hide file tree
Showing 7 changed files with 267 additions and 80 deletions.
6 changes: 6 additions & 0 deletions src/EFCore.Relational/Query/EntityProjectionExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ public virtual ColumnExpression BindProperty(IProperty property)

if (!_propertyExpressionsCache.TryGetValue(property, out var expression))
{
if (_innerTable == null)
{
throw new InvalidOperationException(
$"Could not bind property '{property.Name}' on entity type '{EntityType.DisplayName()}': "
+ "projection was initialized with an expression cache but the property isn't in it.");
}
expression = new ColumnExpression(property, _innerTable, _nullable);
_propertyExpressionsCache[property] = expression;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ protected override ShapedQueryExpression TranslateCast(ShapedQueryExpression sou
protected override ShapedQueryExpression TranslateConcat(ShapedQueryExpression source1, ShapedQueryExpression source2)
{
((SelectExpression)source1.QueryExpression).ApplyUnion((SelectExpression)source2.QueryExpression, distinct: false);

ModifyShaperForSetOperation(source1, source2);
return source1;
}

Expand Down Expand Up @@ -263,6 +263,7 @@ protected override ShapedQueryExpression TranslateElementAtOrDefault(
protected override ShapedQueryExpression TranslateExcept(ShapedQueryExpression source1, ShapedQueryExpression source2)
{
((SelectExpression)source1.QueryExpression).ApplyExcept((SelectExpression)source2.QueryExpression, distinct: true);
ModifyShaperForSetOperation(source1, source2);
return source1;
}

Expand Down Expand Up @@ -440,6 +441,7 @@ protected override ShapedQueryExpression TranslateGroupJoin(
protected override ShapedQueryExpression TranslateIntersect(ShapedQueryExpression source1, ShapedQueryExpression source2)
{
((SelectExpression)source1.QueryExpression).ApplyIntersect((SelectExpression)source2.QueryExpression, distinct: true);
ModifyShaperForSetOperation(source1, source2);
return source1;
}

Expand Down Expand Up @@ -929,6 +931,7 @@ protected override ShapedQueryExpression TranslateThenBy(ShapedQueryExpression s
protected override ShapedQueryExpression TranslateUnion(ShapedQueryExpression source1, ShapedQueryExpression source2)
{
((SelectExpression)source1.QueryExpression).ApplyUnion((SelectExpression)source2.QueryExpression, distinct: true);
ModifyShaperForSetOperation(source1, source2);
return source1;
}

Expand Down Expand Up @@ -1180,5 +1183,33 @@ private ShapedQueryExpression AggregateResultShaper(

return source;
}

/// <summary>
/// If a set operation is between different entity types, the query will return their closest common ancestor.
/// Modify the shaper accordingly.
/// </summary>
private void ModifyShaperForSetOperation(ShapedQueryExpression source1, ShapedQueryExpression source2)
{
if (RemoveConvert(source1.ShaperExpression) is EntityShaperExpression shaper1
&& RemoveConvert(source2.ShaperExpression) is EntityShaperExpression shaper2
&& shaper1.EntityType != shaper2.EntityType)
{
source1.ShaperExpression = new EntityShaperExpression(
shaper1.EntityType.GetClosestCommonParent(shaper2.EntityType),
shaper1.ValueBufferExpression,
shaper1.IsNullable);
}

static Expression RemoveConvert(Expression expression)
{
if (expression is UnaryExpression unaryExpression
&& expression.NodeType == ExpressionType.Convert)
{
return RemoveConvert(unaryExpression.Operand);
}

return expression;
}
}
}
}
163 changes: 133 additions & 30 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;

Expand Down Expand Up @@ -525,7 +526,7 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
if (joinedMapping.Value1 is EntityProjectionExpression entityProjection1
&& joinedMapping.Value2 is EntityProjectionExpression entityProjection2)
{
HandleEntityMapping(joinedMapping.Key, select1, entityProjection1, select2, entityProjection2);
HandleEntityMapping(joinedMapping.Key, entityProjection1, entityProjection2);
continue;
}

Expand Down Expand Up @@ -574,50 +575,152 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
_tables.Clear();
_tables.Add(setExpression);

void HandleEntityMapping(
ProjectionMember projectionMember,
SelectExpression select1, EntityProjectionExpression projection1,
SelectExpression select2, EntityProjectionExpression projection2)
void HandleEntityMapping(ProjectionMember projectionMember, EntityProjectionExpression projection1, EntityProjectionExpression projection2)
{
if (projection1.EntityType != projection2.EntityType)
var (entityType1, entityType2) = (projection1.EntityType, projection2.EntityType);

var propertyExpressions = new Dictionary<IProperty, ColumnExpression>();
var expressionsByColumnName = new Dictionary<string, ColumnExpression>();

if (entityType1 == entityType2)
{
foreach (var property in GetAllPropertiesInHierarchy(entityType1))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property.GetColumnName(),
projection1.BindProperty(property),
projection2.BindProperty(property));
}

_projectionMapping[projectionMember] = new EntityProjectionExpression(entityType1, propertyExpressions);
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 = entityType1.GetClosestCommonParent(entityType2);

if (commonParentEntityType == null)
{
throw new InvalidOperationException(
"Set operations over different entity types are currently unsupported (see Issue#16298)");
throw new InvalidOperationException(RelationalStrings.SetOperationNotWithinEntityTypeHierarchy);
}

var propertyExpressions = new Dictionary<IProperty, ColumnExpression>();
foreach (var property in GetAllPropertiesInHierarchy(projection1.EntityType))
var properties1 = GetAllPropertiesInHierarchy(entityType1).ToList();
var properties2 = GetAllPropertiesInHierarchy(entityType2).ToList();

// First handle shared properties that come from common base entity types
foreach (var property in properties1.Intersect(properties2).ToArray())
{
propertyExpressions[property] = AddSetOperationColumnProjections(
select1, projection1.BindProperty(property),
select2, projection2.BindProperty(property));
property.GetColumnName(),
projection1.BindProperty(property),
projection2.BindProperty(property));

properties1.Remove(property);
properties2.Remove(property);
}

_projectionMapping[projectionMember] = new EntityProjectionExpression(projection1.EntityType, propertyExpressions);
}
// Next, find different model property pairs which are mapped to the same database column
foreach (var (group1, group2) in properties1
.GroupBy(p => p.GetColumnName())
.Join(
properties2.GroupBy(p => p.GetColumnName()),
g => g.Key,
g => g.Key,
(p1, p2) => (p1, p2))
.ToArray())
{
var outerProjection = AddSetOperationColumnProjections(
group1.Key,
projection1.BindProperty(group1.First()),
projection2.BindProperty(group2.First()));

foreach (var property in group1)
{
propertyExpressions[property] = outerProjection;
properties1.Remove(property);
}

ColumnExpression AddSetOperationColumnProjections(
SelectExpression select1, ColumnExpression column1,
SelectExpression select2, ColumnExpression column2)
{
var alias = GenerateUniqueAlias(column1.Name);
var innerProjection1 = new ProjectionExpression(column1, alias);
var innerProjection2 = new ProjectionExpression(column2, alias);
select1._projection.Add(innerProjection1);
select2._projection.Add(innerProjection2);
var outerProjection = new ColumnExpression(innerProjection1, setExpression);
if (IsNullableProjection(innerProjection1)
|| IsNullableProjection(innerProjection2))
foreach (var property in group2)
{
propertyExpressions[property] = outerProjection;
properties2.Remove(property);
}
}

// Remaining properties exist only on one side, so inject a null constant projection on the other side.
foreach (var property in properties1)
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property.GetColumnName(),
projection1.BindProperty(property),
new SqlConstantExpression(
Constant(null, property.ClrType.MakeNullable()),
property.GetRelationalTypeMapping()));
}

foreach (var property in properties2)
{
outerProjection = outerProjection.MakeNullable();
propertyExpressions[property] = AddSetOperationColumnProjections(
property.GetColumnName(),
new SqlConstantExpression(
Constant(null, property.ClrType.MakeNullable()),
property.GetRelationalTypeMapping()),
projection2.BindProperty(property));
}

if (select1._identifier.Contains(column1))
// Finally, the shaper will expect to read properties from unrelated siblings, since the set operations
// return type is the common ancestor. Add appropriate null constant projections for both sides.
// See #16215 for a possible optimization.
var unrelatedSiblingProperties = GetAllPropertiesInHierarchy(commonParentEntityType)
.Except(GetAllPropertiesInHierarchy(entityType1))
.Except(GetAllPropertiesInHierarchy(entityType2));
foreach (var property in unrelatedSiblingProperties)
{
_identifier.Add(outerProjection);
propertyExpressions[property] = AddSetOperationColumnProjections(
property.GetColumnName(),
new SqlConstantExpression(
Constant(null, property.ClrType.MakeNullable()),
property.GetRelationalTypeMapping()),
new SqlConstantExpression(
Constant(null, property.ClrType.MakeNullable()),
property.GetRelationalTypeMapping()));
}

return outerProjection;
_projectionMapping[projectionMember] = new EntityProjectionExpression(commonParentEntityType, propertyExpressions);

ColumnExpression AddSetOperationColumnProjections(string columnName, SqlExpression innerExpression1, SqlExpression innerExpression2)
{
if (expressionsByColumnName.TryGetValue(columnName, out var outerProjection))
{
return outerProjection;
}

var alias = GenerateUniqueAlias(columnName);
var innerProjection1 = new ProjectionExpression(innerExpression1, alias);
var innerProjection2 = new ProjectionExpression(innerExpression2, alias);
select1._projection.Add(innerProjection1);
select2._projection.Add(innerProjection2);

outerProjection = new ColumnExpression(innerProjection1, setExpression);

if (IsNullableProjection(innerProjection1)
|| IsNullableProjection(innerProjection2))
{
outerProjection = outerProjection.MakeNullable();
}

if (select1._identifier.Contains(innerExpression1))
{
_identifier.Add(outerProjection);
}

return expressionsByColumnName[columnName] = outerProjection;
}
}

string GenerateUniqueAlias(string baseAlias)
Expand Down
53 changes: 40 additions & 13 deletions test/EFCore.Specification.Tests/Query/InheritanceTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -434,25 +434,52 @@ protected virtual void UseTransaction(DatabaseFacade facade, IDbContextTransacti
{
}

[ConditionalFact(Skip = "Issue#16298")]
public virtual void Union_siblings_with_duplicate_property_in_subquery()
[ConditionalFact]
public virtual void Union_of_supertype_with_itself_with_properties_mapped_to_same_column()
{
// Both Tea and Coffee define properties called CaffeineGrams mapped to the same database column.
// The column should only be selected once (asserted in SQL in SqlServer override).
using var context = CreateContext();
var drinks = context.Set<Drink>()
.Union(context.Set<Drink>())
.ToList();

Assert.Equal(3, drinks.Count);
}

[ConditionalFact]
public virtual void Except_parent_with_child()
{
// Coke and Tea both have CaffeineGrams, which both need to be projected out on each side and so
// requiring alias uniquification. They also have a different number of properties.
using var context = CreateContext();
var cokes = context.Set<Coke>();
var drinks = context.Set<Drink>()
.Except(context.Set<Coke>())
.ToList();

Assert.Collection(
drinks,
d => Assert.IsType<Tea>(d),
d => Assert.IsType<Lilt>(d));

var teas = context.Set<Tea>();
Assert.Equal(2, drinks.Count);
}

var concat = cokes.Cast<Drink>()
.Union(teas)
.Where(d => d.Id > 0)
[ConditionalFact]
public virtual void Concat_siblings_with_two_properties_mapped_to_same_column()
{
// Coke and Tea both have CaffeineGrams, which both need to be projected out on each side and so
// requiring alias uniquification. They also have a different number of properties.
using var context = CreateContext();
var drinks = context.Set<Coke>()
.Concat(context.Set<Tea>().Cast<Drink>())
.ToList();

Assert.Equal(2, concat.Count);
Assert.Collection(
drinks,
d => Assert.Equal(6, Assert.IsType<Coke>(d).SugarGrams),
d => Assert.True(Assert.IsType<Tea>(d).HasMilk));
}

[ConditionalFact(Skip = "Issue#16298")]
[ConditionalFact]
public virtual void OfType_Union_subquery()
{
using var context = CreateContext();
Expand All @@ -465,7 +492,7 @@ public virtual void OfType_Union_subquery()
.ToList();
}

[ConditionalFact(Skip = "Issue#16298")]
[ConditionalFact]
public virtual void OfType_Union_OfType()
{
using var context = CreateContext();
Expand All @@ -487,7 +514,7 @@ public virtual void Subquery_OfType()
.ToList();
}

[ConditionalFact(Skip = "Issue#16298")]
[ConditionalFact]
public virtual void Union_entity_equality()
{
using var context = CreateContext();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -364,12 +364,8 @@ public virtual Task Select_Union_different_fields_in_anonymous_with_subquery(boo
.Union(
ss.Set<Customer>()
.Where(c => c.City == "London")
.Select(c => new { Foo = c.Region, Customer = c })) // Foo is Region
.OrderBy(x => x.Foo)
.Skip(1)
.Take(10)
.Where(x => x.Foo == "Berlin"),
entryCount: 1);
.Select(c => new { Foo = c.PostalCode, Customer = c })), // Foo is PostalCode
entryCount: 7);
}

[ConditionalTheory]
Expand Down
Loading

0 comments on commit 54620ca

Please sign in to comment.