Remove rest of page limit #5429
Conversation
|
I released i-love-jesus version 0.2.0. To migrate the lemmy code, replace https://docs.rs/i-love-jesus/0.2.0/i_love_jesus/struct.PaginatedQueryBuilder.html#method.new |
|
Sweet thx, I'll give it a try now. |
|
When should I be using page_before_or_equal ? For a simple example, check this block of the person_view list banned code. Currently I have Right now I'm just passing |
That is correct. The parameter is only used for the optimization that post view does. |
| ActiveMonthly => pq.then_order_by(key::users_active_month), | ||
| ActiveWeekly => pq.then_order_by(key::users_active_week), | ||
| ActiveDaily => pq.then_order_by(key::users_active_day), | ||
| // TODO the lower function doesn't work for these |
There was a problem hiding this comment.
Create a LowerKey struct, exactly the same as ReverseTimestampKey other than what SQL function it calls.
| @@ -256,6 +271,7 @@ impl CommentQuery<'_> { | |||
| // + !post_id isn't used anyways (afaik) | |||
| if o.post_id.is_some() || o.parent_path.is_some() { | |||
| // Always order by the parent path first | |||
| // TODO not sure if this initial then_order_by will work correctly | |||
There was a problem hiding this comment.
Doing any sorting before creating the paginated query builder is always incorrect.
There was a problem hiding this comment.
This one I couldn't figure out and I'll need your help with. Problem is here, trying do a subpath sort:
pq = pq.then_order_by(subpath(key::path, 0, -1));
There was a problem hiding this comment.
That can be done using a new struct similar to ReverseTimestampKey but it would call subpath(x, 0, -1) instead of reverse_timestamp_key(x). If it needs to be in a different direction than the direction that PaginatedQueryBuilder is initialized with, then something more complicated is needed, which I could try to figure out when I have time.
There was a problem hiding this comment.
If I understand correctly, it doesn't matter if the subpath sort is ascending or descending, just like the id tie-breaker at the end
There was a problem hiding this comment.
Also, to prevent the confusion that I had, add a comment saying that putting comments underneath their parents is done by the frontend instead of in the db query
|
|
||
| // Only sort by ascending for Old | ||
| let sort = o.sort.unwrap_or(CommentSortType::Hot); | ||
| let sort_direction = asc_if(sort == CommentSortType::Old); |
There was a problem hiding this comment.
This will cause distinguished comments to go to the bottom when sorting by old. Either use ReverseTimestampKey here again, conditionally invert the distinguished key (using a new wrapper similar to ReverseTimestampKey), or don't sort by distinguished at all when using the Old sort type (I think this is the best solution, and maybe it also should be done for New sort for consistency). Update indexes to match.
There was a problem hiding this comment.
K I got this one handled (indexes I'll go over shortly.)
| // |pool| PersonActions::read_blocks_for_person(pool, person_id), | ||
| // |pool| CommunityModeratorView::for_person(pool, person_id, Some(&local_user_view.local_user)), | ||
| // |pool| LocalUserLanguage::read(pool, local_user_id) | ||
| // )) |
There was a problem hiding this comment.
Do all of these functions return the same error type?
|
|
||
| for comment_id in &comment_ids { | ||
| Comment::update(&mut conn.into(), *comment_id, form).await?; | ||
| } |
There was a problem hiding this comment.
This query is extremely inefficient if there are more than a dozen comments. It would be much more efficient with update(comment::table).filter(comment_id.eq_any(comment_ids)).set(form)
|
I'm going to wait for #5515 to be merged first since there are going to be a ton of conflicts. |
WIP
#4517