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

v1.1 - The multi-part identifier "x.Id" could not be bound #7033

Closed
ctolkien opened this issue Nov 17, 2016 · 8 comments
Closed

v1.1 - The multi-part identifier "x.Id" could not be bound #7033

ctolkien opened this issue Nov 17, 2016 · 8 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@ctolkien
Copy link

The following linq:

var foo = await _context.Things
                .Select(x => new ThingViewModel
                {
                    Name = x.Name,
                    Bars = x.Bars
                        .Take(5)
                        .Select(q => new ThingViewModel.BarViewModel {
                            OtherName = q.Name,
                    })
                }).ToListAsync();

Generates a Select N+1 of the following:

SELECT [t].[Name]
FROM (
    SELECT TOP(5) [q0].*
    FROM [Bars] AS [q0]
    WHERE [x].[Id] = [q0].[ThingId]
) AS [t]

This is invalid SQL, as [x] doesn't exist here. Removing the .Take(5) allows the query to proceed without issue (it is a select n+1, but I presume this is by design for now).

@ctolkien ctolkien changed the title 1.1 - The multi-part identifier "x.Id" could not be bound v1.1 - The multi-part identifier "x.Id" could not be bound Nov 17, 2016
@ctolkien
Copy link
Author

I should also flag, this is a regression from v1.0.

@smitpatel smitpatel self-assigned this Nov 17, 2016
@smitpatel
Copy link
Member

We do client eval in this case producing N+1 queries.
In the second query, ParameterInjection is faulty hence we generated incorrect SQL instead of having injected parameter.

@smitpatel
Copy link
Member

Possible workarounds

  1. Makes 2 queries but fetches all records from second table though only relevant columns. So if there are many Bars for each Thing it may perform poorly.
var foo1 = db.Things.Select(t => new {Id = t.Id, Name = t.Name}).ToList();
var bars =
    db.Set<Bar>()
        .Select(b => new {Id = b.ThingId, OtherName = b.Name})
        .GroupBy(t => t.Id)
        .Select(g => new {Id = g.Key, Bars = g.Take(5)})
        .ToList();

var foo = foo1.Join(bars, f1 => f1.Id, f2 => f2.Id,
    (f1, f2) => new { Name = f1.Name, Bars = f2.Bars }).ToList();
  1. N+1 queries but only fetches required rows from Bars table. (Also fetches all columns)
var temp = db.Things.Select(t => new { t.Name, Bars = t.Bars.Take(5) }).ToList();
var foo = temp.Select(t => new {Name = t.Name, Bars = t.Bars.Select(b => new {OtherName = b.Name})}).ToList();

@divega
Copy link
Contributor

divega commented Jan 25, 2017

@smitpatel could you please take a stab at writing a justification/risk assessment for taking the fix in 1.1.1? A comment here in the bug works for me. Thanks.

@smitpatel
Copy link
Member

Justification: This is regression from 1.0. Due to changes in query pipeline we started binding with outer query even though it was a separate SelectExpression overzealously without verifying if it would generate valid scenario. The fix blocks additional binding which could generate invalid queries.

Risk: This is low risk. It makes queries to evaluate certain parts on client side instead of translating to server. And in most of those cases, the server side translation was producing invalid queries.

@Eilon
Copy link
Member

Eilon commented Feb 8, 2017

This patch bug is approved. Please use the normal code review process w/ a PR and make sure the fix is in the correct branch, then close the bug and mark it as done.

@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 Feb 9, 2017
@smitpatel
Copy link
Member

merged to 1.1.1 in b8fb258
in dev a457f2e

@ctolkien
Copy link
Author

ctolkien commented Feb 9, 2017

Thanks @smitpatel 👍

maumar added a commit that referenced this issue Mar 15, 2017
…subquery that returns a single element

Problem was that in the fix for #7033 we force client evaluation on queries that try to bind to parents, when the parent has a client projection.
However, for the case when single element is returned (e.g. cusotmers.Select(c => new DTO { Count = c.Orders.Count() })) we don't need to do client evaluation.
We actually try to merge the subquery into the parent query, but this causes a problem because earlier in the pipeline we assumed that client eval is needed (since parent QM has client projection)

Fix is to be consistent and never try to lift subquery if the parent QM has a client projection. This causes more scenarios to produce N+1 queries, but the proper fix would require a breaking change:
we need additional information passed to the SqlTranslatingExpressionVisitor to know that the subquery that is being visited returns a single value.
maumar added a commit that referenced this issue Mar 25, 2017
…ntaining subquery that returns a single element

Problem was that in the fix for #7033 we force client evaluation on queries that try to bind to parents, when the parent has a client projection.
However, for the case when single element is returned (e.g. cusotmers.Select(c => new DTO { Count = c.Orders.Count() })) we don't need to do client evaluation.
We actually try to merge the subquery into the parent query, but this causes a problem because earlier in the pipeline we assumed that client eval is needed (since parent QM has client projection)

Fix is to flow information about query model's StreamedDataInfo into SqlTranslatingExpressionVisitor so that we only client-eval when it's really necessary (i.e. when more than one element is returned from the subquery).
Information is stored in (proper) internal fields as we don't really want to expose this API in any way.
maumar added a commit that referenced this issue Mar 27, 2017
…ntaining subquery that returns a single element

Problem was that in the fix for #7033 we force client evaluation on queries that try to bind to parents, when the parent has a client projection.
However, for the case when single element is returned (e.g. cusotmers.Select(c => new DTO { Count = c.Orders.Count() })) we don't need to do client evaluation.
We actually try to merge the subquery into the parent query, but this causes a problem because earlier in the pipeline we assumed that client eval is needed (since parent QM has client projection)

Fix is to flow information about query model's StreamedDataInfo into SqlTranslatingExpressionVisitor so that we only client-eval when it's really necessary (i.e. when more than one element is returned from the subquery).
Information is stored in (proper) internal fields as we don't really want to expose this API in any way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

4 participants