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

PoC for EF Core 5 (and may be 3.1) support #2179

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

kosinsky
Copy link
Contributor

@kosinsky kosinsky commented May 28, 2020

Trying to solve $apply for EF Core 5 (and 3.1 if possible).

I've updated EF Core to 5 Preview 4 in .NET 3 E2E test it caused. After that 2 x (26 of 34) started to fail

Most of that was caused by casting to IEnumerable in expressions like

.GroupBy($it => True)
.Select($it => new NoGroupByAggregationWrapper() {
    Container = new LastInChain() {
        Name = "Count", 
        Value = Convert(Convert($it, IEnumerable`1).LongCount(), Object)}})}

Exception:

System.InvalidOperationException: Processing of the LINQ expression 'GroupByShaperExpression:
KeySelector: CAST(1 AS bit), 
ElementSelector:EntityShaperExpression: 
    EntityType: Customer
    ValueBufferExpression: 
        ProjectionBindingExpression: EmptyProjectionMember
    IsNullable: False
' by 'RelationalProjectionBindingExpressionVisitor' failed. This may indicate either a bug or a limitation in EF Core. See https://go.microsoft.com/fwlink/?linkid=2101433 for more detailed information.
   at Microsoft.EntityFrameworkCore.Query.Internal.RelationalProjectionBindingExpressionVisitor.VisitExtension(Expression extensionExpression)
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)

I think that was attempt to use client eval.

I modified optimization made for EF 2.2 (47756e0) and changed expression to

.GroupBy($it => True)
.Select($it => new NoGroupByAggregationWrapper() {
    Container = new LastInChain() {
     Name = "Count", 
     Value = Convert($it.AsQueryable().LongCount(), Object)}})}

After that change only 9 tests are failing.
All failing tests are using navigation properties in following way:
$apply=aggregate(Order/Price with sum as Result) that translates to following Linq expression:


.GroupBy($it => True)
  .Select($it => new NoGroupByAggregationWrapper() {
   Container = new LastInChain() {
        Name = "Result", 
        Value = Convert($it.AsQueryable().Average($it => $it.Order.Price), Object)}})}

that causes following error:

System.InvalidOperationException: Processing of the LINQ expression 'GroupByShaperExpression:
KeySelector: ExpressionExtensions.ValueBufferTryReadValue<bool>(
    valueBuffer: grouping.Key, 
    index: 0, 
    property: null), 
ElementSelector:EntityShaperExpression: 
    EntityType: Customer
    ValueBufferExpression: 
        ProjectionBindingExpression: EmptyProjectionMember
    IsNullable: False
' by 'InMemoryProjectionBindingExpressionVisitor' failed. This may indicate either a bug or a limitation in EF Core. See https://go.microsoft.com/fwlink/?linkid=2101433 for more detailed information.
   at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryProjectionBindingExpressionVisitor.VisitExtension(Expression extensionExpression)
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)

Looks like EF has a problem to deal with $it => $it.Order.Price

cc @xuzhg @smitpatel

@smitpatel
Copy link

Looks like EF has a problem to deal with $it => $it.Order.Price

Yes, that is true. EF Core at present only expands navigations upto KeySelector or whatever is in GroupBy call. Any navigation used further is not expanded yet. (difficulty is that navigation needs to be expanded before applying GroupBy even in expression tree). Work-around would be to expand the navigation before calling GroupBy manually.

@kosinsky
Copy link
Contributor Author

Work-around would be to expand the navigation before calling GroupBy manually.

That helped. We already had flattening to solve some EF6 issues. I extended it for that case too

I replaced

.GroupBy($it => new NoGroupByWrapper())
.Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {
    Name = "Result",
    Value = Convert($it.AsQueryable().Average($it => $it.Order.Price), Object)}})}

by

.Select($it => new FlatteningWrapper`1() {Source = $it, GroupByContainer = new LastInChain() {Name = "Property0", Value = Convert($it.Order.Price, Object)}})
.GroupBy($it => new NoGroupByWrapper())
.Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {
    Name = "Result",
    Value = Convert($it.AsQueryable().Average($it => Convert($it.GroupByContainer.Value, Int32)), Object)}})}

It's solved almost all test.

I'm seeing an issue with implementing count distinct. Linq looks like:

.Select($it => new FlatteningWrapper`1() {Source = $it, GroupByContainer = new LastInChain() {Name = "Property0", Value = Convert($it.Order.Price, Object)}})
.GroupBy($it => new NoGroupByWrapper())
.Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {
    Name = "Result",
    Value = Convert($it.AsQueryable().Select($it => Convert($it.GroupByContainer.Value, Int32)).Distinct().LongCount(), Object)}})}   

Exception is:

System.InvalidOperationException: The LINQ expression 'GroupByShaperExpression:
KeySelector: new NoGroupByWrapper(), 
ElementSelector:new FlatteningWrapper<Customer>{ 
    Source = EntityShaperExpression: 
        EntityType: Customer
        ValueBufferExpression: 
            ProjectionBindingExpression: Source
        IsNullable: False
    , 
    GroupByContainer = new LastInChain{ 
        Name = ProjectionBindingExpression: GroupByContainer.Name, 
        Value = ProjectionBindingExpression: GroupByContainer.Value 
    }
     
}

    .Select($it => (int)$it.GroupByContainer.Value)' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to either AsEnumerable(), AsAsyncEnumerable(), ToList(), or ToListAsync(). See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

Is it something that I could do to solve that.

@smitpatel
Copy link

@kosinsky - That would be being tracked by dotnet/efcore#17376
We have planned it for EF Core 5.0. Sadly there is no way to work-around it at present.

@kosinsky
Copy link
Contributor Author

kosinsky commented Jun 1, 2020

That helped and I managed to make all tests green.

However, I noticed that we had no test coverage EF Core for aggregation on nested collections.

We are generating something like:

{[Microsoft.EntityFrameworkCore.Query.QueryRootExpression]
.GroupBy($it => new NoGroupByWrapper()
).Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = "Orders", 
    Value = $it.AsQueryable()
        .SelectMany($it => $it.Orders)
        .GroupBy($gr => new Object())
        .Select($p => new EntitySetAggregationWrapper() {Container = new LastInChain() {Name = "TotalPrice", Value = Convert($p.AsQueryable().Sum($it => $it.Price), Object)}})}})}

and getting exception:

System.InvalidOperationException: The LINQ expression 'GroupByShaperExpression:
KeySelector: new NoGroupByWrapper(), 
ElementSelector:EntityShaperExpression: 
    EntityType: Customer
    ValueBufferExpression: 
        ProjectionBindingExpression: EmptyProjectionMember
    IsNullable: False

    .SelectMany($it => $it.Orders)' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to either AsEnumerable(), AsAsyncEnumerable(), ToList(), or ToListAsync(). See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.
   at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryExpressionTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression 

Is SelectMany supported or will be supported in EF Core 5?

@smitpatel
Copy link

That is nested GroupBy. Even if inner GroupBy can be translated, outer GroupBy may not work since outer GroupBy results are still in the form of IGrouping (no aggregation applied). Regardless, SelectMany inside selector after GroupBy won't work due to same root case as expanded navigations does not work.

@d-a-s
Copy link

d-a-s commented Nov 5, 2021

Are there any plans to merge this? I've tested the old 7.4.1 nightly build that includes this, and it fixed the problem I'm having.

@kosinsky
Copy link
Contributor Author

kosinsky commented Nov 8, 2021

Are there any plans to merge this? I've tested the old 7.4.1 nightly build that includes this, and it fixed the problem I'm having.

Unfortunately, I'm not working on projects that involve OData and don't have spare cycles to continue that work. If you wold like to fork that PR and continue the work, please do.

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.

3 participants