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

Reimplement entity equality #15920

Merged
merged 1 commit into from
Jun 6, 2019
Merged

Reimplement entity equality #15920

merged 1 commit into from
Jun 6, 2019

Conversation

roji
Copy link
Member

@roji roji commented Jun 3, 2019

New implementation of entity equality rewriter, which follows exact entity (and anonymous) from roots to the comparison expression, for more precise rewriting.

Note that unlike the first attempt in #15823, the visitors in this PR are completely stateless.

Closes #15588

Additional scoped-out items (excluding enhancements we probably won't do for 3.0):

@roji roji requested review from smitpatel and maumar June 3, 2019 21:04
@roji
Copy link
Member Author

roji commented Jun 3, 2019

(will check failing tests tomorrow, in the meantime a review would be great)

@roji
Copy link
Member Author

roji commented Jun 3, 2019

And one last comment before I'm off... Near the end of this refactor I realized that the EntityReferenceExpression nodes can probably be reduced during the first visit, obviating a second reduce-only pass. I'll look at that tomorrow.

@smitpatel
Copy link
Contributor

smitpatel commented Jun 3, 2019

Was gonna just that only. You don't need reducing visitor. Just top level method to unwrap the custom expression.

We probably need to do the same for nav expansion too.

@roji
Copy link
Member Author

roji commented Jun 3, 2019

@smitpatel yep :) It took me a while but I did get it in the end...

@maumar
Copy link
Contributor

maumar commented Jun 4, 2019

@roji you can also remove entity equality code from nav expansion (NavigationExpandingVisitor.VisitBinary + CreateNullComparisonArguments) to make sure the new logic covers all the scenarios

@roji
Copy link
Member Author

roji commented Jun 4, 2019

@maumar done, 0 test failures after removing that code :)

@roji roji force-pushed the EntityEquality branch from 1fdf452 to 7fb20b5 Compare June 4, 2019 22:26
@roji
Copy link
Member Author

roji commented Jun 4, 2019

OK guys, I think it's done.

The visitor now reduces nodes as it does its pass, no more 2nd reducing pass. There are also a ton more fixes and cleanups - it may need to be reviewed fully again. Sorry for all the back-and-forths on this, hopefully my future query PRs will be more quick and direct to the point.

@roji roji force-pushed the EntityEquality branch from 7fb20b5 to cb40004 Compare June 4, 2019 22:38
// If the source type is an IQueryable over the return type, this is a cardinality-reducing method (e.g. First).
// These don't flow the last navigation. In addition, these will be translated into a subquery, and we should not
// perform entity equality rewriting if the entity type has a composite key.
if (methodCallExpression.Method.ReturnType == sourceElementType)
Copy link
Contributor

@maumar maumar Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about: ctx.Customers.OfType<VipCustomer>().FirstOrDefault<Customer>(), i.e. when type argument of cardinality reducing operator is explicitly defined and different than one on source?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting - I've never seen someone call FirstOrDefault with an explicit type parameter, any specific value in doing so?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't think of anything reasonable where the result would be an entity but I'm working on a somewhat similar problem in different area (#15535), where the First method was typed as object. Might be worth adding test just in case. We have some inheritance in GearsOfWar model (Faction entity) that can be used for that purpose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am deferring this for now (made a note at the top post) so that I can merge this PR quickly. Will get around to it soon.

New implementation of entity equality rewriter, which follows exact
entity (and anonymous) from roots to the comparison expression, for
more precise rewriting.

Closes #15588
@roji roji force-pushed the EntityEquality branch from cb40004 to 6dbf690 Compare June 6, 2019 09:14
@roji roji merged commit 200a1ba into master Jun 6, 2019
@ghost ghost deleted the EntityEquality branch June 6, 2019 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query: Rewrite Entity Equality
4 participants