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: Filter out entities that are using table splitting with a derived type #8973

Closed
AndriySvyryd opened this issue Jun 26, 2017 · 10 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.0 type-enhancement
Milestone

Comments

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Jun 26, 2017

Entities that are splitting the table with a derived type need to be queried with a filter on the principal type discriminator.

See TableSplittingTestBase.Can_query_shared_derived() and OwnedQuerySqlServerTest

@AndriySvyryd AndriySvyryd changed the title Query: Filter out entities that are using using table splitting with a derived type Query: Filter out entities that are using table splitting with a derived type Jun 26, 2017
@anpete
Copy link
Contributor

anpete commented Jun 26, 2017

@AndriySvyryd What does this mean?

@AndriySvyryd
Copy link
Member Author

@anpete It will make more sense when #8976 is checked in

@anpete
Copy link
Contributor

anpete commented Jun 29, 2017

I think we should consider reworking discriminators to be in-terms of QueryModel rewrites (like entity filters).

@ajcvickers ajcvickers assigned smitpatel and unassigned anpete Jun 30, 2017
@ajcvickers
Copy link
Contributor

@anpete to write a note on details.
@smitpatel We should either fix this or throw an exception in, for example, model validation so we can fix it post 2.0 without it being a breaking change. Please don't take a long time on a complex fix--if the fix isn't simple, then let's go the throw route.

@anpete
Copy link
Contributor

anpete commented Jun 30, 2017

@smitpatel The fix to try is to modify the two places in relational that apply discriminator predicates to SelectExpression: MaterializerFactory.CreateMaterializer and RelationalEntityQueryableExpressionVisitor.DiscriminateProjectionQuery. What we need to do is expand the cases for when we introduce discriminators by taking into account any related principal ETs that are sharing the same table. E.g. For model:

    A
  /
B --> C

Where B inherits A ,and B references C, and A, B, C all share the same table, then any direct query for Cs necessarily needs to have a B's discriminator predicate applied. Note, no join should be required because we are always table splitting.

@AndriySvyryd has some context on this, too.

@smitpatel
Copy link
Contributor

@anpete - I prefer the idea of converting discriminators to where predicates at query model level. At present, we are in the world where creating SelectExpression for EntityQueryable can give rise to "where" clause in Select. That causes issues with joins where a flat QM can also generated a complex Select which requires to be pushdown.
https://github.com/aspnet/EntityFramework/blob/f034d5d18e57667e6b9af2424f8e6676340bae77/src/EFCore.Relational/Query/RelationalQueryModelVisitor.cs#L1537
Above code special case it for Left join cases.
And we still have bug for #8375

Both above manifest from the fact that a random where predicate can arise for TPH scenario. With table splitting like above, that case becomes even more expanded since now, a shared type of a derived type will start having where.

It would be much more easier for our translation pipeline to work, if QM is modified instead of working around while translating. Though that will be decent amount of work with not enough time and then throw becomes the solution for 2.0.

If we want to support this for 2.0, then we can go with suggestion above and make additional check for owners.

Your call what we should do for 2.0 ⌚️

@AndriySvyryd
Copy link
Member Author

I'll add a validation rule for 2.0 so we don't waste time on a temporary solution.

@anpete
Copy link
Contributor

anpete commented Jun 30, 2017

Sounds good to me.

@ajcvickers
Copy link
Contributor

Thanks for investigating. Validation rule seems appropriate for 2.0.

@ajcvickers ajcvickers added this to the 2.0.0 milestone Jul 3, 2017
AndriySvyryd added a commit that referenced this issue Jul 5, 2017
…ned types and table splitting with a derived type.

Part of #8973
Fixes #8871
AndriySvyryd added a commit that referenced this issue Jul 5, 2017
…ned types and table splitting with a derived type.

Part of #8973
Fixes #8871
AndriySvyryd added a commit that referenced this issue Jul 7, 2017
…ned types and table splitting with a derived type.

Part of #8973
Fixes #8871
AndriySvyryd added a commit that referenced this issue Jul 7, 2017
…ned types and table splitting with a derived type.

Part of #8973
Part of #8871
@AndriySvyryd AndriySvyryd removed their assignment Jul 7, 2017
@AndriySvyryd AndriySvyryd modified the milestones: Backlog, 2.0.0 Jul 7, 2017
@ajcvickers ajcvickers added this to the 2.1.0 milestone Sep 7, 2017
@ajcvickers
Copy link
Contributor

@smitpatel This is a decent amount of work, so marking as priority high for 2.1 to indicate that it should be done soon.

smitpatel added a commit that referenced this issue Feb 14, 2018
DRY code to generate discriminator for a query for materialize & non-materialized case
Find identifying relationship chain to root and add discriminator of relavent types

Resolves #8973
smitpatel added a commit that referenced this issue Feb 15, 2018
DRY code to generate discriminator for a query for materialize & non-materialized case
Find identifying relationship chain to root and add discriminator of relavent types

Resolves #8973
@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 15, 2018
smitpatel added a commit that referenced this issue Feb 16, 2018
DRY code to generate discriminator for a query for materialize & non-materialized case
Find identifying relationship chain to root and add discriminator of relavent types

Resolves #8973
smitpatel added a commit that referenced this issue Feb 16, 2018
DRY code to generate discriminator for a query for materialize & non-materialized case
Find identifying relationship chain to root and add discriminator of relavent types

Resolves #8973
@ajcvickers ajcvickers modified the milestones: 2.1.0-preview2, 2.1.0 Nov 11, 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.0 type-enhancement
Projects
None yet
Development

No branches or pull requests

4 participants