Skip to content
This repository was archived by the owner on Oct 21, 2020. It is now read-only.

Conversation

matthewmueller
Copy link
Contributor

@matthewmueller matthewmueller commented May 14, 2020

This PR attempts to further simplify @sorenbs's proposal by supporting a negative take value. This is similar to how JS also supports arr.slice(-5)

Rendered View

Open Questions:

  • at: 5 instead of cursor: 5?

These are questions that are relevant to previous implementations and proposals, but I think it's important to list them out here.

  • take: 0 should mean take none or take at the cursor? and if take: 0 takes 1, then take: 3 would actually return 4 items?
  • what happens with? { cursor: 5, take: undefined }, is this take: 0 or take: 1 or not allowed?

@timsuchanek
Copy link
Contributor

That looks good!
For me personally cursor is more intuitive than at.
I would not allow take: 0 and not allow take: undefined.

@matthewmueller
Copy link
Contributor Author

Sounds good for Typescript – I guess for the others like Go, it can be a runtime error.

@Sytten
Copy link

Sytten commented May 27, 2020

Might I add that what we usually need in pagination is also the total number of records. Would be interesting to see if some optimization could be done about that.

@dpetrick dpetrick self-requested a review May 27, 2020 14:10
Copy link
Contributor

@dpetrick dpetrick left a comment

Choose a reason for hiding this comment

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

The spec is not really a spec, it leaves way too much room for interpretation and speculation:

  • Do I need to provide both cursor and take? If no, which combination is valid? What are the implications?
  • In which context is pagination supposed to be usable? findOne, findMany, delete, ... ?
  • What happens if the cursor doesn't exist? Do different operations have different behavior?
  • How is pagination influenced by other query arguments? When is it applied? Ordering plays a major role here.

@matthewmueller
Copy link
Contributor Author

matthewmueller commented May 27, 2020

Thanks for the feedback! I'll answer these questions here then persist it in the PR when everything is resolved.

Do I need to provide both cursor and take? If no, which combination is valid? What are the implications?

  • Both are optional
  • If cursor is present, but take is not (it's undefined), then we just take the item at the cursor.
  • take cannot appear without cursor

In which context is pagination supposed to be usable? findOne, findMany, delete, ... ?

All the contexts where we currently have:

after?: UserWhereUniqueInput | null
before?: UserWhereUniqueInput | null
first?: number | null
last?: number | null

This PR replaces these.

What happens if the cursor doesn't exist? Do different operations have different behavior?
How is pagination influenced by other query arguments? When is it applied? Ordering plays a major role here.

In general, same order/behavior as the previous implementation. We're simplifying the concept, not changing the concept.

If this doesn't answer the questions above for you, maybe we can talk about specific examples/issues?

@dpetrick
Copy link
Contributor

A broader point: While I knew most of the answers beforehand, the spec doesn't know any of these. I couldn't hand off the implementation to somebody without knowledge of the current QE behavior, which makes the spec in itself insufficient in my opinion.

Moving on to an actual question: I find it surprising that take can't occur without cursor, because that effectively means that I need 2 queries for something really simple like "give me the first 10 items on feed x" (I can imagine a lot of our users do something like that). I need to fetch ALL ITEMS of said model, because I can't know the starting point of the pagination, then take an ID, after which I can start paging. Unless I'm missing something, this sounds unwieldy and like a major downgrade to the previous API.

@matthewmueller
Copy link
Contributor Author

Moving on to an actual question: I find it surprising that take can't occur without cursor, because that effectively means that I need 2 queries for something really simple like "give me the first 10 items on feed x" (I can imagine a lot of our users do something like that). I need to fetch ALL ITEMS of said model, because I can't know the starting point of the pagination, then take an ID, after which I can start paging. Unless I'm missing something, this sounds unwieldy and like a major downgrade to the previous API.

Ah yes good catch. You're absolutely right! take without cursor would be the equivalent of first and last.

e.g.

Take last 10 users

prisma.users.findMany({
  take: -10
})

Take first 10 users

prisma.users.findMany({
  take: 10
})

@Sytten
Copy link

Sytten commented May 27, 2020

What happens to the case of having both the after and before specified? I guess this is a very specific use case that you know the start and end cursor but not the amount of items.

@matthewmueller
Copy link
Contributor Author

@Sytten that doesn't currently work does it? It's definitely an interesting use case, but I think it's pretty obscure and I'm don't think we'd be able to write efficient queries for it.

@Sytten
Copy link

Sytten commented May 27, 2020

I don't know, but I expect it would not work. thinking about the query, I think it can only work with numerical IDs since they are naturally ordered. So no, not a usecase for prisma IMO.

@dpetrick
Copy link
Contributor

Additional comment, motivated by @mavilein: Skip is also not really mentioned here. It's required for any kind of count-based pagination to actually work.

take: 5, skip: 0
take: 5, skip: 5
take: 5, skip: 10
take: 5, skip: 15
...

Is this a correct assumption that this should still be possible @matthewmueller?

@dpetrick dpetrick added this to the Beta 7 milestone May 28, 2020
@matthewmueller
Copy link
Contributor Author

matthewmueller commented May 28, 2020

Good call, yep. I think it would look like this. Please make sure it matches your expectations. Also thought about negative skips, but I think that could get confusing. I'll update the spec after.

             cursor: 5                              
             skip: 0 or undefined                   
                       │                            
                       │                            
                       │                            
                       ▼                            
 ┌───┐┌───┐┌───┐┏━━━┓┏━━━┓┌───┐┌───┐┌───┐┌───┐┌───┐ 
 │ 1 ││ 2 ││ 3 │┃ 4 ┃┃ 5 ┃│ 6 ││ 7 ││ 8 ││ 9 ││10 │ 
 └───┘└───┘└───┘┗━━━┛┗━━━┛└───┘└───┘└───┘└───┘└───┘ 
                ◀────────                           
                 take: -2                           
                                                    
                                                    
                                                    
                    cursor: 5                       
                     skip: 1                        
                        │                           
                        │                           
                        │                           
                        ▼                           
  ┌───┐┌───┐┏━━━┓┏━━━┓┌───┐┌───┐┌───┐┌───┐┌───┐┌───┐
  │ 1 ││ 2 │┃ 3 ┃┃ 4 ┃│ 5 ││ 6 ││ 7 ││ 8 ││ 9 ││10 │
  └───┘└───┘┗━━━┛┗━━━┛└───┘└───┘└───┘└───┘└───┘└───┘
                 ◀────────                          
                  take: -2                          
                                                    
                                                    
                                                    
                    cursor: 5                       
                     skip: 2                        
                        │                           
                        │                           
                        │                           
                        ▼                           
  ┌───┐┏━━━┓┏━━━┓┌───┐┌───┐┌───┐┌───┐┌───┐┌───┐┌───┐
  │ 1 │┃ 2 ┃┃ 3 ┃│ 4 ││ 5 ││ 6 ││ 7 ││ 8 ││ 9 ││10 │
  └───┘┗━━━┛┗━━━┛└───┘└───┘└───┘└───┘└───┘└───┘└───┘
                 ◀────────                          
                  take: -2                          
                                                    
                                                    
            cursor: 5                               
            skip: 0 or undefined                    
                      │                             
                      │                             
                      │                             
                      ▼                             
┌───┐┌───┐┌───┐┌───┐┏━━━┓┏━━━┓┏━━━┓┌───┐┌───┐┌───┐  
│ 1 ││ 2 ││ 3 ││ 4 │┃ 5 ┃┃ 6 ┃┃ 7 ┃│ 8 ││ 9 ││10 │  
└───┘└───┘└───┘└───┘┗━━━┛┗━━━┛┗━━━┛└───┘└───┘└───┘  
                      ──────────▶                   
                        take: 3                     
                                                    
                                                    
                  cursor: 5                         
                  skip: 1                           
                      │                             
                      │                             
                      │                             
                      ▼                             
┌───┐┌───┐┌───┐┌───┐┌───┐┏━━━┓┏━━━┓┏━━━┓┌───┐┌───┐  
│ 1 ││ 2 ││ 3 ││ 4 ││ 5 │┃ 6 ┃┃ 7 ┃┃ 8 ┃│ 9 ││10 │  
└───┘└───┘└───┘└───┘└───┘┗━━━┛┗━━━┛┗━━━┛└───┘└───┘  
                      ──────────▶                   
                        take: 3                     
                                                    
                                                    
                  cursor: 5                         
                  skip: 2                           
                      │                             
                      │                             
                      │                             
                      ▼                             
┌───┐┌───┐┌───┐┌───┐┌───┐┌───┐┏━━━┓┏━━━┓┏━━━┓┌───┐  
│ 1 ││ 2 ││ 3 ││ 4 ││ 5 ││ 6 │┃ 7 ┃┃ 8 ┃┃ 9 ┃│10 │  
└───┘└───┘└───┘└───┘└───┘└───┘┗━━━┛┗━━━┛┗━━━┛└───┘  
                      ──────────▶                   
                        take: 3                     

@dpetrick
Copy link
Contributor

If cursor is present, but take is not (it's undefined), then we just take the item at the cursor.

Not really what I expected. Just setting a cursor should set the starting point of the read in the list. Everything after and including cursor should be returned.

@matthewmueller
Copy link
Contributor Author

Good catch. Completely agree. Without take, there's no limit.

@dpetrick
Copy link
Contributor

Implemented in: prisma/prisma-engines#767

@dpetrick dpetrick mentioned this pull request May 29, 2020
Jolg42 added a commit to prisma/prisma that referenced this pull request May 29, 2020
Jolg42 added a commit to prisma/prisma that referenced this pull request May 29, 2020
@janpio janpio modified the milestones: Beta 7, Beta 8 Jun 2, 2020
@pantharshit00
Copy link
Contributor

Shouldn't this be merged now?

@matthewmueller
Copy link
Contributor Author

Not yet. I still need to write down some of the on-the-fly decisions we made during implementation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants