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

Query: Distinct on non-materialized QSRE could give incorrect result #10433

Closed
smitpatel opened this issue Nov 29, 2017 · 3 comments
Closed

Query: Distinct on non-materialized QSRE could give incorrect result #10433

smitpatel opened this issue Nov 29, 2017 · 3 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-2.1 type-bug
Milestone

Comments

@smitpatel
Copy link
Contributor

https://github.com/aspnet/EntityFrameworkCore/blob/dev/src/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs#L2726-L2738

Generates

SELECT [t].[HasSoulPatch]
FROM (
    SELECT DISTINCT [ig].*
    FROM [Gears] AS [ig]
    WHERE [ig].[Discriminator] IN (N'Officer', N'Gear')
) AS [t]

This could generate incorrect results if Gears table has some non-mapped columns. So even though expectation is distinct records there could be duplicates.
We should probably expand * projection to full list of mapped properties here to get closer to linq behavior.

@anpete @divega @ajcvickers - thoughts?

@divega
Copy link
Contributor

divega commented Nov 30, 2017

@smitpatel explained that he was removing the star projection from cases like this, but it became apparent that for DISTINCT we should have the full list of columns (as opposed to just what is being projected in the outer projection).

Personally, I would not expect non-mapped columns to have an influence in the results.

There is however an optimization when the root is a scan over an entity type (with optional filters, paging and sorting): just eliminate the whole DISTINCT because we know that we will be getting a unique PK value per row (see #8643).

@ajcvickers
Copy link
Member

Assigning to @anpete as part of #9290.

@ajcvickers ajcvickers added this to the 2.1.0 milestone Dec 4, 2017
@ajcvickers ajcvickers modified the milestones: 2.1.0-preview1, 2.1.0 Jan 17, 2018
@divega divega modified the milestones: 2.1.0-preview2, 2.1.0 Apr 2, 2018
@ajcvickers ajcvickers modified the milestones: 2.1.0, 2.2.0, Backlog Apr 10, 2018
@AndriySvyryd AndriySvyryd added the verify-fixed This issue is likely fixed in new query pipeline. label Aug 22, 2019
@smitpatel smitpatel self-assigned this Sep 4, 2019
@ajcvickers ajcvickers modified the milestones: Backlog, 3.1.0 Sep 4, 2019
@smitpatel
Copy link
Contributor Author

Generated SQL in new pipeline

SELECT [t].[HasSoulPatch]
FROM (
    SELECT DISTINCT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOrBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank]
    FROM [Gears] AS [g]
    WHERE [g].[Discriminator] IN (N'Gear', N'Officer')
) AS [t]

@smitpatel smitpatel added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed verify-fixed This issue is likely fixed in new query pipeline. labels Sep 9, 2019
@smitpatel smitpatel modified the milestones: 3.1.0, 3.0.0 Sep 9, 2019
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. punted-for-2.1 type-bug
Projects
None yet
Development

No branches or pull requests

5 participants