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 Pagination To List Apis #9782

Merged
merged 20 commits into from
May 3, 2024
Merged

Conversation

rahil-c
Copy link
Contributor

@rahil-c rahil-c commented Feb 22, 2024

Implemented pagination in list apis based on the spec: #9660
@jackye1995 @nastra @danielcweeks @geruh

Testing

  • Ran gradle build
  • Manual test passed using spark sql and rest catalog

@github-actions github-actions bot added the core label Feb 22, 2024
@rahil-c
Copy link
Contributor Author

rahil-c commented Mar 26, 2024

@jackye1995 @rdblue @danielcweeks @nastra If you can take a look at the revision for this whenever you guys get a chance would be appreciated.

@danielcweeks
Copy link
Contributor

I just want to add a few high-level comments here:

  1. It looks like pageSize is currently required, but I believe the default should be that if it is not set, no pagination should be requested and all results are returned. This is going to be the typical behavior for any REST catalog that does not support paging.
  2. If a REST server has a specific page limit, it should return that as part of the config route properties as an override. Implementations may have different limits and we should not require users to configure the client to correspond to the limitations of the server.

As a follow up to #2, it seems there may be a chase were some servers require pagination, so they will probably need to return a 400 BadRequest if a client is asking for all values in a request and the server requires they support pagination.

@rahil-c
Copy link
Contributor Author

rahil-c commented Apr 4, 2024

I just want to add a few high-level comments here:

  1. It looks like pageSize is currently required, but I believe the default should be that if it is not set, no pagination should be requested and all results are returned. This is going to be the typical behavior for any REST catalog that does not support paging.
  2. If a REST server has a specific page limit, it should return that as part of the config route properties as an override. Implementations may have different limits and we should not require users to configure the client to correspond to the limitations of the server.

As a follow up to #2, it seems there may be a chase were some servers require pagination, so they will probably need to return a 400 BadRequest if a client is asking for all values in a request and the server requires they support pagination.

@danielcweeks @nastra @rdblue @jackye1995 @sachet

  1. Will fix this
  2. im curious if this page limit is to be returned by the rest server, should this be apart of the capabilities pr? I think from the pr OpenAPI: Express server capabilities via /config endpoint #9940 we did not want to have pagination listed there but wanted to bring this up.

@rahil-c rahil-c requested a review from sachet April 4, 2024 01:14
@rahil-c rahil-c requested a review from nastra April 24, 2024 15:30
Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

I've left a few minor comments, but this LGTM now. Thanks @rahil-c. I'll wait a bit with merging to also give @danielcweeks a chance to take another look

@rahil-c
Copy link
Contributor Author

rahil-c commented Apr 26, 2024

Thanks @nastra for the help I appreciate it, will wait on @danielcweeks review.

@rahil-c
Copy link
Contributor Author

rahil-c commented Apr 30, 2024

@danielcweeks @nastra Have made recent revisions, hoping we can land this if no other concerns and if CI run is green.

@rahil-c rahil-c requested a review from nastra May 1, 2024 03:46
@danielcweeks
Copy link
Contributor

Approved, pending checks. There's one that failed, but may have been a transient failure.

@rahil-c
Copy link
Contributor Author

rahil-c commented May 2, 2024

Approved, pending checks. There's one that failed, but may have been a transient failure.

@danielcweeks yea I think it is transient, since every time I go to check this link it still keeps running.

@rahil-c
Copy link
Contributor Author

rahil-c commented May 2, 2024

@danielcweeks Seems to be green now

@nastra
Copy link
Contributor

nastra commented May 3, 2024

Thanks for working on this @rahil-c

@nastra nastra merged commit 7600ba7 into apache:main May 3, 2024
41 checks passed
@rahil-c
Copy link
Contributor Author

rahil-c commented May 3, 2024

Thanks @nastra and @danielcweeks for the help on this!

sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants