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

Aggregate on optional dependent lacks condition #23230

Closed
AndriySvyryd opened this issue Nov 7, 2020 · 6 comments · Fixed by #25949
Closed

Aggregate on optional dependent lacks condition #23230

AndriySvyryd opened this issue Nov 7, 2020 · 6 comments · Fixed by #25949
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@AndriySvyryd
Copy link
Member

See GraphUpdatesSqlServerTest.Owned.Required_one_to_one_are_cascade_deleted

context.Set<Root>().Select(r => r.RequiredSingle).Count(e => e.Id == removedId)
SELECT COUNT(*)
FROM [Root] AS [r]
WHERE [r].[Id] = @__removedId_0
@smitpatel
Copy link
Member

smitpatel commented Nov 7, 2020

Would the count be different though?
Is RequiredSingle optional? 😆

@AndriySvyryd
Copy link
Member Author

Yes, despite the name

@ajcvickers ajcvickers added this to the 6.0.0 milestone Nov 9, 2020
@AndriySvyryd
Copy link
Member Author

In this case left join isn't actually needed, but a condition on the nullable RequiredSingle_Bool column is.

This query works correctly:

context.Set<Root>().Any(r => r.RequiredSingle != null)
SELECT CASE
    WHEN EXISTS (
        SELECT 1
        FROM [Root] AS [r]
        WHERE [r].[RequiredSingle_Bool] IS NOT NULL) THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END

But these also lack the condition:

context.Set<Root>().Select(r => r.RequiredSingle).Select(e => e.Id == removedId)
SELECT CASE
    WHEN [r].[Id] = @__removedId_0 THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END
FROM [Root] AS [r]
context.Set<Root>().Select(r => r.RequiredSingle).AsNoTracking()
SELECT [r].[Id], [r].[RequiredSingle_Bool], [r].[RequiredSingle_Single_Bool]
FROM [Root] AS [r]

@AndriySvyryd AndriySvyryd changed the title Aggregate on optional dependent lacks left joins Aggregate on optional dependent lacks condition Nov 9, 2020
@smitpatel
Copy link
Member

This would redefine the concept of property access on owned entities. Basically, "e.Id" in the query cannot be accessed by just using the column from the database. And we need to add checks for principal (whole chain) to verify existence of the instance everytime any of the shared columns are used (except in final materialization). Do we want to go down that path?
Somewhat related, for TPH scenario while accessing derived type with casting which could be sharing column with a sibling, we decided to ignore it could give incorrect results and just use the column. #20335

@AndriySvyryd
Copy link
Member Author

AndriySvyryd commented Nov 9, 2020

Do we want to go down that path?

If we don't then we need to throw in this case as

context.Set<Root>().Select(r => r.RequiredSingle).Select(e => e.Id == removedId);

is different from

context.Set<Root>().Select(e => e.Id == removedId)

Somewhat related, for TPH scenario while accessing derived type with casting which could be sharing column with a sibling, we decided to ignore it could give incorrect results and just use the column. #20335

That's different because there's no other way of getting the shared behavior and non-shared behavior is available without casting.

@smitpatel
Copy link
Member

Design, we should generate conditional in SQL to access property conditionally.
We should also simplify case blocks if we are not doing already.
a ? b : null == c => a AND b == c

@smitpatel smitpatel removed the blocked label Aug 16, 2021
smitpatel added a commit that referenced this issue Sep 9, 2021
@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 Sep 9, 2021
smitpatel added a commit that referenced this issue Sep 10, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Sep 10, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc2, 6.0.0 Nov 8, 2021
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. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants