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

add new method to support pagination #57

Closed
wants to merge 21 commits into from

Conversation

zhangzunke
Copy link

@zhangzunke zhangzunke commented Jun 26, 2021

Sir, I added a method to support pagination, please review it. thanks.

Fixes #53

@IEvangelist
Copy link
Owner

This is awesome, thank you so much @zhangzunke - I'll do a full review this weekend. Thanks again

@IEvangelist IEvangelist self-assigned this Jun 26, 2021
Copy link
Owner

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

Here is a few initial observations. I do really like what you have thus far -- would you be able to add some tests, or a sample app usage? See the samples and test directories.

Microsoft.Azure.CosmosRepository/src/DefaultRepository.cs Outdated Show resolved Hide resolved
Microsoft.Azure.CosmosRepository/src/Dtos/PagedResult.cs Outdated Show resolved Hide resolved
Microsoft.Azure.CosmosRepository/src/Dtos/PagedResult.cs Outdated Show resolved Hide resolved
Microsoft.Azure.CosmosRepository/src/Dtos/PagedResult.cs Outdated Show resolved Hide resolved
Microsoft.Azure.CosmosRepository/src/Dtos/PagedResult.cs Outdated Show resolved Hide resolved
Microsoft.Azure.CosmosRepository/src/DefaultRepository.cs Outdated Show resolved Hide resolved
Microsoft.Azure.CosmosRepository/src/DefaultRepository.cs Outdated Show resolved Hide resolved
zhangzunke and others added 18 commits June 27, 2021 09:41
That makes sense.

Co-authored-by: David Pine <david.pine@microsoft.com>
Good catch.

Co-authored-by: David Pine <david.pine@microsoft.com>
Make sense.

Co-authored-by: David Pine <david.pine@microsoft.com>
Good catch

Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
…t.cs

Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
Co-authored-by: David Pine <david.pine@microsoft.com>
@zhangzunke
Copy link
Author

Hi @IEvangelist,
Thanks for your suggestion, and the changes ready for review.

Copy link
Owner

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

Hi again @zhangzunke - thanks for all these updates, I have a few more things I'm curious about.

Also, from the design perspective - as a consumer of this API how would I know if there are more pages, and is there a way to also communicate the total number of pages? If I query for a max of 10 items, and get back 10 items, how would I know if there are more items than the 10 I received for the first page?

@zhangzunke
Copy link
Author

zhangzunke commented Jun 29, 2021

Hi @IEvangelist, good catch, the method GetListAsync will return a PagedResult, it has a property TotalCount which is all of the items count when you query, so a consumer can use the property to calculate the total number of pages, the property is not the current page items count.

@IEvangelist
Copy link
Owner

IEvangelist commented Jun 30, 2021

@zhangzunke Another thought is that the PagedResult should also include some calculated properties about the current page and the total number of pages. Also, how does the consumer get the next page?

@zhangzunke
Copy link
Author

Hi @IEvangelist, the first design is that the method only provides a basic pagination feature, the pagination method only accept SkipCount and MaxResultCount parameters, so you mention the current page and a total number of pages value should be put on consumer side to implementation, that why the PagedResult only gives a total count property to the consumer.

Regarding how to get the next page, because the consumer knows the grid current page, page size and the total count of the data (it comes from PagedResult property), so it is very easy to get the next page.

@IEvangelist
Copy link
Owner

Hi @IEvangelist, the first design is that the method only provides a basic pagination feature, the pagination method only accept SkipCount and MaxResultCount parameters, so you mention the current page and a total number of pages value should be put on consumer side to implementation, that why the PagedResult only gives a total count property to the consumer.

Regarding how to get the next page, because the consumer knows the grid current page, page size and the total count of the data (it comes from PagedResult property), so it is very easy to get the next page.

@zhangzunke Could you show me an example of it being used, so that I can wrap my head around the expected usage? Do you have an example, where you there are 1,037 records that match the query -- and you page them with a max of 50 per page... what would that look like?

@zhangzunke
Copy link
Author

zhangzunke commented Jul 1, 2021

Hi @IEvangelist, I showed a simple example as below, in our project these codes should be implemented by typescript on the SPA, so I just showed the basic cases when we used pagination, is that expected usage for you?

  private int totalPageCount = 0;
  private int pageSize = 50;
  private int currentPage = 1;
  private IReadOnlyList<Media> mediasDataSource;
  public async Task Demo()
  {
      // Click next button
      await ClickNextButtonAsync();

      // click previous button
      await ClickPreviousButtonAsync();
  }

  private async Task ClickNextButtonAsync()
  {
      currentPage++;
      if (currentPage <= totalPageCount)
      {
          await ClickPreviousOrNextButtonAsync(currentPage);
      }
  }

  private async Task ClickPreviousButtonAsync()
  {
      currentPage--;
      if (currentPage > 0)
      {
          await ClickPreviousOrNextButtonAsync(currentPage);
      }
  }

  private async Task<PagedResult<Media>> GetMediaListAsync(int skipCount = 0, int maxResultCount = 50)
  {
      var pagedResultRequest = new PagedResultRequest { SkipCount = skipCount, MaxResultCount = maxResultCount };
      Expression<Func<Media, bool>> predicate = x => x.UserId == "xxxxxxx";
      var mediaList = await _mediaRepository.GetListAsync(predicate, pagedResultRequest);
      return mediaList;
  }

  private async Task ClickPreviousOrNextButtonAsync(int currentPage)
  {
      var mediaList = await GetMediaListAsync((currentPage - 1) * pageSize, pageSize);
      mediasDataSource = mediaList.Items;
      // get total page count, the value will be 21
      totalPageCount = (int)Math.Ceiling(Convert.ToDouble((decimal)mediaList.TotalCount / pageSize));
  }

@zhangzunke zhangzunke closed this Jul 7, 2021
@zhangzunke
Copy link
Author

Cosmos DB team suggest using continuation tokens instead of the Skip/Take approach to pagination, so I closed the PR.

The RU charge of a query with OFFSET LIMIT will increase as the number of terms being offset increases. For queries that have multiple pages of results, we typically recommend using continuation tokens. Continuation tokens are a "bookmark" for the place where the query can later resume. If you use OFFSET LIMIT, there is no "bookmark". If you wanted to return the query's next page, you would have to start from the beginning.

@roji
Copy link

roji commented Jul 12, 2021

Quoting @AndriySvyryd here, the EF Cosmos provider automatically uses continuation tokens under the hood when normal query results are enumerated. In other words, for optimal pagination, simply execute your query and stream the results (e.g. with foreach); the provider will fetch results incrementally. Avoid buffering the entire results e.g. with ToList. dotnet/efcore#24513 tracks allowing the user to set the number of items fetched each time.

Using Skip may still be useful if one doesn't want to traverse the first N items; but executing the same query with different Skip/Take windows would result in bad perf and increased costs.

(@AndriySvyryd will correct if I wrote anything wrong)

@IEvangelist
Copy link
Owner

@all-contributors please add @zhangzunke for ideas and code

@allcontributors
Copy link
Contributor

@IEvangelist

I've put up a pull request to add @zhangzunke! 🎉

@IEvangelist
Copy link
Owner

@all-contributors please add @roji for research

@allcontributors
Copy link
Contributor

@IEvangelist

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@IEvangelist
Copy link
Owner

@all-contributors please add @roji for research

@allcontributors
Copy link
Contributor

@IEvangelist

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@IEvangelist
Copy link
Owner

@all-contributors please add @roji for review

@allcontributors
Copy link
Contributor

@IEvangelist

I've put up a pull request to add @roji! 🎉

@mumby0168
Copy link
Collaborator

Quoting @AndriySvyryd here, the EF Cosmos provider automatically uses continuation tokens under the hood when normal query results are enumerated. In other words, for optimal pagination, simply execute your query and stream the results (e.g. with foreach); the provider will fetch results incrementally. Avoid buffering the entire results e.g. with ToList. dotnet/efcore#24513 tracks allowing the user to set the number of items fetched each time.

Using Skip may still be useful if one doesn't want to traverse the first N items; but executing the same query with different Skip/Take windows would result in bad perf and increased costs.

(@AndriySvyryd will correct if I wrote anything wrong)

Hi I have been trying to wrap my head around the refresh tokens would this comment on the open issue sound about right, continuation tokens are great for a load more ... scenario but for someone who wants to skip directly to page 6 for example they would not have any real benefit?

#53 (comment)

Thanks in advance :)

@roji
Copy link

roji commented Aug 6, 2021

@mumby0168 if my mental model is correct (not an expert), you can still use Skip to skip directly to page 6, and from that point, stream everything else (using continuation tokens). IIRC doing so wouldn't reduce Cosmos processing costs for the 5 skipped pages, but would save you from having to transfer that data over the network.

@mumby0168
Copy link
Collaborator

mumby0168 commented Aug 6, 2021

@mumby0168 if my mental model is correct (not an expert), you can still use Skip to skip directly to page 6, and from that point, stream everything else (using continuation tokens). IIRC doing so wouldn't reduce Cosmos processing costs for the 5 skipped pages, but would save you from having to transfer that data over the network.

Got it, put the basis of my plan together in this draft PR #82 , for the implementation if you could spare some time to have a look it would be greatly appreciated 👍

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.

Feature request: pagination support
4 participants