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

Cursor Pagination #754

Merged
merged 7 commits into from
Jun 26, 2022
Merged

Cursor Pagination #754

merged 7 commits into from
Jun 26, 2022

Conversation

billy1624
Copy link
Member

@billy1624 billy1624 commented May 20, 2022

PR Info

Adds

  • Add Cursor & CursorTrait, implementing cursor pagination for SeaORM

@billy1624 billy1624 self-assigned this May 20, 2022
@billy1624 billy1624 marked this pull request as ready for review May 20, 2022 08:05
@billy1624 billy1624 requested a review from tyt2y3 May 20, 2022 08:05
@Sytten
Copy link
Contributor

Sytten commented May 20, 2022

Nice API! My biggest critic is that people will want to paginate on models that dont have a natural order via the PK. And supporting that will require a breaking change.

src/executor/cursor.rs Show resolved Hide resolved
src/executor/cursor.rs Outdated Show resolved Hide resolved
src/executor/cursor.rs Outdated Show resolved Hide resolved
@tyt2y3
Copy link
Member

tyt2y3 commented May 23, 2022

I am a little confused that why there isn't any raw SQL unit tests to illustrate the behaviour
And that the mock test part is not proving anything

@billy1624
Copy link
Member Author

billy1624 commented May 25, 2022

Nice API! My biggest critic is that people will want to paginate on models that dont have a natural order via the PK. And supporting that will require a breaking change.

Hey @Sytten, I have updated the API to... which user have full control over what column to filter

Entity::find()
    .order_by_asc(Column::Id)
    .cursor()
    .after(Column::Id, 10)
    .last(2)
    .all(&db)
    .await?,

@billy1624
Copy link
Member Author

Hey @tyt2y3, unit tests were being updated and now it asserts on plain SQL string

@negezor
Copy link
Contributor

negezor commented Jun 2, 2022

What about order by non-unique keys? 🤔

@Sytten
Copy link
Contributor

Sytten commented Jun 2, 2022

What about order by non-unique keys? 🤔

You always need a column that is totally orderable for cursor pagination, even if you order by a partially orderable one.

src/executor/cursor.rs Outdated Show resolved Hide resolved
src/executor/cursor.rs Outdated Show resolved Hide resolved
src/executor/cursor.rs Outdated Show resolved Hide resolved
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 19, 2022

Thank you so far. Great work!

I think there are 2 things we need to address:

Instead of "inheriting" the order by clause, we should override it by the cursor column the user passes in via the cursor() method, and so we don't assume it is ordered already, and don't have to flip-flop with that "reverse ordering" thing.

The Cursor itself should hold which column to paginate on. As a result, the API can be simplified into:

Entity::find()
    .cursor(Column::Id)
    .after(10)
    .last(2)
    .all(&db)
    .await?

And by the way, is there a real use case where we want to have a tuple of columns as cursor? We can always add that later with IntoCursor and impl IntoCursor for (A, B) trick we have been doing all along.

Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

As stated

@Sytten
Copy link
Contributor

Sytten commented Jun 19, 2022

@tyt2y3 like I commented above, you need to support two column if you want to support partially orderable columns. If not then you can use one column.

@billy1624 billy1624 force-pushed the cursor-pagination branch from 1e2c1c4 to 9720c63 Compare June 20, 2022 08:03
@billy1624 billy1624 force-pushed the cursor-pagination branch from 9720c63 to bf882c8 Compare June 20, 2022 10:12
@billy1624 billy1624 force-pushed the cursor-pagination branch from a4ea08e to 2201e4b Compare June 21, 2022 02:54
@tyt2y3 tyt2y3 merged commit 6f9dbd2 into master Jun 26, 2022
@tyt2y3 tyt2y3 deleted the cursor-pagination branch June 26, 2022 09:48
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 26, 2022

oh I didn't merge the sea-query side yet

tyt2y3 added a commit that referenced this pull request Jun 26, 2022
@tyt2y3 tyt2y3 restored the cursor-pagination branch June 26, 2022 10:24
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 26, 2022

I reverted this merge. We need to upgrade sea-query to 0.25 first.
As stated before, git = XXX can never be merged into master to avoid dependency confusion.

@tyt2y3 tyt2y3 mentioned this pull request Jun 26, 2022
billy1624 added a commit to SeaQL/seaql.github.io that referenced this pull request Jul 13, 2022
tyt2y3 added a commit to SeaQL/seaql.github.io that referenced this pull request Jul 17, 2022
* Custom join condition (SeaQL/sea-orm#793)

* Migration does not depend on entity crate

* Define integer enum with repr[x] syntax

* Document datatype mappings (SeaQL/sea-orm#772)

* Cursor pagination (SeaQL/sea-orm#754, SeaQL/sea-orm#822)

* (de)serialize custom JSON types (SeaQL/sea-orm#794)

* Generate new migration file (SeaQL/sea-orm#656)

* Skip generating entity file for specific tables (SeaQL/sea-orm#837)

* Generate entity with date time crate option (SeaQL/sea-orm#724)

* Drop `SelectTwoMany::one()` method (SeaQL/sea-orm#813)

* Datatype mappings of primitives (SeaQL/sea-orm#850, SeaQL/sea-schema#75)

* Join with table alias (SeaQL/sea-orm#852)

* SQLx logging level (SeaQL/sea-orm#800)

* Insert with on conflict (SeaQL/sea-orm#791)

* Migrate generate should take file name as argument instead of option (SeaQL/sea-orm#870)

* Upgrade docusaurus to 2.0.0-beta.22

* What's new in SeaORM 0.9.0

* Move migration section forward

* Rename "Generating Database Schema" section to "Generating SeaQuery Statement"

* Fix broken links

* Edit

* Edit

* Edit

* Edit

Co-authored-by: Chris Tsang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Cursor Based Pagination
5 participants