Fix to #35393 - GroupJoin in EF Core 9 Returns Null for Joined Entities #35395
Conversation
There was a problem hiding this comment.
Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.
Files not reviewed (7)
- src/EFCore.Relational/Query/SqlNullabilityProcessor.cs: Evaluated as low risk
- test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs: Evaluated as low risk
- test/EFCore.Specification.Tests/Query/NorthwindJoinQueryTestBase.cs: Evaluated as low risk
- test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs: Evaluated as low risk
- test/EFCore.SqlServer.FunctionalTests/Query/NorthwindJoinQuerySqlServerTest.cs: Evaluated as low risk
- test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs: Evaluated as low risk
- test/EFCore.SqlServer.FunctionalTests/Query/TPCGearsOfWarQuerySqlServerTest.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)
test/EFCore.Sqlite.FunctionalTests/Query/NorthwindJoinQuerySqliteTest.cs:61
- The new test case 'GroupJoin_on_true_equal_true' does not have assertions for the actual SQL generated. Consider adding assertions to verify the SQL output.
public override async Task GroupJoin_on_true_equal_true(bool async)
src/EFCore.Relational/Query/SqlExpressionFactory.cs:660
- Ensure that the new behavior for
Notexpressions, especially the removed optimizations, are covered by tests.
// !(a AND b) -> !a OR !b (De Morgan)
|
/cc @ranma42 |
| @@ -4452,7 +4452,7 @@ INNER JOIN ( | |||
| FROM [Factions] AS [f] | |||
| WHERE [f].[Name] = N'Swarm' | |||
| ) AS [f0] ON [l].[Name] = [f0].[CommanderName] | |||
| WHERE [f0].[Eradicated] = CAST(0 AS bit) OR [f0].[Eradicated] IS NULL | |||
| WHERE [f0].[Eradicated] <> CAST(1 AS bit) OR [f0].[Eradicated] IS NULL | |||
There was a problem hiding this comment.
we could maybe keep this form by adding/duplicating "use equality where possible" section to the SqlNullabilityProcessor.OptimizeComparison - worth?
There was a problem hiding this comment.
several DBs can use indexes on = filters, but not on <> (even on bool/enum columns) as per #34164
There was a problem hiding this comment.
I believe that #34166 would do that already and even make this whole optimization pointless (it takes care of the interesting case)... which makes me wonder if it handles correctly these trivial cases 👀
There was a problem hiding this comment.
Confirmed:
- Prefer equality in boolean comparisons #34166 already undoes all of these changes; see 0eb325a
- removing the
!(a == constBool)optimization would cause some minimal regressions when the boolean expression has a value converter; this would be fixed by implementing the!(a == b)->a != btransformation inUpdate(tracked in Apply early SqlExpression optimizations (e.g. x AND true -> x) in VisitChildren #34556 )
It is actually wrong regardless of it being a constant/null: it is wrong when
i.e. it is an optimization that is legitimate on SQL EDIT: mentioned the non-nullable case. |
|
I guess I should investigate why it did not come up earlier in testing 🤔 |
|
most of those cases are simplified by the funcletizer, so would never show up in tests. This case fails because EF does the rewrite itself, after the funcletizer has done it's job. Its possible to create a case that bypasses funcletizer but it's bit contrived. It's a tricky bug for sure, I was looking at truth tables for the optimization and it all worked fine (in 3-value logic). Easy to miss that it's wrong for 2-value, usually it's the other way around :D |
test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindJoinQuerySqlServerTest.cs
Show resolved
Hide resolved
Problem was that in EF9 we moved some optimizations from sql nullability processor to SqlExpressionFactory (so that we optimize things early). One of the optimizations: ``` !(true == a) -> false == a !(false == a) -> true == a ``` is not safe to do when a is nullable. Fix is to constrain this optimization in SqlExpressionFactory to only work on argument which we know is not null (constant, column, non-nullable parameter) and do the comprehensive one back in OptimizeNotExpression, once we've converted everything we could to IS NULL checks already. Fixes #35393
Problem was that in EF9 we moved some optimizations from sql nullability processor to SqlExpressionFactory (so that we optimize things early). One of the optimizations:
is not safe to do when a is a constant or parameter null value, because it evaluates to true IS NULL (two value logic). Fix is to remove this optimization from SqlExpressionFactory and instead put it back into OptimizeNotExpression, once we've converted everything we could to IS NULL checks already.
Fixes #35393