Skip to content

Commit

Permalink
Set operation cleanup
Browse files Browse the repository at this point in the history 
* We no longer allow ColumnExpression without table alias
* Set operations over operands with different types have been disabled
  for now.
* We now push down set operations into a subquery on Orderby, Skip or
  Take, which shouldn't be necessary (#16244).
* Some test consolidation and cleanup.

Continues #6812
  • Loading branch information
@roji
roji committed Jun 28, 2019
1 parent 4f1f019 commit 1285b4f
Show file tree
Hide file tree
Showing 11 changed files with 332 additions and 506 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}";
}
}
190 changes: 66 additions & 124 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Reflection;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Query.Internal;
Expand Down Expand Up @@ -258,9 +259,8 @@ public void ApplyPredicate(SqlExpression expression)

public void ApplyOrdering(OrderingExpression orderingExpression)
{
if (IsDistinct
|| Limit != null
|| Offset != null)
// TODO: We should not be pushing down set operations, see #16244
if (IsDistinct || Limit != null || Offset != null || IsSetOperation)
{
orderingExpression = orderingExpression.Update(
new SqlRemappingVisitor(PushdownIntoSubquery())
Expand All @@ -281,7 +281,8 @@ public void AppendOrdering(OrderingExpression orderingExpression)

public void ApplyLimit(SqlExpression sqlExpression)
{
if (Limit != null)
// TODO: We should not be pushing down set operations, see #16244
if (Limit != null || IsSetOperation)
{
PushdownIntoSubquery();
}
Expand All @@ -291,8 +292,8 @@ public void ApplyLimit(SqlExpression sqlExpression)

public void ApplyOffset(SqlExpression sqlExpression)
{
if (Limit != null
|| Offset != null)
// TODO: We should not be pushing down set operations, see #16244
if (Limit != null || Offset != null || IsSetOperation)
{
PushdownIntoSubquery();
}
Expand Down Expand Up @@ -367,11 +368,14 @@ public Expression ApplySetOperation(
select1._projectionMapping = new Dictionary<ProjectionMember, Expression>(_projectionMapping);
_projectionMapping.Clear();

select1._identifier.AddRange(_identifier);
_identifier.Clear();

var select2 = otherSelectExpression;

if (_projection.Any())
{
throw new NotImplementedException("Set operation on SelectExpression with populated _projection");
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 @@ -387,106 +391,24 @@ 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)
{
var propertyExpressions = new Dictionary<IProperty, ColumnExpression>();

if (entityProjection1.EntityType == entityProjection2.EntityType)
{
foreach (var property in GetAllPropertiesInHierarchy(entityProjection1.EntityType))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property,
select1, entityProjection1.GetProperty(property),
select2, entityProjection2.GetProperty(property));
}

_projectionMapping[joinedMapping.Key] = new EntityProjectionExpression(entityProjection1.EntityType, propertyExpressions);
continue;
}

// 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 = entityProjection1.EntityType.GetClosestCommonParent(entityProjection2.EntityType);

if (commonParentEntityType == null)
{
throw new NotSupportedException(RelationalStrings.SetOperationNotWithinEntityTypeHierarchy);
}

var properties1 = GetAllPropertiesInHierarchy(entityProjection1.EntityType).ToArray();
var properties2 = GetAllPropertiesInHierarchy(entityProjection2.EntityType).ToArray();

foreach (var property in properties1.Intersect(properties2))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property,
select1, entityProjection1.GetProperty(property),
select2, entityProjection2.GetProperty(property));
}

foreach (var property in properties1.Except(properties2))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property,
select1, entityProjection1.GetProperty(property),
select2, null);
}

foreach (var property in properties2.Except(properties1))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property,
select1, null,
select2, entityProjection2.GetProperty(property));
}

foreach (var property in GetAllPropertiesInHierarchy(commonParentEntityType)
.Except(properties1).Except(properties2))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property,
select1, null,
select2, null);
}

_projectionMapping[joinedMapping.Key] = new EntityProjectionExpression(commonParentEntityType, propertyExpressions);

if (commonParentEntityType != entityProjection1.EntityType)
{
if (!(shaperExpression.RemoveConvert() is EntityShaperExpression entityShaperExpression))
{
throw new Exception("Non-entity shaper expression while handling set operation over siblings.");
}

shaperExpression = new EntityShaperExpression(
commonParentEntityType, entityShaperExpression.ValueBufferExpression, entityShaperExpression.Nullable);
}

HandleEntityMapping(joinedMapping.Key, select1, entityProjection1, select2, entityProjection2);
continue;
}

if (joinedMapping.Value1 is ColumnExpression innerColumn1
&& joinedMapping.Value2 is ColumnExpression innerColumn2)
if (joinedMapping.Value1 is ColumnExpression && joinedMapping.Value2 is ColumnExpression
|| joinedMapping.Value1 is SubSelectExpression && joinedMapping.Value2 is SubSelectExpression)
{
// 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 = joinedMapping.Key.LastMember?.Name;
var index = select1.AddToProjection(innerColumn1, alias);
var projectionExpression1 = select1._projection[index];
select2.AddToProjection(innerColumn2, alias);
var outerColumn = new ColumnExpression(projectionExpression1, select1, IsNullableProjection(projectionExpression1));
_projectionMapping[joinedMapping.Key] = outerColumn;
HandleColumnMapping(
joinedMapping.Key,
select1, (SqlExpression)joinedMapping.Value1,
select2, (SqlExpression)joinedMapping.Value2);
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 All @@ -502,34 +424,59 @@ public Expression ApplySetOperation(

return shaperExpression;

static ColumnExpression AddSetOperationColumnProjections(
void HandleEntityMapping(
ProjectionMember projectionMember,
SelectExpression select1, EntityProjectionExpression projection1,
SelectExpression select2, EntityProjectionExpression projection2)
{
var propertyExpressions = new Dictionary<IProperty, ColumnExpression>();

if (projection1.EntityType == projection2.EntityType)
{
foreach (var property in GetAllPropertiesInHierarchy(projection1.EntityType))
{
propertyExpressions[property] = AddSetOperationColumnProjections(
property,
select1, projection1.GetProperty(property),
select2, projection2.GetProperty(property));
}

_projectionMapping[projectionMember] = new EntityProjectionExpression(projection1.EntityType, propertyExpressions);
return;
}

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 columnName = column1.Name;

var typeMapping = column1?.TypeMapping ?? column2?.TypeMapping ?? property.FindRelationalMapping();
select1._projection.Add(new ProjectionExpression(column1, columnName));
select2._projection.Add(new ProjectionExpression(column2, columnName));

select1._projection.Add(new ProjectionExpression(column1 != null
? (SqlExpression)column1
: new SqlConstantExpression(Constant(null), typeMapping),
columnName));
if (select1._identifier.Contains(column1))
{
_identifier.Add(column1);
}

select2._projection.Add(new ProjectionExpression(column2 != null
? (SqlExpression)column2
: new SqlConstantExpression(Constant(null), typeMapping),
columnName));
return column1;
}

var projectionExpression = select1._projection[select1._projection.Count - 1];
var outerColumn = new ColumnExpression(projectionExpression, select1, IsNullableProjection(projectionExpression));
return outerColumn;
void HandleColumnMapping(
ProjectionMember projectionMember,
SelectExpression select1, SqlExpression innerColumn1,
SelectExpression select2, SqlExpression innerColumn2)
{
// 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;
select1.AddToProjection(innerColumn1, alias);
select2.AddToProjection(innerColumn2, alias);
_projectionMapping[projectionMember] = innerColumn1;
}
}

Expand Down Expand Up @@ -901,9 +848,7 @@ public void AddInnerJoin(SelectExpression innerSelectExpression, SqlExpression j

public void AddLeftJoin(SelectExpression innerSelectExpression, SqlExpression joinPredicate, Type transparentIdentifierType)
{
if (Limit != null
|| Offset != null
|| IsDistinct)
if (Limit != null || Offset != null || IsDistinct || IsSetOperation)
{
joinPredicate = new SqlRemappingVisitor(PushdownIntoSubquery())
.Remap(joinPredicate);
Expand Down Expand Up @@ -951,10 +896,7 @@ public void AddLeftJoin(SelectExpression innerSelectExpression, SqlExpression jo

public void AddCrossJoin(SelectExpression innerSelectExpression, Type transparentIdentifierType)
{
if (Limit != null
|| Offset != null
|| IsDistinct
|| Predicate != null)
if (Limit != null || Offset != null || IsDistinct || Predicate != null || IsSetOperation)
{
PushdownIntoSubquery();
}
Expand Down
11 changes: 4 additions & 7 deletions src/EFCore/Query/NavigationExpansion/NavigationTreeNode.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,17 +132,14 @@ public static NavigationTreeNode Create(
return result;
}

public List<NavigationTreeNode> Flatten()
public IEnumerable<NavigationTreeNode> Flatten()
{
var result = new List<NavigationTreeNode>();
result.Add(this);
yield return this;

foreach (var child in Children)
foreach (var child in Children.SelectMany(c => c.Flatten()))
{
result.AddRange(child.Flatten());
yield return child;
}

return result;
}

// TODO: just make property settable?
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
Loading

0 comments on commit 1285b4f

Please sign in to comment.