QueryRewrite: when comparing entity with composite key to null, only check it's first key property, rather than all of them

[maumar]

Query:

from g1 in gs
join g2 in gs on g1.HasSoulPatch equals true into grouping
from g2 in grouping.DefaultIfEmpty()
orderby g2.Nickname
select g2 != null
  ? new
  {
	  g2.Nickname,
	  Five = 5
  }
  : null

Expected:

SELECT CASE
    WHEN [t].[Nickname] IS NOT NULL
    THEN CAST(1 AS bit) ELSE CAST(0 AS bit)
END, [t].[Nickname]
FROM [Gears] AS [g1]
LEFT JOIN (
    SELECT [g2].*
    FROM [Gears] AS [g2]
    WHERE [g2].[Discriminator] IN (N'Officer', N'Gear')
) AS [t] ON [g1].[HasSoulPatch] = CAST(1 AS bit)
WHERE [g1].[Discriminator] IN (N'Officer', N'Gear')
ORDER BY [t].[Nickname]

current:

SELECT CASE
    WHEN [t].[Nickname] IS NOT NULL OR [t].[SquadId] IS NOT NULL
    THEN CAST(1 AS bit) ELSE CAST(0 AS bit)
END, [t].[Nickname]
FROM [Gears] AS [g1]
LEFT JOIN (
    SELECT [g2].*
    FROM [Gears] AS [g2]
    WHERE [g2].[Discriminator] IN (N'Officer', N'Gear')
) AS [t] ON [g1].[HasSoulPatch] = CAST(1 AS bit)
WHERE [g1].[Discriminator] IN (N'Officer', N'Gear')
ORDER BY [t].[Nickname]");

Tests affected:
Select_null_propagation_negative4
Select_null_propagation_negative5

[ajcvickers]

@maumar What if one key value is null and the other is non-null? This should be treated as a null key, but the first property may not be null.

[smitpatel]

@ajcvickers - We may have code in place right now which checks only first null key rather than all of them. Since none of the key should be null if stored to database by EF. Is there any incorrect assumption here?

[ajcvickers]

@smitpatel If any part of a composite key is null, then the whole key is considered null. So I think the assumption is correct.

[smitpatel]

So you need to tell us what is correct query for above linq. In current source code, we do generate expected SQL. But it is just checking first property being not null.

[ajcvickers]

@smitpatel If the first value is null => key is null. If the first value is non-null => we don't know if the key is null or not. So we can't "only check its first property" but we can short-circuit the decision as soon as we find a null value. If that's what we're doing already, then fine. If that's what we're going to do, then fine. The issue filed doesn't say that. It says we only need to check the value of one property, and that's not fine.

[smitpatel]

The current SQL only checks first property being non null when translating entity is non-null.

[smitpatel]

So it's a bug in entity equality rewrite. Bump priority?

[ajcvickers]

Re-triage.

[ajcvickers]

See #20164

[smitpatel]

efcore/src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs

Lines 889 to 895 in 7166201

	// When comparing an entity to null, it's sufficient to simply compare its first primary key column to null.
	// (this is also why we can do it even over a subquery with a composite key)
	return Expression.MakeBinary(
	equality ? ExpressionType.Equal : ExpressionType.NotEqual,
	CreatePropertyAccessExpression(nonNullExpression, keyProperties[0]),
	Expression.Constant(null));
	}

[smitpatel]

Following @ajcvickers statement
Implemented behavior:

• If any part of composite key is null then key is null.
• If comparing entity with null then check if "any" key value is null.
• If comparing entity with non-null then check if "all" key values are non null.

please review @ajcvickers @AndriySvyryd