Skip to content

Commit

Permalink
Query: Materialize results of FirstOrDefault in subquery only when th…
Browse files Browse the repository at this point in the history
…ere are rows

Issue: When we generate translation for subquery.FirstOrDefault() using Left join, we get null in values when there is no associated record.
Then we try to create result with default values rather than returning default of result type.

Fix: Whenever we go through path where we are adding subquery.FirstOrDefault in projection, we inject 1 as dummy column. If we see 1 in result then only we materialize the object else we return default of the type.
- For entityTypes this is not needed because we return null if key values are null.
- For scalar which is server eval (i.e. only 1 column), we generate projectionBinding using resultType so we will get correct default back.
- For scalar result which is client eval, we inject 1 into client projections.
- For non scalar results (anonymous/nominal types), in client/server eval case, we inject 1 into projection/projectionMapping.

Resolves #17287
  • Loading branch information
smitpatel committed Nov 1, 2019
1 parent c9d02c7 commit 64b8642
Show file tree
Hide file tree
Showing 8 changed files with 197 additions and 59 deletions.
33 changes: 32 additions & 1 deletion src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,31 @@ EntityProjectionExpression LiftEntityProjectionFromSubquery(EntityProjectionExpr
public Expression AddSingleProjection(ShapedQueryExpression shapedQueryExpression)
{
var innerSelectExpression = (SelectExpression)shapedQueryExpression.QueryExpression;
var shaperExpression = shapedQueryExpression.ShaperExpression;
var innerExpression = RemoveConvert(shaperExpression);
if (!(innerExpression is EntityShaperExpression))
{
var sentinelExpression = innerSelectExpression.Limit;
ProjectionBindingExpression dummyProjection;
if (innerSelectExpression.Projection.Any())
{
var index = innerSelectExpression.AddToProjection(sentinelExpression);
dummyProjection = new ProjectionBindingExpression(
innerSelectExpression, index, sentinelExpression.Type);
}
else
{
innerSelectExpression._projectionMapping[new ProjectionMember()] = sentinelExpression;
dummyProjection = new ProjectionBindingExpression(
innerSelectExpression, new ProjectionMember(), sentinelExpression.Type);
}

shaperExpression = Condition(
Equal(dummyProjection, Default(dummyProjection.Type)),
Default(shaperExpression.Type),
shaperExpression);
}

innerSelectExpression.ApplyProjection();
var projectionCount = innerSelectExpression.Projection.Count;
AddOuterApply(innerSelectExpression, null);
Expand All @@ -816,7 +841,13 @@ public Expression AddSingleProjection(ShapedQueryExpression shapedQueryExpressio
}

return new ShaperRemappingExpressionVisitor(this, innerSelectExpression, indexOffset)
.Visit(shapedQueryExpression.ShaperExpression);
.Visit(shaperExpression);

static Expression RemoveConvert(Expression expression)
=> expression is UnaryExpression unaryExpression
&& unaryExpression.NodeType == ExpressionType.Convert
? RemoveConvert(unaryExpression.Operand)
: expression;
}

public CollectionShaperExpression AddCollectionProjection(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,11 @@ public override Task GroupBy_with_boolean_groupin_key_thru_navigation_access(boo
{
return GroupBy_with_boolean_groupin_key_thru_navigation_access(isAsync);
}

[ConditionalTheory(Skip = "issue #17260")]
public override Task Select_subquery_projecting_single_constant_inside_anonymous(bool isAsync)
{
return base.Select_subquery_projecting_single_constant_inside_anonymous(isAsync);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5904,8 +5904,7 @@ public virtual Task Lift_projection_mapping_when_pushing_down_subquery(bool isAs
elementAsserter: (e, a) =>
{
Assert.Equal(e.Id, a.Id);
// See issue#17287
Assert.Equal(e.c1?.Id ?? 0, a.c1?.Id);
Assert.Equal(e.c1?.Id, a.c1?.Id);
AssertCollection(e.c2, a.c2, elementSorter: i => i.Id, elementAsserter: (ie, ia) => Assert.Equal(ie.Id, ia.Id));
});
}
Expand Down
66 changes: 51 additions & 15 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,8 @@ public virtual Task Select_enum_has_flag(bool isAsync)
.Select(
b => new
{
hasFlagTrue = b.Rank.HasFlag(MilitaryRank.Corporal), hasFlagFalse = b.Rank.HasFlag(MilitaryRank.Sergeant)
hasFlagTrue = b.Rank.HasFlag(MilitaryRank.Corporal),
hasFlagFalse = b.Rank.HasFlag(MilitaryRank.Sergeant)
}));
}

Expand Down Expand Up @@ -1251,11 +1252,11 @@ public virtual Task Where_compare_anonymous_types(bool isAsync)
ss => from g in ss.Set<Gear>()
from o in ss.Set<Gear>().OfType<Officer>()
where new
{
Name = g.LeaderNickname,
Squad = g.LeaderSquadId,
Five = 5
}
{
Name = g.LeaderNickname,
Squad = g.LeaderSquadId,
Five = 5
}
== new
{
Name = o.Nickname,
Expand All @@ -1282,8 +1283,8 @@ public virtual Task Where_compare_anonymous_types_with_uncorrelated_members(bool
{
return AssertQuery(
isAsync,
// ReSharper disable once EqualExpressionComparison
ss => from g in ss.Set<Gear>()
// ReSharper disable once EqualExpressionComparison
where new { Five = 5 } == new { Five = 5 }
select g.Nickname);
}
Expand Down Expand Up @@ -3275,7 +3276,8 @@ where f is LocustHorde
orderby f.Name
select new
{
Name = EF.Property<string>(horde, "Name"), Eradicated = EF.Property<bool>((LocustHorde)f, "Eradicated")
Name = EF.Property<string>(horde, "Name"),
Eradicated = EF.Property<bool>((LocustHorde)f, "Eradicated")
},
ss => from f in ss.Set<Faction>()
where f is LocustHorde
Expand Down Expand Up @@ -3443,7 +3445,7 @@ public virtual Task Comparing_two_collection_navigations_composite_key(bool isAs
isAsync,
ss => from g1 in ss.Set<Gear>()
from g2 in ss.Set<Gear>()
// ReSharper disable once PossibleUnintendedReferenceComparison
// ReSharper disable once PossibleUnintendedReferenceComparison
where g1.Weapons == g2.Weapons
orderby g1.Nickname
select new { Nickname1 = g1.Nickname, Nickname2 = g2.Nickname },
Expand Down Expand Up @@ -3596,7 +3598,8 @@ public virtual Task Project_collection_navigation_with_inheritance3(bool isAsync
.Select(
f => new
{
f.Id, Gears = EF.Property<ICollection<Gear>>((Officer)((LocustHorde)f).Commander.DefeatedBy, "Reports")
f.Id,
Gears = EF.Property<ICollection<Gear>>((Officer)((LocustHorde)f).Commander.DefeatedBy, "Reports")
}),
ss => ss.Set<Faction>()
.Where(f => f is LocustHorde)
Expand Down Expand Up @@ -5576,6 +5579,33 @@ public virtual Task Project_one_value_type_from_empty_collection(bool isAsync)
s => new { s.Name, SquadId = s.Members.Where(m => m.HasSoulPatch).Select(m => m.SquadId).FirstOrDefault() }));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Project_one_value_type_converted_to_nullable_from_empty_collection(bool isAsync)
{
return AssertQuery(
isAsync,
ss => ss.Set<Squad>().Where(s => s.Name == "Kilo").Select(
s => new { s.Name, SquadId = s.Members.Where(m => m.HasSoulPatch).Select(m => (int?)m.SquadId).FirstOrDefault() }));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Project_one_value_type_with_client_projection_from_empty_collection(bool isAsync)
{
return AssertQuery(
isAsync,
ss => ss.Set<Squad>().Where(s => s.Name == "Kilo").Select(
s => new
{
s.Name,
SquadId = s.Members.Where(m => m.HasSoulPatch).Select(m => ClientFunction(m.SquadId, m.LeaderSquadId)).FirstOrDefault()
}),
elementSorter: s => s.Name);
}

private static int ClientFunction(int a, int b) => a + b + 1;

[ConditionalTheory(Skip = "issue #15864")]
[MemberData(nameof(IsAsyncData))]
public virtual Task Filter_on_subquery_projecting_one_value_type_from_empty_collection(bool isAsync)
Expand Down Expand Up @@ -5616,7 +5646,7 @@ public virtual Task Select_subquery_projecting_single_constant_bool(bool isAsync
s => new { s.Name, Gear = s.Members.Where(g => g.HasSoulPatch).Select(g => true).FirstOrDefault() }));
}

[ConditionalTheory(Skip = "Issue#17287")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Select_subquery_projecting_single_constant_inside_anonymous(bool isAsync)
{
Expand All @@ -5626,12 +5656,11 @@ public virtual Task Select_subquery_projecting_single_constant_inside_anonymous(
s => new
{
s.Name,
Gear = s.Members.Where(g => g.HasSoulPatch).Select(
g => new { One = 1 }).FirstOrDefault()
Gear = s.Members.Where(g => g.HasSoulPatch).Select(g => new { One = 1 }).FirstOrDefault()
}));
}

[ConditionalTheory(Skip = "Issue#17287")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Select_subquery_projecting_multiple_constants_inside_anonymous(bool isAsync)
{
Expand Down Expand Up @@ -5712,7 +5741,14 @@ public virtual Task Select_subquery_projecting_single_constant_of_non_mapped_typ
ss => ss.Set<Squad>().Select(
s => new { s.Name, Gear = s.Members.Where(g => g.HasSoulPatch).Select(g => new MyDTO()).FirstOrDefault() }),
elementSorter: e => e.Name,
elementAsserter: (e, a) => Assert.Equal(e.Name, a.Name));
elementAsserter: (e, a) =>
{
Assert.Equal(e.Name, a.Name);
if (e.Gear == null)
{
Assert.Null(a.Gear);
}
});
}

[ConditionalTheory(Skip = "issue #11567")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,26 @@ public virtual Task Select_collection_FirstOrDefault_project_anonymous_type(bool
{
return AssertQuery(
isAsync,
ss => ss.Set<Customer>().OrderBy(c => c.CustomerID).Take(2).Select(
ss => ss.Set<Customer>().Where(e => e.CustomerID.StartsWith("F")).OrderBy(c => c.CustomerID).Take(2).Select(
c => c.Orders.OrderBy(o => o.OrderID).Select(
o => new { o.CustomerID, o.OrderID }).FirstOrDefault()),
assertOrder: true);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Select_collection_FirstOrDefault_project_anonymous_type_client_eval(bool isAsync)
{
return AssertQuery(
isAsync,
ss => ss.Set<Customer>().Where(e => e.CustomerID.StartsWith("F")).OrderBy(c => c.CustomerID).Take(2).Select(
c => c.Orders.OrderBy(o => o.OrderID).Select(
o => new { o.CustomerID, OrderID = ClientFunction(o.OrderID, 5) }).FirstOrDefault()),
assertOrder: true);
}

private static int ClientFunction(int a, int b) => a + b + 1;

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Select_collection_FirstOrDefault_project_entity(bool isAsync)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4411,15 +4411,15 @@ public override async Task Lift_projection_mapping_when_pushing_down_subquery(bo
AssertSql(
@"@__p_0='25'
SELECT [t].[Id], [t1].[Id], [l1].[Id]
SELECT [t].[Id], [t1].[Id], [t1].[c], [l1].[Id]
FROM (
SELECT TOP(@__p_0) [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
FROM [LevelOne] AS [l]
) AS [t]
LEFT JOIN (
SELECT [t0].[Id], [t0].[OneToMany_Required_Inverse2Id]
SELECT [t0].[Id], [t0].[c], [t0].[OneToMany_Required_Inverse2Id]
FROM (
SELECT [l0].[Id], [l0].[OneToMany_Required_Inverse2Id], ROW_NUMBER() OVER(PARTITION BY [l0].[OneToMany_Required_Inverse2Id] ORDER BY [l0].[Id]) AS [row]
SELECT [l0].[Id], 1 AS [c], [l0].[OneToMany_Required_Inverse2Id], ROW_NUMBER() OVER(PARTITION BY [l0].[OneToMany_Required_Inverse2Id] ORDER BY [l0].[Id]) AS [row]
FROM [LevelTwo] AS [l0]
) AS [t0]
WHERE [t0].[row] <= 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5684,6 +5684,38 @@ FROM [Squads] AS [s]
WHERE ([s].[Name] = N'Kilo') AND [s].[Name] IS NOT NULL");
}

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

AssertSql(
@"SELECT [s].[Name], (
SELECT TOP(1) [g].[SquadId]
FROM [Gears] AS [g]
WHERE ([g].[Discriminator] IN (N'Gear', N'Officer') AND ([s].[Id] = [g].[SquadId])) AND ([g].[HasSoulPatch] = CAST(1 AS bit))) AS [SquadId]
FROM [Squads] AS [s]
WHERE ([s].[Name] = N'Kilo') AND [s].[Name] IS NOT NULL");
}

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

AssertSql(
@"SELECT [s].[Name], [t0].[SquadId], [t0].[LeaderSquadId], [t0].[c]
FROM [Squads] AS [s]
LEFT JOIN (
SELECT [t].[SquadId], [t].[LeaderSquadId], [t].[c], [t].[Nickname]
FROM (
SELECT [g].[SquadId], [g].[LeaderSquadId], 1 AS [c], [g].[Nickname], ROW_NUMBER() OVER(PARTITION BY [g].[SquadId] ORDER BY [g].[Nickname], [g].[SquadId]) AS [row]
FROM [Gears] AS [g]
WHERE [g].[Discriminator] IN (N'Gear', N'Officer') AND ([g].[HasSoulPatch] = CAST(1 AS bit))
) AS [t]
WHERE [t].[row] <= 1
) AS [t0] ON [s].[Id] = [t0].[SquadId]
WHERE ([s].[Name] = N'Kilo') AND [s].[Name] IS NOT NULL");
}

public override async Task Filter_on_subquery_projecting_one_value_type_from_empty_collection(bool isAsync)
{
await base.Filter_on_subquery_projecting_one_value_type_from_empty_collection(isAsync);
Expand Down Expand Up @@ -5739,41 +5771,35 @@ public override async Task Select_subquery_projecting_single_constant_inside_ano
await base.Select_subquery_projecting_single_constant_inside_anonymous(isAsync);

AssertSql(
@"SELECT [s].[Name], [s].[Id]
FROM [Squads] AS [s]",
//
@"@_outer_Id='1'
SELECT TOP(1) 1
FROM [Gears] AS [g]
WHERE ([g].[Discriminator] IN (N'Officer', N'Gear') AND (@_outer_Id = [g].[SquadId])) AND ([g].[HasSoulPatch] = CAST(1 AS bit))",
//
@"@_outer_Id='2'
SELECT TOP(1) 1
FROM [Gears] AS [g]
WHERE ([g].[Discriminator] IN (N'Officer', N'Gear') AND (@_outer_Id = [g].[SquadId])) AND ([g].[HasSoulPatch] = CAST(1 AS bit))");
@"SELECT [s].[Name], [t0].[c]
FROM [Squads] AS [s]
LEFT JOIN (
SELECT [t].[c], [t].[Nickname], [t].[SquadId]
FROM (
SELECT 1 AS [c], [g].[Nickname], [g].[SquadId], ROW_NUMBER() OVER(PARTITION BY [g].[SquadId] ORDER BY [g].[Nickname], [g].[SquadId]) AS [row]
FROM [Gears] AS [g]
WHERE [g].[Discriminator] IN (N'Gear', N'Officer') AND ([g].[HasSoulPatch] = CAST(1 AS bit))
) AS [t]
WHERE [t].[row] <= 1
) AS [t0] ON [s].[Id] = [t0].[SquadId]");
}

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

AssertSql(
@"SELECT [s].[Name], [s].[Id]
FROM [Squads] AS [s]",
//
@"@_outer_Id='1'
SELECT TOP(1) 1
FROM [Gears] AS [g]
WHERE ([g].[Discriminator] IN (N'Officer', N'Gear') AND (@_outer_Id = [g].[SquadId])) AND ([g].[HasSoulPatch] = CAST(1 AS bit))",
//
@"@_outer_Id='2'
SELECT TOP(1) 1
FROM [Gears] AS [g]
WHERE ([g].[Discriminator] IN (N'Officer', N'Gear') AND (@_outer_Id = [g].[SquadId])) AND ([g].[HasSoulPatch] = CAST(1 AS bit))");
@"SELECT [s].[Name], [t0].[c], [t0].[c0], [t0].[c1]
FROM [Squads] AS [s]
LEFT JOIN (
SELECT [t].[c], [t].[c0], [t].[c1], [t].[Nickname], [t].[SquadId]
FROM (
SELECT CAST(1 AS bit) AS [c], CAST(0 AS bit) AS [c0], 1 AS [c1], [g].[Nickname], [g].[SquadId], ROW_NUMBER() OVER(PARTITION BY [g].[SquadId] ORDER BY [g].[Nickname], [g].[SquadId]) AS [row]
FROM [Gears] AS [g]
WHERE [g].[Discriminator] IN (N'Gear', N'Officer') AND ([g].[HasSoulPatch] = CAST(1 AS bit))
) AS [t]
WHERE [t].[row] <= 1
) AS [t0] ON [s].[Id] = [t0].[SquadId]");
}

public override async Task Include_with_order_by_constant(bool isAsync)
Expand Down Expand Up @@ -5827,12 +5853,12 @@ public override async Task Select_subquery_projecting_single_constant_null_of_no
await base.Select_subquery_projecting_single_constant_null_of_non_mapped_type(isAsync);

AssertSql(
@"SELECT [s].[Name]
@"SELECT [s].[Name], [t0].[c]
FROM [Squads] AS [s]
LEFT JOIN (
SELECT [t].[Nickname], [t].[SquadId]
SELECT [t].[c], [t].[Nickname], [t].[SquadId]
FROM (
SELECT [g].[Nickname], [g].[SquadId], ROW_NUMBER() OVER(PARTITION BY [g].[SquadId] ORDER BY [g].[Nickname], [g].[SquadId]) AS [row]
SELECT 1 AS [c], [g].[Nickname], [g].[SquadId], ROW_NUMBER() OVER(PARTITION BY [g].[SquadId] ORDER BY [g].[Nickname], [g].[SquadId]) AS [row]
FROM [Gears] AS [g]
WHERE [g].[Discriminator] IN (N'Gear', N'Officer') AND ([g].[HasSoulPatch] = CAST(1 AS bit))
) AS [t]
Expand All @@ -5845,12 +5871,12 @@ public override async Task Select_subquery_projecting_single_constant_of_non_map
await base.Select_subquery_projecting_single_constant_of_non_mapped_type(isAsync);

AssertSql(
@"SELECT [s].[Name]
@"SELECT [s].[Name], [t0].[c]
FROM [Squads] AS [s]
LEFT JOIN (
SELECT [t].[Nickname], [t].[SquadId]
SELECT [t].[c], [t].[Nickname], [t].[SquadId]
FROM (
SELECT [g].[Nickname], [g].[SquadId], ROW_NUMBER() OVER(PARTITION BY [g].[SquadId] ORDER BY [g].[Nickname], [g].[SquadId]) AS [row]
SELECT 1 AS [c], [g].[Nickname], [g].[SquadId], ROW_NUMBER() OVER(PARTITION BY [g].[SquadId] ORDER BY [g].[Nickname], [g].[SquadId]) AS [row]
FROM [Gears] AS [g]
WHERE [g].[Discriminator] IN (N'Gear', N'Officer') AND ([g].[HasSoulPatch] = CAST(1 AS bit))
) AS [t]
Expand Down
Loading

0 comments on commit 64b8642

Please sign in to comment.