[release/3.1] Query: Simplify SQL generated for optional owned/weak entities when table splitting

[smitpatel]

Partial fix for #18299

Description

Issue: The way we construct SelectExpression for owned entity type involves adding inner joins to owner when table splitting so that only owned entity for given owner is in result. Considering the fact that any owned entity translation would originate from a query root which would be non-owned, the join condition is unnecessary when it is same as outer query root which initiated this owned entity.
Fix: When we are generating join condition for linking FK if the FK is ownership for entity type and principal type is root entity type then the join condition would be redundant hence we skip adding it.

Notes:

• The joins would be unnecessary even for derived type if the owned navigation is on base type or the given entity type but integrating that information involves breaking change to API. Essentially when generating SelectExpression for owned entity we need to keep track of which selectExpression initiated it.

When generating SQL for owned entity we add inner join with the owner to make sure that we are selecting owned entity for requested owner. But in some cases the inner join can be eliminated as outer filter achieves the same effect.

Customer Impact

Many customers are unhappy with the way queries are generated for these cases since 3.0. This was due to a design change which in retrospect we probably should not have made in 3.0 since it is not complete enough for them to get back to the 2.x behavior.

We have analyzed the design change holistically and concluded that it cannot be fundamentally changed in a patch. However, we are now looking at tactical changes to specific cases that we can patch. This is the first of those changes.

How found

Reported by multiple customers.

Test coverage

We have test coverage, but the generated SQL isn't what customers want.

Regression?

This is a regression in experience due to an intentional design change.

Risk

Low. Affects only in case of owned entity.

Also, fix is on by default, but quirk switch is present to revert back to old behavior.

[smitpatel]

Generated SQL for both the query in original issue

SELECT [o].[Id], [o].[Title], [t].[Id], [t].[Address_City], [t].[Address_Street]
FROM [Order] AS [o]
LEFT JOIN (
    SELECT [o0].[Id], [o0].[Address_City], [o0].[Address_Street]
    FROM [Order] AS [o0]
    WHERE [o0].[Address_Street] IS NOT NULL OR [o0].[Address_City] IS NOT NULL
) AS [t] ON [o].[Id] = [t].[Id]
WHERE [t].[Id] IS NULL

SELECT [o].[Id], [o].[Title], [t].[Id], [t].[Address_City], [t].[Address_Street]
FROM [Order] AS [o]
LEFT JOIN (
    SELECT [o0].[Id], [o0].[Address_City], [o0].[Address_Street]
    FROM [Order] AS [o0]
    WHERE [o0].[Address_Street] IS NOT NULL OR [o0].[Address_City] IS NOT NULL
) AS [t] ON [o].[Id] = [t].[Id]
WHERE [t].[Address_City] = N'Rome'

[ajcvickers]

@smitpatel "involves breaking change to API." Can you point to specifically what the breaking change would be?

[smitpatel]

efcore/src/EFCore.Relational/Query/ISqlExpressionFactory.cs

Line 190 in 4667074

SelectExpression Select([NotNull] IEntityType entityType);

The interface needs another overload which can pass in what is the parent entityType expanding this owned/weak entity. So if the owned navigation is defined on the particular entityType or its base type then we can remove join in few more cases.

[ajcvickers]

@smitpatel Can you open a separate issue for the interface change. Could we not do this without breaking by making use of interface default implementations? This would still be adding API surface in a patch, but given EF isn't in the shared framework and also where this API is and how it is used that may be okay.

[ajcvickers]

@smitpatel Approved by Tactics and branches are open; let's try to get this merged today.