search

photocrowd / django-cursor-pagination

Cursor-based pagination for Django
BSD 3-Clause "New" or "Revised" License
156 stars 27 forks source link

Remove caveat of not supporting different directions of ordering #27

Closed yeahframeoff closed 2 years ago

yeahframeoff commented 5 years ago

@Drarok what tests does it need so that it could be merged?

mjtamlyn commented 5 years ago

Hi! This looks like it has all the correct logic in place. A few thoughts:

  1. Is there any performance implication in the database of the longer query structure? If so, could we drop to the tuple style code when the ordering is simpler. Some real-ish benchmarks would be useful.
  2. Can we add some more tests of some more complex situations - 3 or 4 variables with some different orderings (i.e. a, -b, -c, d, a, b, -c, -d - perhaps even all permutations). Won't exercise any new code but will encourage the feeling that this works.
  3. The above OR/AND comment needs updating

This should then allow us to update and merge it.

nitely commented 3 years ago

Is there any performance implication in the database of the longer query structure? If so, could we drop to the tuple style code when the ordering is simpler. Some real-ish benchmarks would be useful.

You can expect this query to be slower than the offset/limit approach, as it won't use the index. There is a variation that will use the index for the first field only, so it'll still be slow in most cases.

AndrewIngram commented 2 years ago

Just bumping this because we found that there's limited utility for this library without support for mixed sort directions. We ended up vendoring this branch for internal use. This PR essentially implements the approach i've had good success with for several years.

patrick91 commented 2 years ago

Just bumping this because we found that there's limited utility for this library without support for mixed sort directions. We ended up vendoring this branch for internal use. This PR essentially implements the approach i've had good success with for several years.

Thanks Andrew! I'll make a not to merge this and make a new major release in the next few weeks!

patrick91 commented 2 years ago

I'll merge this and make a release soon! sorry it took a while to get this in 😊