dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.78k stars 3.19k forks source link

Skip/Take after client evaluated predicate is not translated to Sql #8506

Closed ghost closed 2 years ago

ghost commented 7 years ago

Hi all,

I have been fighting against the Skip/Take functions for the last couple of days. I'm using a very simple code in charge of filtering/returning entities matching a set of criteria. Everything is working fine except the Take/Skip part.

        [HttpGet]
        public async Task<List<GetBookingResponse>> GetBookings(GetBookingRequest request)
        {
            var query = Context.Bookings
                .Include(x => x.Resource)
                .AsQueryable();

            if (!string.IsNullOrWhiteSpace(request.TecTacLocationId))
                query = query.Where(x => x.ResourceId.HasValue && x.Resource.TecTacLocationId == request.TecTacLocationId);

            if (request.Start.HasValue && request.End.HasValue)
                query = query.Where(x => x.Start >= request.Start && x.End <= request.End);
            else if (request.Start.HasValue && !request.End.HasValue)
                query = query.Where(x => x.Start >= request.Start);
            else if (!request.Start.HasValue && request.End.HasValue)
                query = query.Where(x => x.End <= request.End);

            var data = await query
                .Skip(request.Offset)   // 0
                .Take(request.Limit)    // 15
                .AsNoTracking() // tried with this commented out.
                .ToListAsync();

            return Mapper.Map<List<Core.Entities.Booking>, List<GetBookingResponse>>(data);
        }

This should generate a SQL query with a top (15) statement. Instead, i'm getting the following query:

SELECT [x].[Booking_Id], [x].[ActualPrice], [x].[CanEdit], [x].[EndDate], [x].[TimeEnd], [x].[IsVcBooking], [x].[ListedPrice], [x].[Memo], [x].[Resource_Id], [x].[Date], [x].[TimeStart], [x].[BookingStatus], [x].[TecTacClientId], [x].[TecTacContactId], [x].[BookingType], [x].[Version], [x.Resource].[ResourceId], [x.Resource].[ResourceCode], [x.Resource].[ResourceDisplayName], [x.Resource].[TecTacLocationId], [b].[ResourceId], [b].[ResourceCode], [b].[ResourceDisplayName], [b].[TecTacLocationId]
FROM [Booking] AS [x]
LEFT JOIN [BookingResource] AS [x.Resource] ON [x].[Resource_Id] = [x.Resource].[ResourceId]
LEFT JOIN [BookingResource] AS [b] ON [x].[Resource_Id] = [b].[ResourceId]
WHERE [x].[Resource_Id] IS NOT NULL AND (([x.Resource].[TecTacLocationId] = @__request_TecTacLocationId_0) AND [x.Resource].[TecTacLocationId] IS NOT NULL)
ORDER BY [x].[Resource_Id]

Further technical details

EF Core version: 1.1.2 Database Provider: Microsoft.EntityFrameworkCore.SqlServer Operating system: Visual Studio 2017

Thank you, Sebastien

smitpatel commented 7 years ago

Does this give correct results?

ghost commented 7 years ago

I didn't check because I have approx 841,000 bookings in my table. It takes a massive amount of times to return the record set... I have no other choice but to do the paging at the SQL Server level

smitpatel commented 7 years ago

There are few shortcomings or EF you are hitting in this case.

6206 - Using include & navigation filter on same navigation causes multiple joins. Hence you get Left Join with same table.

6647 - Left join always pulled data from both the tables.

With 2 left join like above, I believe top(15) on the generated SQL would produce incorrect results (not enough rows from first table will be selected) due to 2 left joins. Perhaps that is the reason we did not translate it to server.

Both of the above issues are fixed in 2.0.0 so it should generate much optimized query. @maumar - Can you verify generated SQL in this kind of query?

As for work-around in 1.1.2 version, @sroche-tec can you try writing query without include part and see if it translate Take to server. If it does then you can include the related data with explicit loading or a separate query to achieve result you want. In that case, you would be doing 2 queries but not pulling so much of data.

ghost commented 7 years ago

Hey, thanks for your answers. I have tried to comment out the Include part.

            var query = Context.Bookings
                //.Include(x => x.Resource)
                .AsQueryable();

            if (!string.IsNullOrWhiteSpace(request.TecTacLocationId))
                query = query.Where(x => x.ResourceId.HasValue && x.Resource.TecTacLocationId == request.TecTacLocationId);

            if (request.Start.HasValue && request.End.HasValue)
                query = query.Where(x => x.Start >= request.Start && x.End <= request.End);
            else if (request.Start.HasValue && !request.End.HasValue)
                query = query.Where(x => x.Start >= request.Start);
            else if (!request.Start.HasValue && request.End.HasValue)
                query = query.Where(x => x.End <= request.End);

            var data = await query
                .Skip(0)
                .Take(15) 
                .ToListAsync();

            return Mapper.Map<List<Core.Entities.Booking>, List<GetBookingResponse>>(data);

AS a result, EF core generates the following query:

SELECT [x].[Booking_Id], [x].[ActualPrice], [x].[CanEdit], [x].[EndDate], [x].[TimeEnd], [x].[IsVcBooking], [x].[ListedPrice], [x].[Memo], [x].[Resource_Id], [x].[Date], [x].[TimeStart], [x].[BookingStatus], [x].[TecTacClientId], [x].[TecTacContactId], [x].[BookingType], [x].[Version], [x.Resource].[ResourceId], [x.Resource].[ResourceCode], [x.Resource].[ResourceDisplayName], [x.Resource].[TecTacLocationId]
FROM [Booking] AS [x]
LEFT JOIN [BookingResource] AS [x.Resource] ON [x].[Resource_Id] = [x.Resource].[ResourceId]
WHERE [x].[Resource_Id] IS NOT NULL AND (([x.Resource].[TecTacLocationId] = @__request_TecTacLocationId_0) AND [x.Resource].[TecTacLocationId] IS NOT NULL)
ORDER BY [x].[Resource_Id]

Any idea? Shall I update to 2.0.0 preview (any breaking changes coming)?

smitpatel commented 7 years ago

as 2.0 is major version change, there are some breaking changes for certain. You can upgrade but you may need to update code also for breaking changes.

Thanks for the generated query. Actually lets try the other way around. Keep include as is. (I am hoping that will translate the take correctly) but remove the where predicate on x.Resource.TecTacLocationId. Apply that predicate after ToListAsync part. Once you have included navigation and loaded data in memory, you can still apply the where predicate in memory without any more database query.

ghost commented 7 years ago

Interesting! I found one possible root cause. I removed all where clauses and suddenly it started to work. Actually x.Start and x.End are not database fields. They are calculated properties:

public DateTime? Start => StartDate?.Add(StartTime ?? TimeSpan.Zero);
public DateTime? End => EndDate?.Add(EndTime ?? TimeSpan.Zero);

Removing those filters solve the problems. In the end, it works with the include and even with the where pred on x.Resource.TecTacLocationId.

Thx for your help!

Thx Seb

smitpatel commented 7 years ago

So it was client evaluation of where predicate. I did not think of it, my mistake. Its good all worked out in the end. 😄

nelsonrc commented 5 years ago

I am using version 2.1 of EF and I present a similar problem with a table of 25000 records, when using Skip / Take and Include beyond the record 17000 does not return records, but if I delete the Include clauses everything works more or less well.