feat: (re-)enable timestamp+offset based pagination optimization#2145
feat: (re-)enable timestamp+offset based pagination optimization#2145
Conversation
chenba
force-pushed
the
feat/offset-pagination-opt-stor-473
branch
2 times, most recently
from
d86d3b1 to
e4093a9
Compare
pjenvey
left a comment
pjenvey
left a comment
There was a problem hiding this comment.
Sorry for the delay this offset thing always confuses me
| @@ -68,27 +68,16 @@ pub struct Offset { | |||
|
|
|||
| impl Display for Offset { | |||
| fn fmt(&self, fmt: &mut Formatter) -> Result<(), fmt::Error> { | |||
There was a problem hiding this comment.
I don't recall why we ended up/need a duplicate version of Offset here, maybe we can try killing the other one (in a separate issue)
There was a problem hiding this comment.
| // issue559: Revert to previous sorting | ||
| /* | ||
| Sorting::Index => query.order(bso::id.desc()).order(bso::sortindex.desc()), | ||
| Sorting::Newest | Sorting::None => { |
There was a problem hiding this comment.
There's a subtle bug, this small change here to make None act like Newest is I think at least one solution:
encode_next_offset returns the encoded offset for every type of sort except Index, including None. However that special offset isn't going to make much sense if we're not applying the timestamp above (in the Some(ts) _ => {} arm) -- and we probably also need to enforce some kind of ordering here for the pagination to make sense.
I thought we had a test somewhere for such a case but I guess not?
There was a problem hiding this comment.
Thank you for catching that. I've made None behave like Newest.
chenba
force-pushed
the
feat/offset-pagination-opt-stor-473
branch
from
e4093a9 to
4023289
Compare
Co-authored-by: Philip Jenvey <pjenvey@underboss.org>
| @@ -68,27 +68,16 @@ pub struct Offset { | |||
|
|
|||
| impl Display for Offset { | |||
| fn fmt(&self, fmt: &mut Formatter) -> Result<(), fmt::Error> { | |||
There was a problem hiding this comment.
2 participants
#53 enabled
#559 disabled
And this patch enables it for MySQL and Postgres but not Spanner.
Closes STOR-473