Translate to NULLIF #35327
Translate to NULLIF #35327
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dotnet/efteam this is ready for review. |
| // a == null ? null : a -> a | ||
| // a != null ? a : null -> a |
There was a problem hiding this comment.
we might generalize this to
// a == b ? b : a -> a
// a != b ? a : b -> a
but we might want to wait for fast comparison before (yes, I should really get that done 😅 )
There was a problem hiding this comment.
let's reference #34149 here so it's easier to keep track of this
There was a problem hiding this comment.
So actually, this PR already introduces deep equality (to compare the a's in a == b ? null : a), doing what you suggest would only introduce another one.
In these kinds of very constrained cases, where the deep equality only happens after a very specific pattern match, I think it's OK to do this even without the fast comparison (in other contexts probably not). So I've just gone ahead and done this (pushed a commit).
Let me know how it looks!
There was a problem hiding this comment.
I believe it looks very good 🚀
Specifically, this simplifies away a bunch of
WHERE CASE
WHEN [g].[HasSoulPatch] = CAST(0 AS bit) THEN CAST(0 AS bit)
ELSE [g].[HasSoulPatch]
END = CAST(0 AS bit)
to
WHERE [g].[HasSoulPatch] = CAST(0 AS bit)
which can likely lead to much more efficient queries (the CASE query is unlikely to use an index).
Sorry for causing so much churn that is not really related to NULLIF, but now the PR takes care of simplifying many more (in)equality + ternary patterns :)
There was a problem hiding this comment.
Not at all, this is all great and your comments have been very helpful.
| SELECT CASE | ||
| WHEN "c"."Name" IS NOT NULL THEN "c"."Name" | ||
| END | ||
| SELECT "c"."Name" |
roji
force-pushed
the
NullIf
branch
3 times, most recently
from
770d8be to
8afb824
Compare
| WHEN [e].[NullableStringA] = [e].[NullableStringB] OR ([e].[NullableStringA] IS NULL AND [e].[NullableStringB] IS NULL) THEN [e].[NullableStringA] | ||
| ELSE [e].[NullableStringB] | ||
| END IS NOT NULL) | ||
| WHERE ([e].[NullableStringC] <> [e].[NullableStringB] OR [e].[NullableStringC] IS NULL OR [e].[NullableStringB] IS NULL) AND ([e].[NullableStringC] IS NOT NULL OR [e].[NullableStringB] IS NOT NULL) |
There was a problem hiding this comment.
@ranma42 and @dotnet/efteam check this change out :)
There was a problem hiding this comment.
the original condition is
e => e.NullableStringC
!= (e.NullableStringA == e.NullableStringB
? e.NullableStringA
: e.NullableStringB
which simplifies to
e => e.NullableStringC != e.NullableStringB
so the SQL translation looks correct to me
a different question is: what is the test checking for?
maybe it relied on the assumption that some of the (sub)expressions were not transformed 🤔
There was a problem hiding this comment.
Yep, the test needs to be modified here - it's exercising general null semantics for conditional expressions, but happens to be written in a way which can be simplified away.
There was a problem hiding this comment.
FYI I've modified the test to avoid the simplification taking place.
roji
force-pushed
the
NullIf
branch
2 times, most recently
from
fa790d7 to
b7c0e4a
Compare
Originally done for PostgreSQL by @WhatzGames in npgsql/efcore.pg#3403. Since all relational databases support it, moved this to EFCore.Relational and implemented as a construction-time optimization (in SqlExpressionFactory), so that it potentially applies in scenarios other than just translation.
Closes #31682