Skip to content

Commit

Permalink
Query: Match joined tables properly when lifting for group by aggregate
Browse files Browse the repository at this point in the history 
Earlier we only matched alias and skipped if alias matched. This could happen if the table name starts with same character.
Added logic to unwrap the join table and match exact table name/schema to identify if it is parent table
This may not be the best fix but it avoids different table matching and ends up generating invalid SQL in normal scenario.
The additional tables from navigation are going to be joinExpression only. If we find a shape which is not what we expect we fallback to previous behavior.
The inner table of joinExpression may not always be table (think of a case where there is query filter probably). Though fixing for a complicated case may cause instability in running queries. Deferring for that till customer reports. In most cases running into this bug will generate invalid SQL exception

Resolves #27163
  • Loading branch information
@smitpatel
smitpatel committed Jan 12, 2022
1 parent e7fa151 commit 630ab93
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 7 deletions.
63 changes: 56 additions & 7 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -869,20 +869,30 @@ 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 (!string.Equals(
_selectExpression._tableReferences[i].Alias,
subquery._tableReferences[i].Alias, StringComparison.OrdinalIgnoreCase))
if (AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue27163", out var enabled27163) && enabled27163)
{
break;
if (!string.Equals(
_selectExpression._tableReferences[i].Alias,
subquery._tableReferences[i].Alias, StringComparison.OrdinalIgnoreCase))
{
break;
}
}
else
{
if (!IsSameTable(i))
{
break;
}
}

if (_selectExpression._tables[i] is SelectExpression originalNestedSelectExpression
Expand All @@ -900,7 +910,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 All @@ -924,6 +934,45 @@ public GroupByAggregateLiftingExpressionVisitor(SelectExpression selectExpressio

return updatedProjection;
}

bool IsSameTable(int index)
{
if (!string.Equals(
_selectExpression._tableReferences[index].Alias,
subquery._tableReferences[index].Alias, StringComparison.OrdinalIgnoreCase))
{
return false;
}

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

TableExpression? outerTable = null;
TableExpression? innerTable = null;

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

if (outerTable != null
&& innerTable != null
&& !(string.Equals(outerTable.Name, innerTable.Name, StringComparison.OrdinalIgnoreCase)
&& string.Equals(outerTable.Schema, innerTable.Schema, StringComparison.OrdinalIgnoreCase)))
{
return false;
}

return true;
}
}
}

Expand Down
55 changes: 55 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,60 @@ 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();
}

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

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

public class Parent
{
public int Id { get; set; }
public Child1 Child1 { get; set; }
public Child2 Child2 { 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; }
}
}
}
15 changes: 15 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,5 +213,20 @@ ORDER BY (SELECT 1)), 0) AS [C]
FROM [Table] AS [t]
GROUP BY [t].[Value]");
}

public override async Task Group_by_multiple_aggregate_joining_different_tables(bool async)
{
await base.Group_by_multiple_aggregate_joining_different_tables(async);

AssertSql(
@"SELECT COUNT(DISTINCT ([c].[Value1])) AS [Test1], COUNT(DISTINCT ([c0].[Value2])) AS [Test2]
FROM (
SELECT [p].[Child1Id], [p].[Child2Id], 1 AS [Key]
FROM [Parents] AS [p]
) AS [t]
LEFT JOIN [Child1] AS [c] ON [t].[Child1Id] = [c].[Id]
LEFT JOIN [Child2] AS [c0] ON [t].[Child2Id] = [c0].[Id]
GROUP BY [t].[Key]");
}
}
}

0 comments on commit 630ab93

Please sign in to comment.