-
-
Notifications
You must be signed in to change notification settings - Fork 882
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
Post view: move cursor pagination to separate library, add backward pagination to PostQuery #4320
Merged
Merged
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
3ec3ce1
stuff
dullbananas 4ebf37d
Merge remote-tracking branch 'upstream/main' into i-love-jesus
dullbananas 6144861
stuff
dullbananas 281c61f
crates.io
dullbananas d41bdcc
Merge branch 'main' into i-love-jesus
dullbananas d858b87
Update up.sql
dullbananas ef80e5d
Rerun federation tests
dullbananas fb28321
Update post_view.rs
dullbananas 4ed77aa
Merge branch 'main' into i-love-jesus
dullbananas 9bc6534
Update post_view.rs
dullbananas 2fe52ef
Merge branch 'main' into i-love-jesus
dullbananas 027a9b1
Merge branch 'main' into i-love-jesus
dullbananas cf81ddf
Update up.sql
dullbananas b276cdc
Update utils.rs
dullbananas 81958cf
Merge branch 'main' into i-love-jesus
dullbananas b8be479
Merge branch 'main' into i-love-jesus
dullbananas efe8a8f
Fix precision loss
dullbananas 074fe2c
Merge branch 'main' into i-love-jesus
dullbananas a3844ec
Update up.sql
dullbananas b554218
Update down.sql
dullbananas c2721c3
remove unwrap
dullbananas bc77ae1
Merge branch 'main' into i-love-jesus
dullbananas 74fdb4b
Merge branch 'main' into i-love-jesus
dessalines 0424bf6
Update post_view.rs
dullbananas e0418d1
Merge branch 'main' into i-love-jesus
dullbananas f06308c
Merge branch 'main' into i-love-jesus
dessalines File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be renamed to something more descriptive? like
diesel-tuple-pagination
maybe. Also not sure what dessalines/nutomic think of depending on an external library like this, since it does seem fairly specific to lemmy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will not rename my library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't like the extra complication cursor pagination requires in general, with all of these pre-fetching of bounds, macros, etc. It makes
post_view.rs
so much harder to read and understand even to me.Every example I can find online of cursor pagination is where you are sorting based off a single id or time column, rather than multiple filter and sort columns like we have. Its unfortunate that limit and offset are such a performance burden, since they require none of this complication, but I spose there's no way around it.
Whether that complicated cursor logic lives in lemmy, or elsewhere where it can be used by other projects, doesn't matter too much to me, but the latter seems better if it helps other diesel projects besides ours. We're dependent on a lot of diesel libraries that aren't made by its maintainer.
As far as the name, it doesn't bother me personally, but I could see other people not wanting to use it because of the name. For limited-scope libraries it makes sense to name the thing what it does, and it seems like a lot of other diesel projects are named things like
diesel-....
or...-diesel
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarification, the prefetching is unrelated to the cursor pagination - I just added it in the same PR because it was a convenient related change. It's a performance boost for the post query in general with the "subscribed" filter, even for the first page where the pagination method is irrelevant. For the page-based pagination the same method could in fact be used for both sides of the page so the db doesn't have to join everything while trying to find the page start and end.