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

Complete Discriminator mapping #18106

Closed
fabianus76 opened this issue Sep 27, 2019 · 6 comments · Fixed by #20193
Closed

Complete Discriminator mapping #18106

fabianus76 opened this issue Sep 27, 2019 · 6 comments · Fixed by #20193
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 type-enhancement
Milestone

Comments

@fabianus76
Copy link

I discovered that (at least for queries to mysql) a where clause on the discriminator is formed, even if all derived classes are selected. This slows down remarkably even if you add an index on the discriminator field when millions of rows are in the table.

There are no other derived classes than union, person and organisation, but still ef core forms this where clause:
WHERE o.Discriminator IN ('Union', 'Person', 'Organisation')

Just skip the where Discriminator in ('class1','class2',...) when all classes are selected.

@smitpatel
Copy link
Member

This is by design. We add predicate to make sure that we only select records which correspond to one of the types we know. Your table may have additional values in discriminator column which we don't know about and it can throw runtime exception. It is similar to having non-mapped columns and EF Core selecting only columns we know explicitly.

@AndriySvyryd
Copy link
Member

We could allow to mark the discriminator as 'complete' and make this perf improvement.

@smitpatel smitpatel added this to the Backlog milestone Sep 30, 2019
@AndriySvyryd AndriySvyryd changed the title TPH Discriminator where clause for nothing Complete Discriminator mapping Oct 24, 2019
@AndriySvyryd
Copy link
Member

Related to #2594

@DibyodyutiMondal
Copy link

DibyodyutiMondal commented Feb 6, 2020

@ajcvickers
continuing from #19803 (comment)

A query can use only properties defined in the base class, but still return instances of multiple different types in the hierarchy. So, "involves properties of the base class only" does not mean the same as "involves any type in the hierarchy".

You're right, I did mean "involves any/every type in the hierarchy".

But just to be sure I got it right...

Let's assume a program involving Students and Teachers
I have a User class and Student and Teacher classes derive from it, discriminated by an enum property called Type which lives inside the User class. There are 3 corresponding DbSets.

Now, there can be 2 scenarios depending on the use-case
A. There are only Students and Teachers
B. There are Students, there are Teachers, and there are other "normal" Users who have no extra properties.

In both scenarios, the enum will have to have 3 values, or EF core will not allow us to it as a discriminator.

In scenario A, only 2 of those 3 enum values will actually be present in the data
In scenario B, all 3 enum values will be present in the data

In scenario A, when using dbcontext.Users... the developer wants to query any/every User. Being a Student/Teacher is irrelevant.

In scenario B, ambiguity comes in. When using dbcontext.Users... the developer may want either of 2 things

  1. the developer wants to query any/every user (same as scenario A)
  2. the developer wants to query only the "normal" Users and ignore the Students and Teachers.

What I am looking for is better performance in Scenario A by eliminating the extra where clause.
I imagine global query filters can be used for scenario B.2, but even there, I think EF Core would attach an extra where clause, although I have never tried it, so that's just an assumption.

Personally, I never had experience with a use case like Scenario B, even if I did, I'd probably avoid it by using a 4th enum value for "normal" Users, and thus convert it to Scenario A. Which is why I like the idea put forward in #18106 (comment), because that would allow the developer to chose what's best for them.

@ajcvickers ajcvickers removed this from the Backlog milestone Feb 8, 2020
@ajcvickers
Copy link
Member

Clearing the milestone here to discuss with the team. Making this the default would fix the simple raw SQL issue for composing over TPH tables: #18232

@ajcvickers ajcvickers added this to the 5.0.0 milestone Feb 10, 2020
@smitpatel
Copy link
Member

Filed #20192 for default decision.

@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 Mar 5, 2020
smitpatel added a commit that referenced this issue Mar 5, 2020
- Add fluent API IsComplete over DiscriminatorBuilder
- In query if mapping is complete
  - Don't generate predicate when creating Select for an entityType
  - Don't apply predicate when doing OfType of derived type when the discriminator is not needed

Current default: Mapping is incomplete unless user uses fluent API to mark it as complete.

Resolves #18106
smitpatel added a commit that referenced this issue Mar 5, 2020
- Add fluent API IsComplete over DiscriminatorBuilder
- In query if mapping is complete
  - Don't generate predicate when creating Select for an entityType
  - Don't apply predicate when doing OfType of derived type when the discriminator is not needed

Current default: Mapping is incomplete unless user uses fluent API to mark it as complete.

Resolves #18106
smitpatel added a commit that referenced this issue Mar 5, 2020
- Add fluent API IsComplete over DiscriminatorBuilder
- In query if mapping is complete
  - Don't generate predicate when creating Select for an entityType
  - Don't apply predicate when doing OfType of derived type when the discriminator is not needed

Current default: Mapping is incomplete unless user uses fluent API to mark it as complete.

Resolves #18106
smitpatel added a commit that referenced this issue Mar 6, 2020
- Add fluent API IsComplete over DiscriminatorBuilder
- In query if mapping is complete
  - Don't generate predicate when creating Select for an entityType
  - Don't apply predicate when doing OfType of derived type when the discriminator is not needed

Current default: Mapping is incomplete unless user uses fluent API to mark it as complete.

Resolves #18106
smitpatel added a commit that referenced this issue Mar 6, 2020
- Add fluent API IsComplete over DiscriminatorBuilder
- In query if mapping is complete
  - Don't generate predicate when creating Select for an entityType
  - Don't apply predicate when doing OfType of derived type when the discriminator is not needed

Current default: Mapping is incomplete unless user uses fluent API to mark it as complete.

Resolves #18106
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview2 Mar 13, 2020
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 type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants