search

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.77k stars 3.18k forks source link

Query: SkipLast and TakeLast #17065

Open smitpatel opened 5 years ago

smitpatel commented 4 years ago

Consider #14104

benmccallum commented 4 years ago

Hey @smitpatel / @ajcvickers, I'm keen to get this moving to help out @michaelstaib and I with cursor-based paging in GraphQL land. It will unlock

Here's an example of the SQL that could drive a SkipWhile based on @corstian's approach as mentioned in #14104. It may not be as performant as a SkipWhile supported natively on something like MongoDB, but it might be enough for most of us.

I'd originally started giving this a crack in this PR, but was told to wait until 3.1's new query translation.

Will start looking today, but any pointers in the right direction would be massively appreciated, this is a whole new world for me (both in terms of the repo and in terms of my limited experiance in expression trees and whatnot!). Cheers.

benmccallum commented 4 years ago

Pulled the repo and I feel like I need to have some kind of SqlServerQueryableMethodTranslatingExpressionVisitor implementation that would have a TranslateSkipWhile and TranslateTakeWhile (as this would be mssqlserver specific I'd imagine), but given that it's going to quite drastically "reshape" they query (build a CTE with WHERE [filters] and ORDER BY clause and then pull out then do the SELECT/FROM while using that CTE), I'm just not sure if this would be the right way to do it, or whether it'd be more of a post-processor kind of deal.

I think we might need to talk design on this.

I'm going to need some guidance or even some stubbed out classes/methods to progress.

smitpatel commented 4 years ago

@benmccallum - I looked that approach you mentioned above and briefly skimmed over #14104 (I will give it a longer read if that is necessary). I find there are some differences. The approach posted in first post of #14104 is a query which we can support right now in query tree so that translation can be supported. In the gist you shared above, can you explain why CTE is required? What is the purpose of 2 different SQLs? Translation of SkipWhile would be just one SQL to server side. CTEs are not supported currently in the SQL generated by EF.

benmccallum commented 4 years ago

Hi @smitpatel, thanks for taking a look and sorry I missed that. It's been a while since I wrote that, but I had a look at @corstian 's blog post on this, https://corstianboerman.com/2019-03-06/cursor-based-pagination-with-sql-server.html

I think I went for a CTE to simultaneously support hasNextPage, hasPreviousPage and totalCount; essentially I return the total count in the outer select and can derive the other two based on totalCount and the value of the offset for the first and last record (>1 or <totalCount).

Getting these becomes pretty painful otherwise. Open to suggestions!

smitpatel commented 4 years ago

My brief look at all this especially properties like hasNextPage/hasPreviousPage tells me that while cursor based pagination is nice to have, it is not translation of Skip/TakeWhile. That translation could be as simple as https://github.com/dotnet/efcore/issues/14104#issue-388356410 by integrating row number directly inside FETCH/OFFSET.

Are you trying to do the same or different? If it is not about Skip/TakeWhile, then we should better take the discussion on a separate issue.

corstian commented 4 years ago

As @smitpatel suggested, I think we're looking at two different but related issues here;

  1. The fist is implementation of the SkipWhile method in EF. This on itself would enable a pagination solution which would satisfy many use cases already.

  2. The second is fully featured cursor based pagination which includes information about previous and next items in the set, which while thinking about it would not fit in the function signature of the SkipWhile method either way.

benmccallum commented 4 years ago

@smitpatel, More than happy to just limit this discussion to @corstian's original suggestion.

Created a ticket (#20967) for the page info stuff. Would be good to have some quick suggestions to get that info off the back of just SkipWhile on that ticket so I can get fully back onto EF Core for this though. I guess it'd be a matter of doing two more queries with the start and end cursor to see if there's nodes in either direction. Not ideal, but probably worth it in my case to get back on EF Core.

roji commented 3 years ago

Looking at this issue, it seems like the above discussion is already tracked by #14104 (for a simple SkipWhile translation) and by #20967 (for a more elaborate pagination feature which does not correspond to just SkipWhile).

We may want to close this, and open separate issues for TakeWhile and SkipLast/TakeLast.

michaelstaib commented 3 years ago

@roji Can you link the separate issues for TakeWhile and SkipLast/TakeLast?

michaelstaib commented 3 years ago

If we can get SkipWhile and TakeWhile in it would solidify current paging approaches when using cursor pagination in GraphQL.

roji commented 3 years ago

@michaelstaib great - that's good to know (that was also the direction I was thinking of). But we'll have to discuss a bit planning-wise to see whether it can get in.

I haven't yet reorganized the issues here - I'll link when I do.

roji commented 2 years ago

As per our design discussion, am changing this issue to only track translationg of SkipLast and TakeLast; #14104 tracks translating SkipWhile and TakeWhile. Note that we don't believe SkipWhile is useful for pagination purposes, see https://github.com/dotnet/efcore/issues/14104#issuecomment-989675444.

michaelstaib commented 2 years ago

Yes, we will offer a new provider implementation for EF Core that uses keyset pagination as discussed. So from our view we do no longer need SkipWhile or TakeWhile. CC @pascalsenn