Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Query: Improvements to Relational GroupBy translation for composition #11222

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Query: Improvements to Relational GroupBy translation for composition #11222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ...FCore.Relational/Query/ExpressionVisitors/Internal/ResultTransformingExpressionVisitor.cs
View file
Open in desktop
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
414 changes: 95 additions & 319 deletions src/EFCore.Relational/Query/ExpressionVisitors/RelationalProjectionExpressionVisitor.cs
View file
Open in desktop

Large diffs are not rendered by default.

120 changes: 73 additions & 47 deletions src/EFCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs
View file
Open in desktop
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flip condition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flipping condition wouldn't avoid nesting. First if block retrieves the QSRE in multiple cases. Second if block uses QSRE to find the translation.

{
// 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
Open in desktop
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