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

Relational: Make table reference graph mapping consistent #24701

Merged
1 commit merged into from
Apr 20, 2021
Merged

Relational: Make table reference graph mapping consistent #24701

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
3 changes: 2 additions & 1 deletion src/EFCore.Relational/Query/Internal/SelectExpressionProjectionApplyingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ protected override Expression VisitExtension(Expression extensionExpression)
&& shapedQueryExpression.QueryExpression is SelectExpression selectExpression
? shapedQueryExpression.Update(
selectExpression,
selectExpression.ApplyProjection(shapedQueryExpression.ShaperExpression, _querySplittingBehavior))
selectExpression.ApplyProjection(
shapedQueryExpression.ShaperExpression, shapedQueryExpression.ResultCardinality, _querySplittingBehavior))
: base.VisitExtension(extensionExpression);
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore.Relational/Query/RelationalQueryTranslationPostprocessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ public override Expression Process(Expression query)
query = new SelectExpressionProjectionApplyingExpressionVisitor(
((RelationalQueryCompilationContext)QueryCompilationContext).QuerySplittingBehavior).Visit(query);
#if DEBUG
// TODO: 24460 blocks from enabling this
//query = new TableAliasVerifyingExpressionVisitor().Visit(query);
query = new TableAliasVerifyingExpressionVisitor().Visit(query);
#endif
query = new SelectExpressionPruningExpressionVisitor().Visit(query);
query = new SqlExpressionSimplifyingExpressionVisitor(RelationalDependencies.SqlExpressionFactory, _useRelationalNulls).Visit(query);
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,7 @@ outerKey is NewArrayExpression newArrayExpression

var joinPredicate = _sqlTranslator.Translate(Expression.Equal(outerKey, innerKey))!;
_selectExpression.AddLeftJoin(innerSelectExpression, joinPredicate);
var leftJoinTable = ((LeftJoinExpression)_selectExpression.Tables.Last()).Table;
var leftJoinTable = _selectExpression.Tables.Last();

innerShaper = new RelationalEntityShaperExpression(
targetEntityType,
Expand Down
144 changes: 142 additions & 2 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ public bool Equals((ColumnExpression Column, ValueComparer Comparer) x, (ColumnE
private sealed class AliasUniquefier : ExpressionVisitor
{
private readonly HashSet<string> _usedAliases;
private readonly List<SelectExpression> _visitedSelectExpressions = new();

public AliasUniquefier(HashSet<string> usedAliases)
{
Expand All @@ -324,14 +325,17 @@ public AliasUniquefier(HashSet<string> usedAliases)
[return: NotNullIfNotNull("expression")]
public override Expression? Visit(Expression? expression)
{
if (expression is SelectExpression innerSelectExpression)
if (expression is SelectExpression innerSelectExpression
&& !_visitedSelectExpressions.Contains(innerSelectExpression))
{
for (var i = 0; i < innerSelectExpression._tableReferences.Count; i++)
{
var newAlias = GenerateUniqueAlias(_usedAliases, innerSelectExpression._tableReferences[i].Alias);
innerSelectExpression._tableReferences[i].Alias = newAlias;
UnwrapJoinExpression(innerSelectExpression._tables[i]).Alias = newAlias;
}

_visitedSelectExpressions.Add(innerSelectExpression);
}

return base.Visit(expression);
Expand Down Expand Up @@ -365,6 +369,14 @@ public void UpdateTableReference(SelectExpression oldSelect, SelectExpression ne
}
}

internal void Verify(SelectExpression selectExpression)
{
if (!ReferenceEquals(selectExpression, _selectExpression))
{
throw new InvalidOperationException("Dangling TableReferenceExpression.");
}
}

/// <inheritdoc />
public override bool Equals(object? obj)
=> obj != null
Expand Down Expand Up @@ -414,7 +426,7 @@ private static bool IsNullableProjection(ProjectionExpression projectionExpressi
_ => true,
};

private ConcreteColumnExpression(
public ConcreteColumnExpression(
string name, TableReferenceExpression table, Type type, RelationalTypeMapping typeMapping, bool nullable)
: base(type, typeMapping)
{
Expand Down Expand Up @@ -449,6 +461,14 @@ public override ConcreteColumnExpression MakeNullable()
public void UpdateTableReference(SelectExpression oldSelect, SelectExpression newSelect)
=> _table.UpdateTableReference(oldSelect, newSelect);

internal void Verify(IReadOnlyList<TableReferenceExpression> tableReferences)
{
if (!tableReferences.Contains(_table, ReferenceEqualityComparer.Instance))
{
throw new InvalidOperationException("Dangling column.");
}
}

/// <inheritdoc />
public override bool Equals(object? obj)
=> obj != null
Expand Down Expand Up @@ -584,5 +604,125 @@ public ClientProjectionRemappingExpressionVisitor(object[] clientProjectionIndex
return base.Visit(expression);
}
}

private sealed class SelectExpressionVerifyingExpressionVisitor : ExpressionVisitor
Copy link
Contributor

@maumar maumar Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this visitor is not used anywhere - purely a debugging tool? (i.e. we don't want to run always in debug, like we do with table aliases?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is for debugging. Split query introduces clone method on select expression which was causing a lot of errors (for obvious reason), so I added this. I will see if there is a place where I can add this. It is somewhat more complicated than table aliases as this is more stricter form of graph consistency, so it cannot be run during most point when we are mutating SelectExpression.

{
private readonly List<TableReferenceExpression> _tableReferencesInScope = new();

public SelectExpressionVerifyingExpressionVisitor(IEnumerable<TableReferenceExpression> tableReferencesInScope)
{
_tableReferencesInScope.AddRange(tableReferencesInScope);
}

[return: NotNullIfNotNull("expression")]
public override Expression? Visit(Expression? expression)
{
switch (expression)
{
case SelectExpression selectExpression:
foreach (var tableReference in selectExpression._tableReferences)
{
tableReference.Verify(selectExpression);
}

var currentLevelTableReferences = new List<TableReferenceExpression>();
for (var i = 0; i < selectExpression._tables.Count; i++)
{
var table = selectExpression._tables[i];
var tableReference = selectExpression._tableReferences[i];
switch(table)
{
case PredicateJoinExpressionBase predicateJoinExpressionBase:
Verify(predicateJoinExpressionBase.Table, _tableReferencesInScope);
currentLevelTableReferences.Add(tableReference);
Verify(predicateJoinExpressionBase.JoinPredicate,
_tableReferencesInScope.Concat(currentLevelTableReferences));
break;

case SelectExpression innerSelectExpression:
Verify(innerSelectExpression, _tableReferencesInScope);
break;

case CrossApplyExpression crossApplyExpression:
Verify(crossApplyExpression, _tableReferencesInScope.Concat(currentLevelTableReferences));
break;

case OuterApplyExpression outerApplyExpression:
Verify(outerApplyExpression, _tableReferencesInScope.Concat(currentLevelTableReferences));
break;

case JoinExpressionBase joinExpressionBase:
Verify(joinExpressionBase.Table, _tableReferencesInScope);
break;

case SetOperationBase setOperationBase:
Verify(setOperationBase.Source1, _tableReferencesInScope);
Verify(setOperationBase.Source2, _tableReferencesInScope);
break;
}

if (table is not PredicateJoinExpressionBase)
{
currentLevelTableReferences.Add(tableReference);
}
}

_tableReferencesInScope.AddRange(currentLevelTableReferences);

foreach (var projection in selectExpression._projection)
{
Visit(projection);
}

foreach (var keyValuePair in selectExpression._projectionMapping)
{
Visit(keyValuePair.Value);
}

foreach (var clientProjection in selectExpression._clientProjections)
{
Visit(clientProjection);
}

foreach (var grouping in selectExpression._groupBy)
{
Visit(grouping);
}

foreach (var ordering in selectExpression._orderings)
{
Visit(ordering);
}

Visit(selectExpression.Predicate);
Visit(selectExpression.Having);
Visit(selectExpression.Offset);
Visit(selectExpression.Limit);

foreach (var identifier in selectExpression._identifier)
{
Visit(identifier.Column);
}

foreach (var childIdentifier in selectExpression._childIdentifiers)
{
Visit(childIdentifier.Column);
}

break;

case ConcreteColumnExpression concreteColumnExpression:
concreteColumnExpression.Verify(_tableReferencesInScope);
break;
}

return base.Visit(expression);
}


public static void Verify(Expression expression, IEnumerable<TableReferenceExpression> tableReferencesInScope)
=> new SelectExpressionVerifyingExpressionVisitor(tableReferencesInScope)
.Visit(expression);
}
}
}
Loading