Skip to content

Commit

Permalink
Query: Match joined tables properly when lifting for group by aggrega…
Browse files Browse the repository at this point in the history 
…te (#27170)

Earlier we only matched alias and skipped if alias matched. This could happen if the table name starts with same character.
Even if we do deeper match unwinding joins, we cannot match if the join is to subquery (which can be generated when target of navigation has query filter).
Solution:
When applying group by on SelectExpression, remember the original table count. Once Groupby has been applied we cannot add more joins to SelectExpression other than group by aggregate term lifting.
During lifting,
- If aliases of original tables in parent and subquery doesn't match then we assume subquery is non-grouping element rooted and we don't lift.
- If parent have lifted additional joins, one of them being a subquery join, then we abort lifting if the subquery contains a join to lift which is a subquery.
- If we are allowed to join after first 2 checks then,
  - We copy over owned entity in initial tables
  - We try to match additional joins after initial if they are table joins and joining to same table, in which case we don't need to join them again.
  - We copy over all other joins.

Resolves #27163
  • Loading branch information
@smitpatel
smitpatel authored Jan 20, 2022
1 parent e7fa151 commit 10b4433
Show file tree
Hide file tree
Showing 6 changed files with 294 additions and 18 deletions.
122 changes: 108 additions & 14 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,8 @@ private sealed class CloningExpressionVisitor : ExpressionVisitor
_usedAliases = selectExpression._usedAliases.ToHashSet(),
_projectionMapping = newProjectionMappings,
_groupingCorrelationPredicate = groupingCorrelationPredicate,
_groupingParentSelectExpressionId = selectExpression._groupingParentSelectExpressionId
_groupingParentSelectExpressionId = selectExpression._groupingParentSelectExpressionId,
_groupingParentSelectExpressionTableCount = selectExpression._groupingParentSelectExpressionTableCount,
};

newSelectExpression._tptLeftJoinTables.AddRange(selectExpression._tptLeftJoinTables);
Expand Down Expand Up @@ -869,29 +870,122 @@ public GroupByAggregateLiftingExpressionVisitor(SelectExpression selectExpressio
&& subquery.Offset == null
&& subquery._groupBy.Count == 0
&& subquery.Predicate != null
&& ((AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27102", out var enabled) && enabled)
&& ((AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27102", out var enabled27102) && enabled27102)
|| subquery.Predicate.Equals(subquery._groupingCorrelationPredicate))
&& ((AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27094", out var enabled2) && enabled2)
&& ((AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27094", out var enabled27094) && enabled27094)
|| subquery._groupingParentSelectExpressionId == _selectExpression._groupingParentSelectExpressionId))
{
var initialTableCounts = 0;
var potentialTableCount = Math.Min(_selectExpression._tables.Count, subquery._tables.Count);
for (var i = 0; i < potentialTableCount; i++)
if (AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27163", out var enabled27163) && enabled27163)
{
if (!string.Equals(
_selectExpression._tableReferences[i].Alias,
subquery._tableReferences[i].Alias, StringComparison.OrdinalIgnoreCase))
var potentialTableCount = Math.Min(_selectExpression._tables.Count, subquery._tables.Count);
for (var i = 0; i < potentialTableCount; i++)
{
break;
if (!string.Equals(
_selectExpression._tableReferences[i].Alias,
subquery._tableReferences[i].Alias, StringComparison.OrdinalIgnoreCase))
{
break;
}

if (_selectExpression._tables[i] is SelectExpression originalNestedSelectExpression
&& subquery._tables[i] is SelectExpression subqueryNestedSelectExpression)
{
CopyOverOwnedJoinInSameTable(originalNestedSelectExpression, subqueryNestedSelectExpression);
}

initialTableCounts++;
}
}
else
{
initialTableCounts = _selectExpression._groupingParentSelectExpressionTableCount!.Value;
var potentialTableCount = Math.Min(_selectExpression._tables.Count, subquery._tables.Count);
// First verify that subquery has same structure for initial tables,
// If not then subquery may have different root than grouping element.
for (var i = 0; i < initialTableCounts; i++)
{
if (!string.Equals(
_selectExpression._tableReferences[i].Alias,
subquery._tableReferences[i].Alias, StringComparison.OrdinalIgnoreCase))
{
initialTableCounts = 0;
break;
}
}

if (_selectExpression._tables[i] is SelectExpression originalNestedSelectExpression
&& subquery._tables[i] is SelectExpression subqueryNestedSelectExpression)
if (initialTableCounts > 0)
{
CopyOverOwnedJoinInSameTable(originalNestedSelectExpression, subqueryNestedSelectExpression);
// If initial table structure matches and
// Parent has additional joins lifted already one of them is a subquery join
// Then we abort lifting if any of the joins from the subquery to lift are a subquery join
if (_selectExpression._tables.Skip(initialTableCounts)
.Select(e => UnwrapJoinExpression(e))
.Any(e => e is SelectExpression))
{
for (var i = initialTableCounts; i < subquery._tables.Count; i++)
{
if (UnwrapJoinExpression(subquery._tables[i]) is SelectExpression)
{
// If any of the join is to subquery then we abort the lifting group by term altogether.
initialTableCounts = 0;
break;
}
}
}
}

initialTableCounts++;
if (initialTableCounts > 0)
{
// We need to copy over owned join which are coming from same initial tables.
for (var i = 0; i < initialTableCounts; i++)
{
if (_selectExpression._tables[i] is SelectExpression originalNestedSelectExpression
&& subquery._tables[i] is SelectExpression subqueryNestedSelectExpression)
{
CopyOverOwnedJoinInSameTable(originalNestedSelectExpression, subqueryNestedSelectExpression);
}
}


for (var i = initialTableCounts; i < potentialTableCount; i++)
{
// Try to match additional tables for the cases where we can match exact so we can avoid lifting
// same joins to parent
if (!string.Equals(
_selectExpression._tableReferences[i].Alias,
subquery._tableReferences[i].Alias, StringComparison.OrdinalIgnoreCase))
{
break;
}

var outerTableExpressionBase = _selectExpression._tables[i];
var innerTableExpressionBase = subquery._tables[i];

if (outerTableExpressionBase is InnerJoinExpression outerInnerJoin
&& innerTableExpressionBase is InnerJoinExpression innerInnerJoin)
{
outerTableExpressionBase = outerInnerJoin.Table as TableExpression;
innerTableExpressionBase = innerInnerJoin.Table as TableExpression;
}
else if (outerTableExpressionBase is LeftJoinExpression outerLeftJoin
&& innerTableExpressionBase is LeftJoinExpression innerLeftJoin)
{
outerTableExpressionBase = outerLeftJoin.Table as TableExpression;
innerTableExpressionBase = innerLeftJoin.Table as TableExpression;
}

if (outerTableExpressionBase is TableExpression outerTable
&& innerTableExpressionBase is TableExpression innerTable
&& !(string.Equals(outerTable.Name, innerTable.Name, StringComparison.OrdinalIgnoreCase)
&& string.Equals(outerTable.Schema, innerTable.Schema, StringComparison.OrdinalIgnoreCase)))
{
break;
}

initialTableCounts++;
}
}
}

if (initialTableCounts > 0)
Expand All @@ -900,7 +994,7 @@ public GroupByAggregateLiftingExpressionVisitor(SelectExpression selectExpressio
// We only replace columns from initial tables.
// Additional tables may have been added to outer from other terms which may end up matching on table alias
var columnExpressionReplacingExpressionVisitor =
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27083", out var enabled3) && enabled3
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27083", out var enabled27083) && enabled27083
? new ColumnExpressionReplacingExpressionVisitor(
subquery, _selectExpression._tableReferences)
: new ColumnExpressionReplacingExpressionVisitor(
Expand Down
18 changes: 14 additions & 4 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public sealed partial class SelectExpression : TableExpressionBase

private SqlExpression? _groupingCorrelationPredicate;
private Guid? _groupingParentSelectExpressionId;
private int? _groupingParentSelectExpressionTableCount;
private CloningExpressionVisitor? _cloningExpressionVisitor;

private SelectExpression(
Expand Down Expand Up @@ -1256,7 +1257,7 @@ public GroupByShaperExpression ApplyGrouping(

// We generate the cloned expression before changing identifier for this SelectExpression
// because we are going to erase grouping for cloned expression.
if (!(AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27094", out var enabled2) && enabled2))
if (!(AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27094", out var enabled27094) && enabled27094))
{
_groupingParentSelectExpressionId = Guid.NewGuid();

Expand All @@ -1267,10 +1268,15 @@ public GroupByShaperExpression ApplyGrouping(
.Aggregate((l, r) => sqlExpressionFactory.AndAlso(l, r));
clonedSelectExpression._groupBy.Clear();
clonedSelectExpression.ApplyPredicate(correlationPredicate);
if (!(AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27102", out var enabled) && enabled))
if (!(AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27102", out var enabled27102) && enabled27102))
{
clonedSelectExpression._groupingCorrelationPredicate = clonedSelectExpression.Predicate;
}
if (!(AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27163", out var enabled27163) && enabled27163))
{
_groupingParentSelectExpressionTableCount = _tables.Count;

This comment has been minimized.

Sign in to view

Copy link
@Saibamen

Saibamen Jan 25, 2022

Unnecessary new line
}

if (!_identifier.All(e => _groupBy.Contains(e.Column)))
{
Expand Down Expand Up @@ -1495,6 +1501,7 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
Offset = Offset,
Limit = Limit,
_groupingParentSelectExpressionId = _groupingParentSelectExpressionId,
_groupingParentSelectExpressionTableCount = _groupingParentSelectExpressionTableCount,
_groupingCorrelationPredicate = _groupingCorrelationPredicate
};
Offset = null;
Expand All @@ -1504,6 +1511,7 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
Having = null;
_groupingCorrelationPredicate = null;
_groupingParentSelectExpressionId = null;
_groupingParentSelectExpressionTableCount = null;
_groupBy.Clear();
_orderings.Clear();
_tables.Clear();
Expand Down Expand Up @@ -2819,7 +2827,8 @@ private SqlRemappingVisitor PushdownIntoSubqueryInternal()
Having = Having,
Offset = Offset,
Limit = Limit,
_groupingParentSelectExpressionId = _groupingParentSelectExpressionId
_groupingParentSelectExpressionId = _groupingParentSelectExpressionId,
_groupingParentSelectExpressionTableCount = _groupingParentSelectExpressionTableCount
};
subquery._usedAliases = _usedAliases;
_tables.Clear();
Expand Down Expand Up @@ -3482,7 +3491,8 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
Tags = Tags,
_usedAliases = _usedAliases,
_groupingCorrelationPredicate = groupingCorrelationPredicate,
_groupingParentSelectExpressionId = _groupingParentSelectExpressionId
_groupingParentSelectExpressionId = _groupingParentSelectExpressionId,
_groupingParentSelectExpressionTableCount = _groupingParentSelectExpressionTableCount,
};

newSelectExpression._tptLeftJoinTables.AddRange(_tptLeftJoinTables);
Expand Down
103 changes: 103 additions & 0 deletions test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -786,5 +786,108 @@ protected class Table
public int Id { get; set; }
public int? Value { get; set; }
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Group_by_multiple_aggregate_joining_different_tables(bool async)
{
var contextFactory = await InitializeAsync<Context27163>();
using var context = contextFactory.CreateContext();

var query = context.Parents
.GroupBy(x => new { })
.Select(g => new
{
Test1 = g
.Select(x => x.Child1.Value1)
.Distinct()
.Count(),
Test2 = g
.Select(x => x.Child2.Value2)
.Distinct()
.Count()
});

var orders = async
? await query.ToListAsync()
: query.ToList();
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Group_by_multiple_aggregate_joining_different_tables_with_query_filter(bool async)
{
var contextFactory = await InitializeAsync<Context27163>();
using var context = contextFactory.CreateContext();

var query = context.Parents
.GroupBy(x => new { })
.Select(g => new
{
Test1 = g
.Select(x => x.ChildFilter1.Value1)
.Distinct()
.Count(),
Test2 = g
.Select(x => x.ChildFilter2.Value2)
.Distinct()
.Count()
});

var orders = async
? await query.ToListAsync()
: query.ToList();
}

protected class Context27163 : DbContext
{
public Context27163(DbContextOptions options)
: base(options)
{
}

public DbSet<Parent> Parents { get; set; }

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<ChildFilter1>().HasQueryFilter(e => e.Filter1 == "Filter1");
modelBuilder.Entity<ChildFilter2>().HasQueryFilter(e => e.Filter2 == "Filter2");
}
}

public class Parent
{
public int Id { get; set; }
public Child1 Child1 { get; set; }
public Child2 Child2 { get; set; }
public ChildFilter1 ChildFilter1 { get; set; }
public ChildFilter2 ChildFilter2 { get; set; }
}

public class Child1
{
public int Id { get; set; }
public string Value1 { get; set; }
}

public class Child2
{
public int Id { get; set; }
public string Value2 { get; set; }
}

public class ChildFilter1
{
public int Id { get; set; }
public string Filter1 { get; set; }
public string Value1 { get; set; }
}

public class ChildFilter2
{
public int Id { get; set; }
public string Filter2 { get; set; }
public string Value2 { get; set; }
}
}
}
13 changes: 13 additions & 0 deletions .../EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsSharedTypeQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Threading.Tasks;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.EntityFrameworkCore.Query
Expand Down Expand Up @@ -299,6 +300,18 @@ FROM [Level1] AS [l1]
) AS [t0] ON [l].[Id] = [t0].[Level1_Optional_Id]");
}

[ConditionalTheory(Skip = "Issue#26104")]
public override Task GroupBy_aggregate_where_required_relationship(bool async)
{
return base.GroupBy_aggregate_where_required_relationship(async);
}

[ConditionalTheory(Skip = "Issue#26104")]
public override Task GroupBy_aggregate_where_required_relationship_2(bool async)
{
return base.GroupBy_aggregate_where_required_relationship_2(async);
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
}
Expand Down
Loading

0 comments on commit 10b4433

Please sign in to comment.