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

Implement Cosmos pagination #34103

Merged
merged 6 commits into from
Jul 1, 2024
Merged

Implement Cosmos pagination #34103

merged 6 commits into from
Jul 1, 2024

Conversation

roji
Copy link
Member

@roji roji commented Jun 27, 2024

This didn't turn out to be very hard - check out the test to see it in action.

Suggestions for additional testing scenarios would be appreciated!

Closes #24513

@roji roji requested a review from a team June 27, 2024 15:55
@roji roji marked this pull request as ready for review June 28, 2024 19:44
@@ -5324,6 +5324,65 @@ SELECT c
""");
});

[ConditionalFact]
public virtual async Task ToPageAsync()
Copy link
Member

Choose a reason for hiding this comment

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

Also add a test where responseContinuationTokenLimitInKb value matters.

Copy link
Member Author

@roji roji Jun 29, 2024

Choose a reason for hiding this comment

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

That's trickier than it looks - even setting this to 1kb (the minimum) still passes the test. I suspect that more complex query shapes (and possibly later pages??) should make it go over 1kb, at which point we can assert a failure, but that seems brittle and dependent on internal Cosmos stuff. I've debugged to manually confirm that the value gets passed into the QueryRequestOptions - do you have some other idea on what to do here?

@roji
Copy link
Member Author

roji commented Jun 29, 2024

@AndriySvyryd thanks for the reviewing - pushed some changes, please take a look.

Note especially the commit switching to use ResponseMessageEnumerator for regular document enumeration (and fixing #34092) - can you please take a close look?

@roji
Copy link
Member Author

roji commented Jul 1, 2024

Design decisions:

  • Make continuationToken parameter required
  • Make ToPageAsync experimental on Cosmos, in view of lifting it to core later (i.e. relational implementation). This would also allow us to switch to a non-EF paging abstraction (like the ones in Azure), if that's relevant.
  • Remove regions to improve team morale
  • Rename maxItemCount to pageSize
  • Don't provide sync ToPage()
  • The results on a given page will remain an IReadOnlyList, as opposed to anything streamable (async enumerable).
  • For relational, we may one day want to provide a regular enumerable over the query results, which under the hood fetches chunks/pages; this would allow efficient abort of a query in the middle. This isn't require in Cosmos since results are already fetched in multiple roundtrips. If we do, we can just implement that as a separate API over the ToPageAsync() provided in this PR.

@roji
Copy link
Member Author

roji commented Jul 1, 2024

@dotnet/efteam made the changes as per our decision decisions.

@roji roji enabled auto-merge (squash) July 1, 2024 22:00
@roji roji merged commit fee7c1a into dotnet:main Jul 1, 2024
7 checks passed
@roji roji deleted the CosmosPagination branch July 1, 2024 22:42
@roji roji linked an issue Jul 19, 2024 that may be closed by this pull request
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.

Cosmos: FeedIterator and ResponseMessage aren't properly disposed Cosmos: add support for pagination
2 participants