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

InMemory: Expansion weak entities in OrderBy/ThenBy should happen together #18912

Closed
smitpatel opened this issue Nov 14, 2019 · 3 comments
Closed
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. punted-for-6.0 punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-bug

Comments

@smitpatel
Copy link
Member

OrderBy_collection_count_ThenBy_reference_navigation

@RamunasAdamonis
Copy link

RamunasAdamonis commented Jan 6, 2021

I ran into this issue while migrating my project from .Net Core 2.2 to .Net 5.

Following expression was working with EF Core InMemory version 2.2.3, but does not work anymore in 5.0.1 (also checked 3.1.3 not working as well).

var products = await context.Products
    .OrderBy(x => x.ProductType)
    .ThenByDescending(x => x.Price.Amount)
    .ToListAsync();

*Price is Owned entity

Same error as in #22732

smitpatel added a commit that referenced this issue Mar 10, 2021
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replace all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934
Resolves #17620
Resolves #18912
@smitpatel smitpatel assigned smitpatel and unassigned maumar Sep 11, 2021
@smitpatel smitpatel removed this from the 6.0.0 milestone Sep 13, 2021
@smitpatel smitpatel added the priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release label Sep 13, 2021
@smitpatel
Copy link
Member Author

new regression test

        [ConditionalTheory]
        [MemberData(nameof(IsAsyncData))]
        public virtual Task Owned_navigations_are_expanded_for_OrderBy_ThenBy_together(bool async)
        {
            return AssertQuery(
                async,
                ss => ss.Set<OwnedPerson>()
                    .OrderBy(e => e.PersonAddress.PlaceType)
                    .ThenBy(e => e.PersonAddress.Country.Name),
                assertOrder: true);
        }

Old one works correctly now since it is weak type which uses STET.

@AndriySvyryd AndriySvyryd added this to the Backlog milestone Sep 14, 2021
@ajcvickers ajcvickers modified the milestones: Backlog, 7.0.0 Nov 10, 2021
@ajcvickers ajcvickers added propose-punt punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. and removed propose-punt labels Jul 6, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, Backlog Jul 7, 2022
@smitpatel smitpatel removed their assignment Sep 14, 2022
@ajcvickers ajcvickers added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. type-bug and removed area-in-memory priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release labels Oct 26, 2022
@ajcvickers
Copy link
Member

We recommend against using the in-memory provider for testing--see Testing EF Core Applications. While we have no plans to remove the in-memory provider, we will not be adding any new features to this provider because we believe valuable development time is better spent in other areas. When feasible, we plan to still fix regressions in existing behavior.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2022
@ajcvickers ajcvickers removed this from the Backlog milestone Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. punted-for-6.0 punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-bug
Projects
None yet
Development

No branches or pull requests

5 participants