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

[5.0.1] InMemory: Copy correct value buffer data when doing LeftJoin #23292

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

smitpatel
Copy link
Member

@smitpatel smitpatel commented Nov 12, 2020

Resolves #23285

Description

This is a query regression with the in-memory database when an entity type both references an owned entity and is in a hierarchy.

Customer Impact

Unable to make any queries using models which contains both a hierarchy and acts as a owner for some owned entity. Note that this is only for the in-memory database, which means it is unlikely to impact production code. However, it is very likely to impact tests for production code since it is a common scenario which is likely to be tested by many using the in-memory database.

How found

Customer reported on 5.0.

Test coverage

Added test for affected scenario. Also MQ work is greatly increasing our coverage of owned types.

Regression?

Yes, this worked in 3.1.

Risk

Low. This aligns the processing same as what happens in relational where there are no reported bugs in similar code path.


Long technical description from Smit:

Now my long paragraph to future contributors to this file
This issue reverts the fix from #22202 and fixes it differently.
The main issue is that when we generate a LeftJoin through GJ-DIE-SM pattern we need to generate result selector for SM scenario.
In this result selector, either we copy VB from source as is and the preserve the inner projection mapping with remapped parameters or we apply projection through the result selector and regenerate bindings for outer.
What happened in #22202 that when generating result selector and new bindings, we changed indexes of VB which broke owned navigation mappings. So we fixed that by preserving indexes from entity projections in WeakNavigation expansion scenario.
This caused side-effect that now we are applying selector for non-entity projection but preserving VB slots for entity projections. Further we used the read expression to preserve value buffer as is meaning multiple different properties which are in same slot were not preserved correctly causing InvalidCastException while reading.
A better approach is to generate result selector accounting for the owned navigations too. Essentially add owned navigation projections to result selector too. This has to happen in both case, when adding LeftJoin in non-owned scenario and in owned scenario.
Overall it makes all indexes happy.
Thus, harmony in the universe was restored.

Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

@maumar should also take a look

@ajcvickers
Copy link
Member

@smitpatel

When entity type has an owned entity and hierarchy, query fails.

Would I be correct in saying that this is common?

Low.

This doesn't align with the complexity of the description. :trollface: I may have to invite you to Tactics to explain this. one. 😄

@AndriySvyryd
Copy link
Member

Low. This aligns the processing same as what happens in relational where there are no reported bugs in similar code path.

I would say it's Low because this is in-memory and shouldn't be used in production.

@hgGeorg
Copy link

hgGeorg commented Nov 13, 2020

Low. This aligns the processing same as what happens in relational where there are no reported bugs in similar code path.

I would say it's Low because this is in-memory and shouldn't be used in production.

While this assessment is correct, this bug does break CI/CD pipelines that use InMemory provider. I'd gladly use SQLite in memory for my unit tests, but it does have limitations. Any (sane) advice besides 'Wait for fix'?

@ajcvickers
Copy link
Member

Approved by Tactics for 5.0.1.

Resolves #23285

Now my long paragraph to future contributors to this file
This issue reverts the fix from #22202 and fixes it differently.
The main issue is that when we generate a LeftJoin through GJ-DIE-SM pattern we need to generate result selector for SM scenario.
In this result selector, either we copy VB from source as is and the preserve the inner projection mapping with remapped parameters or we apply projection through the result selector and regenerate bindings for outer.
What happened in #22202 that when generating result selector and new bindings, we changed indexes of VB which broke owned navigation mappings. So we fixed that by preserving indexes from entity projections in WeakNavigation expansion scenario.
This caused side-effect that now we are applying selector for non-entity projection but preserving VB slots for entity projections. Further we used the read expression to preserve value buffer as is meaning multiple different properties which are in same slot were not preserved correctly causing InvalidCastException while reading.
A better approach is to generate result selector accounting for the owned navigations too. Essentially add owned navigation projections to result selector too. This has to happen in both case, when adding LeftJoin in non-owned scenario and in owned scenario.
Overall it makes all indexes happy.
Thus, harmony in the universe was restored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants