Skip to content

Commit

Permalink
Fix to #22049 - Query: consider updating select expression identifier…
Browse files Browse the repository at this point in the history 
…s when projecting a subset of column and adding distinct

As fix to #15873 we started blocking some scenarios that used to work (by accident) - when we have a subquery using Distinct or GroupBy that doesn't happen to have any duplicates.

Fix is to enable those scenarios (and others) by modifying identifier columns in case of distinct and group by, if the original identifiers are not already present. In case of distinct, the entire projection becomes unique identifier, as distinct guarantees it to be unique.
In case of groupby, the grouping key becomes the identifier - since we only support grouping key or group aggregate in the projection, we are also guaranteed to have 1 row per unique grouping key.

Also fix to #24288 - Query: add collection join tries to convert correlated collection from APPLY to JOIN for subqueries with Distinct and GroupBy, which is incorrect

we would always try to convert subquery with groupby and distinct from apply to join, however we can only do this if the projection already contains the join key. Otherwise, adding the join key to the projection would change the meaning of operation in case of distinct and create invalid query in case of group by (projecting column that is not part of grouping key or aggregate).

Fixes #22049
Fixes #24288
  • Loading branch information
@maumar
maumar committed Mar 6, 2021
1 parent 673e6ca commit 2b0ba6e
Show file tree
Hide file tree
Showing 28 changed files with 1,791 additions and 163 deletions.
20 changes: 17 additions & 3 deletions src/EFCore.Relational/Properties/RelationalStrings.Designer.cs
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions src/EFCore.Relational/Properties/RelationalStrings.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -621,8 +621,14 @@
<data name="MissingConcurrencyColumn" xml:space="preserve">
<value>Entity type '{entityType}' doesn't contain a property mapped to the store-generated concurrency token column '{missingColumn}' which is used by another entity type sharing the table '{table}'. Add a store-generated property to '{entityType}' which is mapped to the same column; it may be in shadow state.</value>
</data>
<data name="MissingIdentifyingProjectionInDistinctGroupBySubquery" xml:space="preserve">
<value>Unable to translate a collection subquery in a projection since it uses 'Distinct' or 'Group By' operations and doesn't project key columns of all tables required to generate results on the client side. Missing column: {column}. Either add column(s) to the projection or rewrite the query to not use the 'GroupBy'/'Distinct' operation.</value>
<data name="UnableToTranslateSubqueryWithDistinct" xml:space="preserve">
<value>Subquery with 'Distinct' can only be translated if projection consists only of entities and their properties, or it contains keys of all entities required to generate results on the client side. Either add '{column}' to the projection, remove complex elements of the projection, or rewrite the query to not use the 'Distinct' operation.</value>
</data>
<data name="UnableToTranslateSubqueryWithGroupBy" xml:space="preserve">
<value>Subquery with 'GroupBy' can only be translated if grouping key consists only of entities and their properties, or it contains keys of all entities required to generate results on the client side. Either add '{column}' to the grouping key, remove complex elements of the grouping key, or rewrite the query to not use the 'GroupBy' operation.</value>
</data>
<data name="DistinctOnCollectionNotSupported" xml:space="preserve">
<value>Using 'Distinct' operation on a projection containing a collection is not supported.</value>
</data>
<data name="MissingOrderingInSelectExpression" xml:space="preserve">
<value>'Reverse' could not be translated to the server because there is no ordering on the server side.</value>
Expand Down
219 changes: 179 additions & 40 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,34 @@ public int AddToProjection([NotNull] SqlExpression sqlExpression)
return AddToProjection(sqlExpression, null);
}

/// <summary>
/// Adds given <see cref="EntityProjectionExpression" /> to the projection.
/// </summary>
/// <param name="entityProjection"> An entity projection to add. </param>
/// <returns> A dictionary of <see cref="IProperty" /> to int indicating properties and their corresponding indexes in the projection list. </returns>
public IDictionary<IProperty, int> AddToProjection([NotNull] EntityProjectionExpression entityProjection)
{
Check.NotNull(entityProjection, nameof(entityProjection));

if (!_entityProjectionCache.TryGetValue(entityProjection, out var dictionary))
{
dictionary = new Dictionary<IProperty, int>();
foreach (var property in GetAllPropertiesInHierarchy(entityProjection.EntityType))
{
dictionary[property] = AddToProjection(entityProjection.BindProperty(property));
}

if (entityProjection.DiscriminatorExpression != null)
{
AddToProjection(entityProjection.DiscriminatorExpression, DiscriminatorColumnAlias);
}

_entityProjectionCache[entityProjection] = dictionary;
}

return dictionary;
}

private int AddToProjection(SqlExpression sqlExpression, string? alias)
{
var existingIndex = _projection.FindIndex(pe => pe.Expression.Equals(sqlExpression));
Expand Down Expand Up @@ -459,34 +487,6 @@ private int AddToProjection(SqlExpression sqlExpression, string? alias)
return _projection.Count - 1;
}

/// <summary>
/// Adds given <see cref="EntityProjectionExpression" /> to the projection.
/// </summary>
/// <param name="entityProjection"> An entity projection to add. </param>
/// <returns> A dictionary of <see cref="IProperty" /> to int indicating properties and their corresponding indexes in the projection list. </returns>
public IDictionary<IProperty, int> AddToProjection([NotNull] EntityProjectionExpression entityProjection)
{
Check.NotNull(entityProjection, nameof(entityProjection));

if (!_entityProjectionCache.TryGetValue(entityProjection, out var dictionary))
{
dictionary = new Dictionary<IProperty, int>();
foreach (var property in GetAllPropertiesInHierarchy(entityProjection.EntityType))
{
dictionary[property] = AddToProjection(entityProjection.BindProperty(property));
}

if (entityProjection.DiscriminatorExpression != null)
{
AddToProjection(entityProjection.DiscriminatorExpression, DiscriminatorColumnAlias);
}

_entityProjectionCache[entityProjection] = dictionary;
}

return dictionary;
}

/// <summary>
/// Prepares the <see cref="SelectExpression" /> to apply aggregate operation over it.
/// </summary>
Expand Down Expand Up @@ -699,6 +699,11 @@ public void ReverseOrderings()
/// </summary>
public void ApplyDistinct()
{
if (_pendingCollections.Count > 0)
{
throw new InvalidOperationException(RelationalStrings.DistinctOnCollectionNotSupported);
}

if (Limit != null
|| Offset != null)
{
Expand Down Expand Up @@ -1040,6 +1045,7 @@ static bool IsNullableProjection(ProjectionExpression projectionExpression)
private ColumnExpression GenerateOuterColumn(SqlExpression projection, string? alias = null)
{
var index = AddToProjection(projection, alias);

return new ColumnExpression(_projection[index], this);
}

Expand Down Expand Up @@ -1119,6 +1125,20 @@ public IDictionary<SqlExpression, ColumnExpression> PushdownIntoSubquery()
// if we can't propagate any identifier - clear them all instead
// when adding collection join we detect this and throw appropriate exception
_identifier.Clear();

if (IsDistinct
&& projectionMap.Select(p => p.Value).All(x => x is ColumnExpression))
{
// for distinct try to use entire projection as identifiers
_identifier.AddRange(projectionMap.Select(p => p.Value).Select(x => ((ColumnExpression)x, x.TypeMapping!.Comparer)));
}
else if (GroupBy.Count > 0
&& GroupBy.All(x => x is ColumnExpression))
{
// for group by try to use grouping key as identifiers
_identifier.AddRange(GroupBy.Select(x => ((ColumnExpression)x, x.TypeMapping!.Comparer)));
}

break;
}
}
Expand Down Expand Up @@ -1409,7 +1429,7 @@ public CollectionShaperExpression AddCollectionProjection(
var parentIdentifier = GetIdentifierAccessor(identifierFromParent).Item1;
innerSelectExpression.ApplyProjection();

ValidateIdentifyingProjection(innerSelectExpression);
RemapIdentifiers(innerSelectExpression);

for (var i = 0; i < identifierFromParent.Count; i++)
{
Expand Down Expand Up @@ -1511,7 +1531,7 @@ public CollectionShaperExpression AddCollectionProjection(
var innerClientEval = innerSelectExpression.Projection.Count > 0;
innerSelectExpression.ApplyProjection();

ValidateIdentifyingProjection(innerSelectExpression);
RemapIdentifiers(innerSelectExpression);

if (collectionIndex == 0)
{
Expand Down Expand Up @@ -1635,24 +1655,77 @@ public CollectionShaperExpression AddCollectionProjection(
return result;
}

static void ValidateIdentifyingProjection(SelectExpression selectExpression)
static void RemapIdentifiers(SelectExpression innerSelectExpression)
{
if (selectExpression.IsDistinct
|| selectExpression.GroupBy.Count > 0)
if (innerSelectExpression.IsDistinct
|| innerSelectExpression.GroupBy.Count > 0)
{
var innerSelectProjectionExpressions = selectExpression._projection.Select(p => p.Expression).ToList();
foreach (var innerSelectIdentifier in selectExpression._identifier)
if (!IdentifiersInProjection(innerSelectExpression, out var missingIdentifier))
{
if (!innerSelectProjectionExpressions.Contains(innerSelectIdentifier.Column)
&& (selectExpression.GroupBy.Count == 0
|| !selectExpression.GroupBy.Contains(innerSelectIdentifier.Column)))
// we can safely clear identifiers here - child identifiers will always be empty at this point
// - nested collection scenarios where distinct is applied after projection are blocked
// since we can't translate them in any meaningful way
// - if distinct is applied before the projection, pushdown happens which guarantees child identifiers to be empty
// - for groupby we only support aggregate scenarios so collection can never happen in the projection
if (innerSelectExpression.IsDistinct)
{
throw new InvalidOperationException(RelationalStrings.MissingIdentifyingProjectionInDistinctGroupBySubquery(
innerSelectIdentifier.Column.Table.Alias + "." + innerSelectIdentifier.Column.Name));
if (innerSelectExpression._projection.All(p => p.Expression is ColumnExpression))
{
innerSelectExpression._identifier.Clear();
foreach (var projection in innerSelectExpression._projection)
{
innerSelectExpression._identifier.Add(((ColumnExpression)projection.Expression, projection.Expression.TypeMapping!.Comparer));
}
}
else
{
throw new InvalidOperationException(
RelationalStrings.UnableToTranslateSubqueryWithDistinct(
missingIdentifier.Table.Alias + "." + missingIdentifier.Name));
}
}
else
{
if (innerSelectExpression.GroupBy.All(g => g is ColumnExpression))
{
innerSelectExpression._identifier.Clear();
foreach (var grouping in innerSelectExpression.GroupBy)
{
innerSelectExpression._identifier.Add(((ColumnExpression)grouping, grouping.TypeMapping!.Comparer));
}
}
else
{
throw new InvalidOperationException(
RelationalStrings.UnableToTranslateSubqueryWithGroupBy(
missingIdentifier.Table.Alias + "." + missingIdentifier.Name));
}
}
}
}
}

static bool IdentifiersInProjection(
SelectExpression selectExpression,
[CA.NotNullWhen(false)] out ColumnExpression? missingIdentifier)
{
var innerSelectProjectionExpressions = selectExpression._projection.Select(p => p.Expression).ToList();
foreach (var innerSelectIdentifier in selectExpression._identifier)
{
if (!innerSelectProjectionExpressions.Contains(innerSelectIdentifier.Column)
&& (selectExpression.GroupBy.Count == 0
|| !selectExpression.GroupBy.Contains(innerSelectIdentifier.Column)))
{
missingIdentifier = innerSelectIdentifier.Column;

return false;
}
}

missingIdentifier = null;

return true;
}
}

private sealed class EntityShaperNullableMarkingExpressionVisitor : ExpressionVisitor
Expand Down Expand Up @@ -1709,12 +1782,78 @@ private static SqlExpression MakeNullable(SqlExpression sqlExpression)
joinPredicate = RemoveRedundantNullChecks(joinPredicate, columnExpressions);
}

// we can't convert apply to join in case of distinct and groupby, if the projection doesn't already contain the join keys
// since we can't add the missing keys to the projection - only convert to join if all the keys are already there
if (joinPredicate != null
&& (selectExpression.IsDistinct
|| selectExpression.GroupBy.Count > 0))
{
var innerKeyColumns = new List<ColumnExpression>();
InnerKeyColumns(selectExpression.Tables, joinPredicate, innerKeyColumns);

// if projection has already been applied we can use it directly
// otherwise we extract future projection columns from projection mapping
// and based on that we determine whether we can convert from APPLY to JOIN
var projectionColumns = selectExpression.Projection.Count > 0
? selectExpression.Projection.Select(p => p.Expression).OfType<ColumnExpression>()
: ExtractColumnsFromProjectionMapping(selectExpression._projectionMapping);

foreach (var innerColumn in innerKeyColumns)
{
if (!projectionColumns.Contains(innerColumn))
{
return null;
}
}
}

selectExpression.Predicate = predicate;

return joinPredicate;
}

return null;

static void InnerKeyColumns(IEnumerable<TableExpressionBase> tables, SqlExpression joinPredicate, List<ColumnExpression> resultColumns)
{
if (joinPredicate is SqlBinaryExpression sqlBinaryExpression)
{
InnerKeyColumns(tables, sqlBinaryExpression.Left, resultColumns);
InnerKeyColumns(tables, sqlBinaryExpression.Right, resultColumns);
}
else if (joinPredicate is ColumnExpression columnExpression
&& tables.Contains(columnExpression.Table))
{
resultColumns.Add(columnExpression);
}
}

static List<ColumnExpression> ExtractColumnsFromProjectionMapping(IDictionary<ProjectionMember, Expression> projectionMapping)
{
var result = new List<ColumnExpression>();
foreach (var mappingElement in projectionMapping)
{
if (mappingElement.Value is EntityProjectionExpression entityProjection)
{
foreach (var property in GetAllPropertiesInHierarchy(entityProjection.EntityType))
{
result.Add(entityProjection.BindProperty(property));
}

if (entityProjection.DiscriminatorExpression != null
&& entityProjection.DiscriminatorExpression is ColumnExpression discriminatorColumn)
{
result.Add(discriminatorColumn);
}
}
else if (mappingElement.Value is ColumnExpression column)
{
result.Add(column);
}
}

return result;
}
}

private SqlExpression? TryExtractJoinKey(
Expand Down
Loading

0 comments on commit 2b0ba6e

Please sign in to comment.