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

Ensure backwards and descending pagination work with id-only optimization. #208

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

myronmarston
Copy link
Collaborator

@myronmarston myronmarston commented Feb 21, 2025

In #205, I implemented an optimization, where we avoid a datastore query when the client is only requesting ids which we already have. As part of that, I implemented sorting and pagination support so that the expected page and ordering of ids are returned.

However, I only implemented the simplest case: foward ascending pagination. After merging that, I realized that my implementation wouldn't work for the three other pagination cases:

  • Forward descending pagination (e.g. first: N, after: cursor, orderBy: [id_DESC])
  • Backwards ascending pagination (e.g. last: N, before: cursor, orderBy: [id_ASC])
  • Backwards descending pagination (e.g. last: N, before: cursor, orderBy: [id_DESC])

To fix those three cases, I had to change two things in the implementation:

  • Get the sort from the pagination datastore body rather than directly from query. query.sort contains the sort order requested in the GraphQL query, but for some of the above cases we wind up reversing the sort direction we send in the datastore query.
  • Apply the pagination cursor correctly when results are sorted descending. Previously, we filtered using id > search_after for all cases, but we need to use id < search_after when we are sorting descending.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@myronmarston myronmarston force-pushed the myron/optimization-backwards-paginate branch 2 times, most recently from 0fee94c to 36dd8e3 Compare February 21, 2025 16:58
…tion.

In #205, I implemented an optimization, where we avoid a datastore query
when the client is only requesting ids which we already have. As part of
that, I implemented sorting and pagination support so that the expected
page and ordering of ids are returned.

However, I only implemented the simplest case: foward ascending pagination.
After merging that, I realized that my implementation wouldn't work for the
three other pagination cases:

* Forward descending pagination (e.g. `first: N, after: cursor, orderBy: [id_DESC]`)
* Backwards ascending pagination (e.g. `last: N, before: cursor, orderBy: [id_ASC]`)
* Backwards descending pagination (e.g. `last: N, before: cursor, orderBy: [id_DESC]`)

To fix those three cases, I had to change two things in the implementation:

* Get the `sort` from the `pagination` datastore body rather than directly from `query`.
  `query.sort` contains the sort order requested in the GraphQL query, but for some of
  the above cases we wind up reversing the sort direction we send in the datastore query.
* Apply the pagination cursor correctly when results are sorted descending.
  Previously, we filtered using `id > search_after` for all cases, but we need to use
  `id < search_after` when we are sorting descending.
@myronmarston myronmarston force-pushed the myron/optimization-backwards-paginate branch from 36dd8e3 to 2888ac9 Compare February 21, 2025 16:59
Copy link
Collaborator

@BrianSigafoos-SQ BrianSigafoos-SQ left a comment

Choose a reason for hiding this comment

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

LGTM

@myronmarston myronmarston merged commit f56f4af into main Feb 21, 2025
18 of 19 checks passed
@myronmarston myronmarston deleted the myron/optimization-backwards-paginate branch February 21, 2025 20:28
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.

3 participants