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

Overloaded meaning for limit/offset in pagination guide vs. database engines #51

Open
davidlougheed opened this issue May 17, 2024 · 3 comments
Assignees

Comments

@davidlougheed
Copy link

Hello,

I'm associated with the EpiShare driver project at GA4GH.

I'm looking to implement the GA4GH pagination spec for our own APIs, to have eventual consistency between them and standard APIs we also implement (Beacon V2, etc.)

I'm finding the use of limit/offset to be a bit misleading in the current pagination guide, since these terms already have well-established meanings in database engines such as Postgres - where offset means a record-offset, not a page offset, and limit in both cases means the number of records to cap the result-set at; meaning that the "units" of these two parameters are non-obviously different where in other contexts (SQL) the terms are the same, but with different (and internally consistent) "units".

There's also currently a typo in v3 of the draft guide, saying that with offset=2; limit=10 would give records 11-20, where it should yield records 21-30. If a record-offset was used, this inconsistency becomes immediately apparent (offset=10;limit=10 naturally yields 11-20 with 1-indexed records).

I think this dual meaning for the terms is likely to make for inconsistent implementation in real-world situations.

One possible way to address this would be to switch these to something like page and page_size, to make the units clearer and eliminate the overloaded meaning; another would be to switch to recommending record-based offsets instead. As an implementer, I'm reluctant to use the offset-based recommendations in their current state.

@uniqueg
Copy link

uniqueg commented Jun 18, 2024

Good point, @davidlougheed, and I agree that this should be addressed. I am fine with both suggestions, no real preference.

@andrewyatz
Copy link
Contributor

Thanks all I will review and introduce into the v4 draft of the document. Plus thank you @davidlougheed for pointing out the logic issues and sorry for the delay. I didn't see the issue come in at all

@andrewyatz
Copy link
Contributor

I've integrated your suggestions and fix into the new version of the pagination guide. Please review and see what you think about these changes. I really value your time here and also attempting to adopt the work we've done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants