-
Notifications
You must be signed in to change notification settings - Fork 969
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
feat: order sessions by created_at #3696
Conversation
394b64d
to
668c779
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3696 +/- ##
==========================================
+ Coverage 78.28% 78.32% +0.04%
==========================================
Files 347 346 -1
Lines 23631 23576 -55
==========================================
- Hits 18499 18467 -32
+ Misses 3735 3714 -21
+ Partials 1397 1395 -2 ☔ View full report in Codecov by Sentry. |
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.
Is created_at indexed? If not this will be slow
668c779
to
6460178
Compare
Without any new index (current status quo):
With
I added the index to the migrations. LMK if you see room for improvements. cc @alnr PTAL as well. Ty :) |
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.
PR and index LGTM.
Do we maybe want to add tests to verify:
- results are actually ordered by
created_at
- make sure we're not missing items near a page break when they have identical
created_at
times.
Maybe that's already tested in ory/x? Could also be overkill for this.
Typically, ordering would be done by ID for page pagination to avoid the ambiguity around having two items with the same timestamps. I do understand though why created_at makes sense to order by in this case. |
That's exactly the point of the This is the statement (note the SELECT * FROM sessions AS sessions WHERE nid = $1 AND ("sessions"."created_at" < $2 OR ("sessions"."created_at" = $3 AND "sessions"."id" > $4)) ORDER BY "sessions"."created_at" DESC, "sessions"."id" ASC LIMIT 4 |
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments