Skip to content

Commit

Permalink
Query: Improvements to Relational GroupBy translation for composition
Browse files Browse the repository at this point in the history 
- Add support for translating OrderBy after GroupBy operator
- Add support for `HAVING` clause in SQL which would be generated when translating predicate after GroupByAggregate Resolves #10870
- Make sure client eval warning is not issued when translating GroupByAggregate Resolves #11157
- GroupBy Aggregate works when element/result selector is DTO instead of anonymous type Resolves #11176 (KeySelector has to be client evaluated)
- Make sure that SQL added to GROUP BY clause is not aliased Resolves #11218
- Translate GroupBy Constant/Parameter with aggregates Resolves #9969

Part of #10012
Part of #2341
  • Loading branch information
@smitpatel
smitpatel committed Mar 14, 2018
1 parent 248ec65 commit e8ca357
Show file tree
Hide file tree
Showing 21 changed files with 1,542 additions and 521 deletions.
2 changes: 0 additions & 2 deletions ...FCore.Relational/Query/ExpressionVisitors/Internal/ResultTransformingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@ public class ResultTransformingExpressionVisitor<TResult> : ExpressionVisitorBas
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
public ResultTransformingExpressionVisitor(
[NotNull] IQuerySource outerQuerySource,
[NotNull] RelationalQueryCompilationContext relationalQueryCompilationContext,
bool throwOnNullResult)
{
Check.NotNull(outerQuerySource, nameof(outerQuerySource));
Check.NotNull(relationalQueryCompilationContext, nameof(relationalQueryCompilationContext));

_relationalQueryCompilationContext = relationalQueryCompilationContext;
Expand Down
370 changes: 74 additions & 296 deletions src/EFCore.Relational/Query/ExpressionVisitors/RelationalProjectionExpressionVisitor.cs
View file

Large diffs are not rendered by default.

120 changes: 73 additions & 47 deletions src/EFCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ protected override Expression VisitBinary(BinaryExpression expression)
switch (expression.NodeType)
{
case ExpressionType.Coalesce:
{
var left = Visit(expression.Left);
var right = Visit(expression.Right);

Expand All @@ -157,31 +156,25 @@ protected override Expression VisitBinary(BinaryExpression expression)
&& right.Type != typeof(Expression[])
? expression.Update(left, expression.Conversion, right)
: null;
}

case ExpressionType.Equal:
case ExpressionType.NotEqual:
{
var structuralComparisonExpression
= UnfoldStructuralComparison(
expression.NodeType,
ProcessComparisonExpression(expression));

return structuralComparisonExpression;
}

case ExpressionType.GreaterThan:
case ExpressionType.GreaterThanOrEqual:
case ExpressionType.LessThan:
case ExpressionType.LessThanOrEqual:
{
return ProcessComparisonExpression(expression);
}

case ExpressionType.AndAlso:
{
var left = Visit(expression.Left);
var right = Visit(expression.Right);
left = Visit(expression.Left);
right = Visit(expression.Right);

if (expression == _topLevelPredicate)
{
Expand Down Expand Up @@ -209,7 +202,6 @@ var structuralComparisonExpression
return left != null && right != null
? Expression.AndAlso(left, right)
: null;
}

case ExpressionType.OrElse:
case ExpressionType.Add:
Expand All @@ -219,7 +211,6 @@ var structuralComparisonExpression
case ExpressionType.Modulo:
case ExpressionType.And:
case ExpressionType.Or:
{
var leftExpression = Visit(expression.Left);
var rightExpression = Visit(expression.Right);

Expand All @@ -232,7 +223,6 @@ var structuralComparisonExpression
expression.IsLiftedToNull,
expression.Method)
: null;
}
}

return null;
Expand Down Expand Up @@ -721,11 +711,35 @@ var newMemberExpression

private Expression TryBindQuerySourcePropertyExpression(MemberExpression memberExpression)
{
if (memberExpression.Expression is QuerySourceReferenceExpression qsre)
if (!(memberExpression.Expression is QuerySourceReferenceExpression qsre))
{
// For memberExpression of form g.Key.Id
// We need SelectExpression for g & Member for Id (since we saved mappings)
if (memberExpression.Expression is MemberExpression innerMemberExpression
&& innerMemberExpression.Expression is QuerySourceReferenceExpression
&& innerMemberExpression.Expression.Type.IsGrouping()
&& innerMemberExpression.Member.Name == nameof(IGrouping<int, int>.Key))
{
qsre = (QuerySourceReferenceExpression)innerMemberExpression.Expression;
}
else
{
qsre = null;
}
}

if (qsre != null)
{
var selectExpression = _queryModelVisitor.TryGetQuery(qsre.ReferencedQuerySource);
var sql = _queryModelVisitor.TryGetQuery(qsre.ReferencedQuerySource)
?.GetProjectionForMemberInfo(memberExpression.Member);

return selectExpression?.GetProjectionForMemberInfo(memberExpression.Member);
if (_topLevelPredicate != null
&& sql is AliasExpression aliasExpression)
{
return aliasExpression.Expression;
}

return sql;
}

return null;
Expand Down Expand Up @@ -794,30 +808,27 @@ protected override Expression VisitUnary(UnaryExpression expression)
switch (expression.NodeType)
{
case ExpressionType.Negate:
{
var operand = Visit(expression.Operand);
if (operand != null)
{
return Expression.Negate(operand);
}

break;
}

case ExpressionType.Not:
{
var operand = Visit(expression.Operand);
operand = Visit(expression.Operand);
if (operand != null)
{
return Expression.Not(operand);
}

break;
}

case ExpressionType.Convert:
{
var isTopLevelProjection = _isTopLevelProjection;
_isTopLevelProjection = false;
var operand = Visit(expression.Operand);
operand = Visit(expression.Operand);
_isTopLevelProjection = isTopLevelProjection;

if (operand != null)
Expand All @@ -832,7 +843,7 @@ protected override Expression VisitUnary(UnaryExpression expression)
}

break;
}

}

return null;
Expand Down Expand Up @@ -939,20 +950,46 @@ var subQueryModelVisitor
= (RelationalQueryModelVisitor)_queryModelVisitor.QueryCompilationContext
.CreateQueryModelVisitor(_queryModelVisitor);

if (expression.QueryModel.MainFromClause.FromExpression is QuerySourceReferenceExpression groupQsre
&& groupQsre.Type.IsGrouping())
{
var targetExpression = _queryModelVisitor.QueryCompilationContext.QuerySourceMapping
.GetExpression(groupQsre.ReferencedQuerySource);

if (targetExpression.Type == typeof(ValueBuffer))
{
var outerSelectExpression = _targetSelectExpression.Clone();
subQueryModelVisitor.AddQuery(subQueryModel.MainFromClause, outerSelectExpression);

_queryModelVisitor.QueryCompilationContext.AddOrUpdateMapping(
groupQsre.ReferencedQuerySource,
Expression.Parameter(
typeof(IEnumerable<>).MakeGenericType(typeof(ValueBuffer))));

subQueryModelVisitor.VisitSubQueryModel(subQueryModel);

_queryModelVisitor.QueryCompilationContext.AddOrUpdateMapping(
groupQsre.ReferencedQuerySource,
targetExpression);

if (outerSelectExpression.Projection.Count == 1)
{
var projection = outerSelectExpression.Projection.Single();

return projection is AliasExpression aliasExpression
? aliasExpression.Expression
: projection;
}
}
}

var queriesProjectionCountMapping
= _queryModelVisitor.Queries
.ToDictionary(k => k, s => s.Projection.Count);

var queryModelMapping = new Dictionary<QueryModel, QueryModel>();
subQueryModel.PopulateQueryModelMapping(queryModelMapping);

var groupByTranslation = expression.QueryModel.MainFromClause.FromExpression.Type.IsGrouping();
if (groupByTranslation)
{
var outerSelectExpression = _targetSelectExpression.Clone();
subQueryModelVisitor.AddQuery(subQueryModel.MainFromClause, outerSelectExpression);
}

subQueryModelVisitor.VisitSubQueryModel(subQueryModel);

if (subQueryModelVisitor.IsLiftable)
Expand All @@ -968,12 +1005,6 @@ var queriesProjectionCountMapping

_queryModelVisitor.LiftInjectedParameters(subQueryModelVisitor);

if (groupByTranslation
&& selectExpression.Projection.Count == 1)
{
return selectExpression.Projection.Single();
}

return selectExpression;
}

Expand Down Expand Up @@ -1041,7 +1072,6 @@ protected override Expression VisitExtension(Expression expression)
switch (expression)
{
case StringCompareExpression stringCompare:
{
var newLeft = Visit(stringCompare.Left);
var newRight = Visit(stringCompare.Right);
if (newLeft == null
Expand All @@ -1054,9 +1084,8 @@ protected override Expression VisitExtension(Expression expression)
|| newRight != stringCompare.Right
? new StringCompareExpression(stringCompare.Operator, newLeft, newRight)
: expression;
}

case ExplicitCastExpression explicitCast:
{
var newOperand = Visit(explicitCast.Operand);
if (newOperand == null)
{
Expand All @@ -1066,9 +1095,8 @@ protected override Expression VisitExtension(Expression expression)
return newOperand != explicitCast.Operand
? new ExplicitCastExpression(newOperand, explicitCast.Type)
: expression;
}

case NullConditionalExpression nullConditionalExpression:
{
var newAccessOperation = Visit(nullConditionalExpression.AccessOperation);
if (newAccessOperation == null)
{
Expand All @@ -1081,17 +1109,15 @@ protected override Expression VisitExtension(Expression expression)
}

return new NullableExpression(newAccessOperation);
}

case NullSafeEqualExpression nullSafeEqualExpression:
{
var equalityExpression
= new NullCompensatedExpression(nullSafeEqualExpression.EqualExpression);

return Visit(equalityExpression);
}

case NullCompensatedExpression nullCompensatedExpression:
{
var newOperand = Visit(nullCompensatedExpression.Operand);
newOperand = Visit(nullCompensatedExpression.Operand);
if (newOperand == null)
{
return null;
Expand All @@ -1100,7 +1126,7 @@ var equalityExpression
return newOperand != nullCompensatedExpression.Operand
? new NullCompensatedExpression(newOperand)
: nullCompensatedExpression;
}

case DiscriminatorPredicateExpression discriminatorPredicateExpression:
return new DiscriminatorPredicateExpression(
base.VisitExtension(expression), discriminatorPredicateExpression.QuerySource);
Expand Down
28 changes: 25 additions & 3 deletions src/EFCore.Relational/Query/Expressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ public SelectExpression(
/// </value>
public virtual Expression Predicate { get; [param: CanBeNull] set; }

/// <summary>
/// Gets or sets the predicate corresponding to the HAVING part of the SELECT expression.
/// </summary>
/// <value>
/// The predicate.
/// </value>
public virtual Expression Having { get; [param: CanBeNull] set; }

/// <summary>
/// Gets or sets the table to be used for star projection.
/// </summary>
Expand Down Expand Up @@ -302,7 +310,8 @@ public virtual bool IsIdentityQuery()
&& Offset == null
&& Projection.Count == 0
&& OrderBy.Count == 0
&& GroupBy.Count == 0
// GroupBy is intentionally ommitted because GroupBy does not require a pushdown.
//&& GroupBy.Count == 0
&& Tables.Count == 1;

/// <summary>
Expand Down Expand Up @@ -689,7 +698,10 @@ public virtual void SetProjectionExpression([NotNull] Expression expression)

private Expression CreateUniqueProjection(Expression expression, string newAlias = null)
{
var currentProjectionIndex = _projection.FindIndex(e => e.Equals(expression));
var currentProjectionIndex
= _projection.FindIndex(
e => ExpressionEqualityComparer.Instance.Equals(e, expression)
|| ExpressionEqualityComparer.Instance.Equals((e as AliasExpression)?.Expression, expression));

if (currentProjectionIndex != -1)
{
Expand Down Expand Up @@ -871,7 +883,14 @@ public virtual void AddToPredicate([NotNull] Expression predicate)
{
Check.NotNull(predicate, nameof(predicate));

Predicate = Predicate != null ? AndAlso(Predicate, predicate) : predicate;
if (_groupBy.Count == 0)
{
Predicate = Predicate != null ? AndAlso(Predicate, predicate) : predicate;
}
else
{
Having = Having != null ? AndAlso(Having, predicate) : predicate;
}
}

/// <summary>
Expand All @@ -882,6 +901,9 @@ public virtual void AddToGroupBy([NotNull] Expression[] groupingExpressions)
{
Check.NotNull(groupingExpressions, nameof(groupingExpressions));

// Skip/Take will cause PushDown prior to calling this method so it is safe to clear ordering if any.
// Ordering before GroupBy Aggregate has no effect.
_orderBy.Clear();
_groupBy.AddRange(groupingExpressions);
}

Expand Down
Loading

0 comments on commit e8ca357

Please sign in to comment.