feat: Add optimization feature for composite cursors #62
Conversation
Current implementation of pagination on composite cursors is rather inefficient on large tables even in the presence of index. Changes in this commit add an optimization option that causes paginator to emit a bit different WHERE condition that query optimizators have an easier time optimizing. We kept changes backward-compatible. We need to opt-in to get the new feature enabled.
Pull Request Test Coverage Report for Build 9281306496Details
💛 - Coveralls |
There was a problem hiding this comment.
This PR is so great. I haven't used tuple comparison before and indeed learn a lot from the changes, only a few minor suggestions. Thank you so much for the improvement.
One question would like to consult with you is that, making this feature optional is due to not all database vendors supporting this optimization, or not offering a consistent behavior of it, or perhaps any other considerations?
paginator/option.go
Outdated
| Keys: []string{"ID"}, | ||
| Limit: 10, | ||
| Order: DESC, | ||
| TupleCmp: DISABLED, |
There was a problem hiding this comment.
What about just using bool for this flag, and default to false?
There was a problem hiding this comment.
And AllowTupleCmp could be a better name to me, which is also aligning with the internal state.
There was a problem hiding this comment.
We cannot use boolean directly here. Because of the way the configuration code is structured right now, each option needs to have some value that indicates the "not set, please ignore" state. For strings, we (ab)use "" for this purpose, and for integers, 0 is treated as unset marker. But since booleans only have two valid values that both carry meaning for us, we cannot use them directly.
This is why I opted to implement a new flag type that gives us that additional state. Alternatives would be:
- Use pointer to a boolean, which will give us
nilas an additional state, but will make usage awkward (&falseis not something we can do in go, so we would either need a monstrosity like&[]bool{false}[0]or a helper function). - Do not allow resetting the tuple comparison option and treat
falseas an unset state option.
It would be ideal if Go's stdlib would provide an optional type that we could use here, but we are not there yet. And while I could write an Option[T] wrapper and use it here, that would stick out like a sore thumb from the rest of the code.
That being said, I am open to suggestions on how to proceed here.
There was a problem hiding this comment.
I agree with that pointer is bothering to use. 😔 Although the second alternative is more favorable to me, current options are all implemented based on a shared config structure, which might accidentally override the state if we treat false as unset. For instance:
paginator.New(
WithAllowTupleCmp(true),
// AllowTupleCmp would be overridden to false in other option
WithRules(...)
)Unless with further refinement of config and option, Flag would be the best choice to this feature. We can go for Flag at this moment. 🙌
What about the naming? In my opinion naming the flag to AllowTupleCmp would be more consistent across the whole codebase. 🙂
There was a problem hiding this comment.
What about the naming?
Sorry, I forgot to comment on that and update the code. I am OK with rename.
There was a problem hiding this comment.
That's fine. After document refinement, this PR is ready to go 🚀
Co-authored-by: Cyan Ho <pilagooood@gmail.com>
Co-authored-by: Cyan Ho <pilagooood@gmail.com>
|
@pilagod Thank you for you patience and reviews of my half-baked commits! |
Current implementation of pagination on composite cursors is rather inefficient on large tables even in the presence of index. Changes in this commit add an optimization option that causes paginator to emit a bit different WHERE condition that query engines have an easier time optimizing.
We kept changes backward-compatible. We need to opt-in to get the new feature enabled.