- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3.3k
          Simplify TRUE = expr in filtering contexts
          #33776
        
          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
Conversation
TRUE = nullable_expr in filtering contextsTRUE = expr in filtering contexts
      3123871    to
    984c9cd      
    Compare
  
    | 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 | 
There was a problem hiding this comment.
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
| bool allowOptimizedExpansion, | ||
| out bool nullable) | ||
| { | ||
| if (allowOptimizedExpansion && sqlBinaryExpression.OperatorType == ExpressionType.Equal) | 
There was a problem hiding this comment.
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.
| @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.
984c9cd    to
    ece4eb0      
    Compare
  
    | @maumar I added the improvements you suggested 👍 | 
| @ranma42 no need, thank you for another great contribution! | 
While working on #33757 an issue was found regarding a missing optimization of predicates.