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

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented May 22, 2024

While working on #33757 an issue was found regarding a missing optimization of predicates.

@ranma42 ranma42 changed the title Simplify TRUE = nullable_expr in filtering contexts Simplify TRUE = expr in filtering contexts May 22, 2024
@roji roji requested a review from maumar May 22, 2024 06:17
@ranma42 ranma42 marked this pull request as ready for review May 22, 2024 07:24
WHEN [l2].[Sum] > 10 THEN CAST(1 AS bit)
ELSE CAST(0 AS bit)
END
) 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

@@ -1174,6 +1174,18 @@ SqlExpression GenerateNotNullCheck(SqlExpression operand)
bool allowOptimizedExpansion,
out bool nullable)
{
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.

@maumar
Copy link
Contributor

maumar commented May 23, 2024

@ranma42 just test and the comment, otherwise looks great!

for comparison with the `true == nullableExpr` case, which is (soon-to-be)
optimized explicitly.
For join where keys are anonymous objects we want C# null semantics to mimic
LINQ to objects behavior.
@ranma42
Copy link
Contributor Author

ranma42 commented May 23, 2024

@maumar I added the improvements you suggested 👍
Should I mention you as the author in the commit messages?

@maumar maumar merged commit de2e17e into dotnet:main May 24, 2024
5 of 7 checks passed
@maumar
Copy link
Contributor

maumar commented May 24, 2024

@ranma42 no need, thank you for another great contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants