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.

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

Simplify TRUE = expr in filtering contexts #33776

Merged
merged 4 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions src/EFCore.Relational/Query/SqlNullabilityProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1174,6 +1174,29 @@ protected virtual SqlExpression VisitSqlBinary(
bool allowOptimizedExpansion,
out bool nullable)
{
// Most optimizations are done in OptimizeComparison below, but this one
// benefits from being done early.
// Consider query: (x.NullableString == "Foo") == true
// We recursively visit Left and Right, but when processing the left
// side, allowOptimizedExpansion would be set to false (we only allow it
// to trickle down to child nodes for AndAlso & OrElse operations), so
// the comparison would get unnecessarily expanded. In order to avoid
// this, we would need to modify the allowOptimizedExpansion calculation
// to capture this scenario and then flow allowOptimizedExpansion to
// OptimizeComparison. Instead, we just do the optimization right away
// and the resulting code is clearer.
if (allowOptimizedExpansion && sqlBinaryExpression.OperatorType == ExpressionType.Equal)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a comment for the future reader why we do the optimization here rather than rely on OptimizeComparison.
Something like:

// Most optimizations are done in OptimizeComparison below, but this one benefits from being done early.
// Consider query: (x.NullableString == "Foo") == true
// We recursively visit Left and Right, but when processing the left side, allowOptimizedExpansion would be set to false (we only allow it to trickle down to child nodes for AndAlso & OrElse operations), so the comparison would get unnecessarily expanded. In order to avoid this, we would need to modify the allowOptimizedExpansion calculation to capture this scenario and then flow allowOptimizedExpansion to OptimizeComparison. Instead, we just do the optimization right away and the resulting code is clearer.

{
if (IsTrue(sqlBinaryExpression.Left) && sqlBinaryExpression.Left.TypeMapping!.Converter == null)
{
return Visit(sqlBinaryExpression.Right, allowOptimizedExpansion, out nullable);
}
else if (IsTrue(sqlBinaryExpression.Right) && sqlBinaryExpression.Right.TypeMapping!.Converter == null)
{
return Visit(sqlBinaryExpression.Left, allowOptimizedExpansion, out nullable);
}
}

var optimize = allowOptimizedExpansion;

allowOptimizedExpansion = allowOptimizedExpansion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,23 @@ join e2 in ss.Set<NullSemanticsEntity2>() on e1.NullableIntA equals e2.NullableI
},
elementSorter: e => (e.Id1, e.Id2));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Join_uses_csharp_semantics_for_anon_objects(bool async)
=> AssertQuery(
async,
ss => from e1 in ss.Set<NullSemanticsEntity1>()
join e2 in ss.Set<NullSemanticsEntity2>() on
new { NullInt = e1.NullableIntA } equals new { NullInt = e2.NullableIntB }
select new
{
Id1 = e1.Id,
Id2 = e2.Id,
e1.NullableIntA,
e2.NullableIntB
},
elementSorter: e => (e.Id1, e.Id2));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Contains_with_local_array_closure_with_null(bool async)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3445,6 +3445,22 @@ public virtual Task Composite_key_join_on_groupby_aggregate_projecting_only_grou
},
(o, i) => i.Key));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Composite_key_join_on_groupby_aggregate_projecting_only_grouping_key2(bool async)
=> AssertQueryScalar(
async,
ss => ss.Set<Level1>()
.Join(
ss.Set<Level2>().GroupBy(g => g.Id % 3).Select(g => new { g.Key, Sum = g.Sum(x => x.Id) }),
o => new { o.Id, Condition = false },
i => new
{
Id = i.Key,
Condition = i.Sum <= 10,
},
(o, i) => i.Key));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Multiple_joins_groupby_predicate(bool async)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4107,8 +4107,27 @@ INNER JOIN (
FROM [LevelTwo] AS [l0]
) AS [l1]
GROUP BY [l1].[Key]
) AS [l2] ON [l].[Id] = [l2].[Key] AND CAST(1 AS bit) = CASE
WHEN [l2].[Sum] > 10 THEN CAST(1 AS bit)
) AS [l2] ON [l].[Id] = [l2].[Key] AND [l2].[Sum] > 10
Copy link
Contributor

Choose a reason for hiding this comment

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

also add a test case for the opposite:

    [ConditionalTheory]
    [MemberData(nameof(IsAsyncData))]
    public virtual Task Composite_key_join_on_groupby_aggregate_projecting_only_grouping_key2(bool async)
        => AssertQueryScalar(
            async,
            ss => ss.Set<Level1>()
                .Join(
                    ss.Set<Level2>().GroupBy(g => g.Id % 3).Select(g => new { g.Key, Sum = g.Sum(x => x.Id) }),
                    o => new { o.Id, Condition = false },
                    i => new
                    {
                        Id = i.Key,
                        Condition = i.Sum < 10,
                    },
                    (o, i) => i.Key));

It will generate CASE for now, but it will be interesting to see what comes out of #33757

""");
}

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

AssertSql(
"""
SELECT [l2].[Key]
FROM [LevelOne] AS [l]
INNER JOIN (
SELECT [l1].[Key], COALESCE(SUM([l1].[Id]), 0) AS [Sum]
FROM (
SELECT [l0].[Id], [l0].[Id] % 3 AS [Key]
FROM [LevelTwo] AS [l0]
) AS [l1]
GROUP BY [l1].[Key]
) AS [l2] ON [l].[Id] = [l2].[Key] AND CAST(0 AS bit) = CASE
WHEN [l2].[Sum] <= 10 THEN CAST(1 AS bit)
ELSE CAST(0 AS bit)
END
""");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -920,8 +920,62 @@ WHEN [l2].[OneToOne_Required_PK_Date] IS NOT NULL AND [l2].[Level1_Required_Id]
WHERE [l2].[OneToOne_Required_PK_Date] IS NOT NULL AND [l2].[Level1_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse2Id] IS NOT NULL
) AS [s]
GROUP BY [s].[Key]
) AS [s1] ON [l].[Id] = [s1].[Key] AND CAST(1 AS bit) = CASE
WHEN [s1].[Sum] > 10 THEN CAST(1 AS bit)
) AS [s1] ON [l].[Id] = [s1].[Key] AND [s1].[Sum] > 10
""");
}

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

AssertSql(
"""
SELECT [s1].[Key]
FROM [Level1] AS [l]
INNER JOIN (
SELECT [s].[Key], (
SELECT COALESCE(SUM(CASE
WHEN [l7].[OneToOne_Required_PK_Date] IS NOT NULL AND [l7].[Level1_Required_Id] IS NOT NULL AND [l7].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l7].[Id]
END), 0)
FROM (
SELECT [l3].[Id], CASE
WHEN [l4].[OneToOne_Required_PK_Date] IS NOT NULL AND [l4].[Level1_Required_Id] IS NOT NULL AND [l4].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l4].[Id]
END % 3 AS [Key]
FROM [Level1] AS [l3]
LEFT JOIN (
SELECT [l5].[Id], [l5].[OneToOne_Required_PK_Date], [l5].[Level1_Required_Id], [l5].[OneToMany_Required_Inverse2Id]
FROM [Level1] AS [l5]
WHERE [l5].[OneToOne_Required_PK_Date] IS NOT NULL AND [l5].[Level1_Required_Id] IS NOT NULL AND [l5].[OneToMany_Required_Inverse2Id] IS NOT NULL
) AS [l4] ON [l3].[Id] = CASE
WHEN [l4].[OneToOne_Required_PK_Date] IS NOT NULL AND [l4].[Level1_Required_Id] IS NOT NULL AND [l4].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l4].[Id]
END
WHERE [l4].[OneToOne_Required_PK_Date] IS NOT NULL AND [l4].[Level1_Required_Id] IS NOT NULL AND [l4].[OneToMany_Required_Inverse2Id] IS NOT NULL
) AS [s0]
LEFT JOIN (
SELECT [l6].[Id], [l6].[OneToOne_Required_PK_Date], [l6].[Level1_Required_Id], [l6].[OneToMany_Required_Inverse2Id]
FROM [Level1] AS [l6]
WHERE [l6].[OneToOne_Required_PK_Date] IS NOT NULL AND [l6].[Level1_Required_Id] IS NOT NULL AND [l6].[OneToMany_Required_Inverse2Id] IS NOT NULL
) AS [l7] ON [s0].[Id] = CASE
WHEN [l7].[OneToOne_Required_PK_Date] IS NOT NULL AND [l7].[Level1_Required_Id] IS NOT NULL AND [l7].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l7].[Id]
END
WHERE [s].[Key] = [s0].[Key] OR ([s].[Key] IS NULL AND [s0].[Key] IS NULL)) AS [Sum]
FROM (
SELECT CASE
WHEN [l2].[OneToOne_Required_PK_Date] IS NOT NULL AND [l2].[Level1_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l2].[Id]
END % 3 AS [Key]
FROM [Level1] AS [l0]
LEFT JOIN (
SELECT [l1].[Id], [l1].[OneToOne_Required_PK_Date], [l1].[Level1_Required_Id], [l1].[OneToMany_Required_Inverse2Id]
FROM [Level1] AS [l1]
WHERE [l1].[OneToOne_Required_PK_Date] IS NOT NULL AND [l1].[Level1_Required_Id] IS NOT NULL AND [l1].[OneToMany_Required_Inverse2Id] IS NOT NULL
) AS [l2] ON [l0].[Id] = CASE
WHEN [l2].[OneToOne_Required_PK_Date] IS NOT NULL AND [l2].[Level1_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse2Id] IS NOT NULL THEN [l2].[Id]
END
WHERE [l2].[OneToOne_Required_PK_Date] IS NOT NULL AND [l2].[Level1_Required_Id] IS NOT NULL AND [l2].[OneToMany_Required_Inverse2Id] IS NOT NULL
) AS [s]
GROUP BY [s].[Key]
) AS [s1] ON [l].[Id] = [s1].[Key] AND CAST(0 AS bit) = CASE
WHEN [s1].[Sum] <= 10 THEN CAST(1 AS bit)
ELSE CAST(0 AS bit)
END
""");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -945,7 +945,7 @@ public override async Task Null_propagation_optimization1(bool async)
"""
SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank]
FROM [Gears] AS [g]
WHERE [g].[LeaderNickname] = N'Marcus' AND [g].[LeaderNickname] IS NOT NULL
WHERE [g].[LeaderNickname] = N'Marcus'
""");
}

Expand Down Expand Up @@ -999,10 +999,7 @@ FROM [Gears] AS [g]
WHERE CASE
WHEN [g].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([g].[LeaderNickname]) AS int)
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([g].[LeaderNickname]) AS int)
END IS NOT NULL
END = 5
""");
}

Expand All @@ -1018,10 +1015,7 @@ FROM [Gears] AS [g]
WHERE CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand All @@ -1037,10 +1031,7 @@ FROM [Gears] AS [g]
WHERE CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -906,6 +906,18 @@ FROM [Entities1] AS [e]
""");
}

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

AssertSql(
"""
SELECT [e].[Id] AS [Id1], [e0].[Id] AS [Id2], [e].[NullableIntA], [e0].[NullableIntB]
FROM [Entities1] AS [e]
INNER JOIN [Entities2] AS [e0] ON [e].[NullableIntA] = [e0].[NullableIntB] OR ([e].[NullableIntA] IS NULL AND [e0].[NullableIntB] IS NULL)
""");
}

public override async Task Contains_with_local_array_closure_with_null(bool async)
{
await base.Contains_with_local_array_closure_with_null(async);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1319,7 +1319,7 @@ UNION ALL
SELECT [o].[Nickname], [o].[SquadId], [o].[AssignedCityName], [o].[CityOfBirthName], [o].[FullName], [o].[HasSoulPatch], [o].[LeaderNickname], [o].[LeaderSquadId], [o].[Rank], N'Officer' AS [Discriminator]
FROM [Officers] AS [o]
) AS [u]
WHERE [u].[LeaderNickname] = N'Marcus' AND [u].[LeaderNickname] IS NOT NULL
WHERE [u].[LeaderNickname] = N'Marcus'
""");
}

Expand Down Expand Up @@ -1391,10 +1391,7 @@ FROM [Officers] AS [o]
WHERE CASE
WHEN [u].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([u].[LeaderNickname]) AS int)
END = 5 AND CASE
WHEN [u].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([u].[LeaderNickname]) AS int)
END IS NOT NULL
END = 5
""");
}

Expand All @@ -1416,10 +1413,7 @@ FROM [Officers] AS [o]
WHERE CASE
WHEN [u].[LeaderNickname] IS NOT NULL THEN CAST(LEN([u].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [u].[LeaderNickname] IS NOT NULL THEN CAST(LEN([u].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand All @@ -1441,10 +1435,7 @@ FROM [Officers] AS [o]
WHERE CASE
WHEN [u].[LeaderNickname] IS NOT NULL THEN CAST(LEN([u].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [u].[LeaderNickname] IS NOT NULL THEN CAST(LEN([u].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,7 @@ WHEN [o].[Nickname] IS NOT NULL THEN N'Officer'
END AS [Discriminator]
FROM [Gears] AS [g]
LEFT JOIN [Officers] AS [o] ON [g].[Nickname] = [o].[Nickname] AND [g].[SquadId] = [o].[SquadId]
WHERE [g].[LeaderNickname] = N'Marcus' AND [g].[LeaderNickname] IS NOT NULL
WHERE [g].[LeaderNickname] = N'Marcus'
""");
}

Expand Down Expand Up @@ -1211,10 +1211,7 @@ FROM [Gears] AS [g]
WHERE CASE
WHEN [g].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([g].[LeaderNickname]) AS int)
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([g].[LeaderNickname]) AS int)
END IS NOT NULL
END = 5
""");
}

Expand All @@ -1233,10 +1230,7 @@ FROM [Gears] AS [g]
WHERE CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand All @@ -1255,10 +1249,7 @@ FROM [Gears] AS [g]
WHERE CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,7 @@ public override async Task Null_propagation_optimization1(bool async)
"""
SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[PeriodEnd], [g].[PeriodStart], [g].[Rank]
FROM [Gears] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' AS [g]
WHERE [g].[LeaderNickname] = N'Marcus' AND [g].[LeaderNickname] IS NOT NULL
WHERE [g].[LeaderNickname] = N'Marcus'
""");
}

Expand Down Expand Up @@ -1605,10 +1605,7 @@ public override async Task Null_propagation_optimization6(bool async)
WHERE CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand Down Expand Up @@ -4039,10 +4036,7 @@ public override async Task Null_propagation_optimization5(bool async)
WHERE CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NOT NULL THEN CAST(LEN([g].[LeaderNickname]) AS int)
ELSE NULL
END IS NOT NULL
END = 5
""");
}

Expand Down Expand Up @@ -4971,10 +4965,7 @@ public override async Task Null_propagation_optimization4(bool async)
WHERE CASE
WHEN [g].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([g].[LeaderNickname]) AS int)
END = 5 AND CASE
WHEN [g].[LeaderNickname] IS NULL THEN NULL
ELSE CAST(LEN([g].[LeaderNickname]) AS int)
END IS NOT NULL
END = 5
""");
}

Expand Down
Loading
Loading