Skip to content
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

efcore6 produces wrong SELECT SQL when Where/OrderBy clauses are applied to Owned properties followed by .Take() #26592

Closed
DevTrevi opened this issue Nov 9, 2021 · 10 comments · Fixed by #26641
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-approved type-bug
Milestone

Comments

@DevTrevi
Copy link

DevTrevi commented Nov 9, 2021

Hi everyone,

I found a bug in efcore6 that now produces an invalid SELECT statement when filtering/sorting on owned types stored on other tables.
In particular, the bug happens when more than one owned type is linked to an entity and the where clause is applied NOT to the last owned property, followed by a .Take() clause.

Can you please check this behavior? Thanks in advance!

Example

A Company entity that can be either a Customer, a Supplier or both.
Additional Customer data and Supplier data are stored in separate tables with a 1..1 relationship with the Company table.
These tables are configured as Owned properties of Customer:

/// <summary>
/// Entity representing a company. 
/// A company can be either a customer, a supplier, or both.
/// If the company is a cutomer and/or a supplier, additional data describing customer/supplier is stored in
/// owned types CustomerData and SupplierData, stored in two separate tables with a 1..1 relationship with Company
/// </summary>
public class Company
{
    public int Id { get; set; }

    public string? Name { get; set; }
    public CustomerData? CustomerData { get; set; }
    public SupplierData? SupplierData { get; set; }
}

/// <summary>
/// Owned 1 of Company in 1..1 relationship, on another table
/// </summary>
public class CustomerData
{
    public int Id { get; set; }
    public string? AdditionalCustomerData { get; set; }

}

 /// <summary>
 /// Owned 2 of Company in 1..1 relationship, on another table
 /// </summary>
 public class SupplierData
 {
     public int Id { get; set; }
     public string? AdditionalSupplierData { get; set; }

 }

Mapping:

internal class CompanyMapping : IEntityTypeConfiguration<Company>
{
    public void Configure(EntityTypeBuilder<Company> builder)
    {
        builder.ToTable("Companies");

        builder.HasKey(r => r.Id);
        builder.Property(r=>r.Id).UseIdentityColumn();

        builder.OwnsOne(r => r.CustomerData,
            b =>
            {
                b.ToTable("CustomerData");

                b.HasKey(r => r.Id);

                b.WithOwner().HasForeignKey(r => r.Id).HasPrincipalKey(r => r.Id);
            });

        builder.OwnsOne(r => r.SupplierData,
            b =>
            {
                b.ToTable("SupplierData");

                b.HasKey(r => r.Id);

                b.WithOwner().HasForeignKey(r => r.Id).HasPrincipalKey(r => r.Id);
            });
    }
}

Executing the following query

var ctx = GetDbContext();

var q = ctx.Companies
        .Where(r => r.CustomerData != null)
        .Take(10);

var results = await q.ToArrayAsync();

will produce the following wrong SQL when using efcore6:

-- sql generated by efcore6 (wrong)
-- Columns of the first owned type are NOT included in the inner select, so they are not available in the outer select
-- A left join is also missing from the outer select
DECLARE @__p_0 int = 10;

SELECT[t].[Id], [t].[Name], [c0].[Id], [c0].[AdditionalCustomerData], [s].[Id], [s].[AdditionalSupplierData]
FROM(
    SELECT TOP(@__p_0)[c].[Id], [c].[Name]
    FROM[Companies] AS[c]
    LEFT JOIN[CustomerData] AS[c0] ON[c].[Id] = [c0].[Id]
    WHERE[c0].[Id] IS NOT NULL
) AS[t]
LEFT JOIN[SupplierData] AS[s] ON[t].[Id] = [s].[Id]

-- Error: The multi-part identifier "c0.AdditionalCustomerData" could not be bound.

the same query works as expected in efcore5, producing the following valid SQL:

-- sql generated by efcore5 (working)
DECLARE @__p_0 int = 10;

SELECT[t].[Id], [t].[Name], [t].[Id0], [t].[AdditionalCustomerData], [s0].[Id], [s0].[AdditionalSupplierData]
FROM(
    SELECT TOP(@__p_0)[c].[Id], [c].[Name], [c0].[Id] AS[Id0], [c0].[AdditionalCustomerData]
    FROM[Companies] AS[c]
    LEFT JOIN[CustomerData] AS[c0] ON[c].[Id] = [c0].[Id]
    WHERE[c0].[Id] IS NOT NULL
) AS[t]
LEFT JOIN[SupplierData] AS[s] ON[t].[Id] = [s].[Id]
LEFT JOIN[SupplierData] AS[s0] ON[t].[Id] = [s0].[Id]

Attached solution

EfCore6Bugs.zip

The solution contains projects targeting .net5.0 with efcore 5.0.12 and .net6.0 with efcore 6.0.0.
The first project contains the DbContext, the second one is a simple unit test.

On test initialization, an EfCore6Bugs database will be created on (LocalDb)\MSSQLLocalDB (please customize the connection string in TestDataContext.cs as needed) applying migrations for the DbContext.
Unit tests should all be successful for .net5.0 (efcore5), while EfCore6ClausesOnFirstOwnedFails will fail for .net6.0 (efcore6)

@smitpatel
Copy link
Contributor

Unrelated note - the SQL generated in 5.0 caused unnecessary join with SupplierData

@smitpatel
Copy link
Contributor

smitpatel commented Nov 9, 2021

Potential root cause, we didn't update references to CustomerData owned entity after pushing down into subquery caused by Take(10)

Expanding other navigations causes initial SelectExpression to change which causes earlier expanded owned navigations to be incorrect. This won't be an easy fix.

smitpatel added a commit that referenced this issue Nov 10, 2021
… generated

Issue: Expanding owned navigations in relational falls into 3 categories
- Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression
- Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level
- Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point.

In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.

This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.

Resolves #26592

The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.

The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, which we can do in 7.0.
smitpatel added a commit that referenced this issue Nov 10, 2021
… generated

Issue: Expanding owned navigations in relational falls into 3 categories
- Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression
- Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level
- Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point.

In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.

This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.

Resolves #26592

The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.

The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, which we can do in 7.0.
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 10, 2021
smitpatel added a commit that referenced this issue Nov 11, 2021
… generated

Issue: Expanding owned navigations in relational falls into 3 categories
- Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression
- Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level
- Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point.

In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.

This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.

Resolves #26592

The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.

The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, which we can do in 7.0.
smitpatel added a commit that referenced this issue Nov 11, 2021
… generated

Issue: Expanding owned navigations in relational falls into 3 categories
- Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression
- Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level
- Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point.

In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.

This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.

Resolves #26592

The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.

The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, which we can do in 7.0.
smitpatel added a commit that referenced this issue Nov 11, 2021
… generated

Issue: Expanding owned navigations in relational falls into 3 categories
- Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression
- Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level
- Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point.

In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.

This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.

Resolves #26592

The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.
The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, submitting in a different PR in case we need to cherry-pick this for patch.
@smitpatel smitpatel added this to the 7.0.0 milestone Nov 11, 2021
@smitpatel
Copy link
Contributor

Due to fix being on riskier side, we are fixing this issue in 7.0 release (& nightly builds). We will determine to patch this in future if we get more customers hitting this.

ghost pushed a commit that referenced this issue Nov 19, 2021
… generated (#26641)

Issue: Expanding owned navigations in relational falls into 3 categories
- Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression
- Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level
- Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point.

In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.

This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.

Resolves #26592

The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.
The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, submitting in a different PR in case we need to cherry-pick this for patch.
@ChristopherHaws
Copy link
Contributor

We have now hit this issue several times in our app.

@ajcvickers ajcvickers removed this from the 7.0.0 milestone Dec 3, 2021
@ajcvickers
Copy link
Member

Now two customer reports; re-opening to re-discuss patching.

@ajcvickers ajcvickers reopened this Dec 3, 2021
@ajcvickers ajcvickers added this to the 6.0.x milestone Dec 8, 2021
@ajcvickers
Copy link
Member

Note from triage: @smitpatel Can you open a PR against release/6.0 and we will take this back to Tactics.

smitpatel added a commit that referenced this issue Dec 8, 2021
… generated

Issue: Expanding owned navigations in relational falls into 3 categories
- Collection navigation - which always generates a subquery. The predicate is generated in LINQ to allow mutation of outer SelectExpression
- Reference navigation sharing table - which picks column from same table without needing to add additional join. This only mutate the projection list for SelectExpression at subquery level
- Reference navigation mapped to separate table - which generates additional joins. Generating join can cause push down on outer SelectExpression if it has facets (e.g. limit/offset). This pushdown causes owned expansion from category 2 to be outdated and invalid SQL since they get pushed down to subquery. While their relevant entity projection is updated inside SelectExpression we already inlined older object in the tree at this point.

In order to avoid issue with outdated owned expansion, we defer actual inline-ing while processing owned navigations so that all navigations are expanded (causing any mutation on SelectExpression) before we inline values.

This PR introduces DeferredOwnedExpansionExpression which remembers the source projection binding to SelectExpression (which will remain accurate through pushdown), and navigation chain to traverse entity projections to reach entity shaper for final owned navigation. This way, we get up-to-date information from SelectExpression after all joins are generated.
We also find updated entity projection through binding after we generate a join if pushdown was required.

Resolves #26592

The issue was also present in 5.0 release causing non-performant SQL rather than invalid SQL. During 5.0 we expanded owned navigations again while during client eval phase (which happens in customer scenario due to include). This caused to earlier owned reference to have correct columns. Though the entity projection for owner was changed due to pushdown so we didn't add latter reference navigation binding in correct entity projection causing us to expand it again during 2nd pass.

The exact same issue doesn't occur for InMemory provider (due to slightly different implementation) though we should also make InMemory provider work this way, which we can do in 7.0.
@jwyoung9
Copy link

Just upgraded an app to 6.0 and am also hitting this issue.

@tuggernuts
Copy link

Happy New Year. I just ran into a similar issue (in 6.0.1) which I'm guessing is related.

In short, an entity with 2 owned types - one of which is in an external table - will have the same error as OP when using Take().

I have a repo here.

If this is unrelated - and a bug - please let me know if you want me to open a separate issue.

@ajcvickers
Copy link
Member

@cdrfenix Please open a new issue and attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.

@rofenix2
Copy link

rofenix2 commented Oct 6, 2022

My bad the query was well generated, i just didnt realize it till i checked in the execution plan.
I will delete my old post, thanks for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-approved type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants