Use Pageable.getOffset instead of calculating the start record in JestElasticsearchTemplate
jbogan opened this issue · 3 comments
Request
Could the following line of code in JestElasticsearchTemplate
be changed to use Pageable.getOffset
instead?
Details
In the referenced line of code, the start position is calculated using Pageable.getPageSize * Pageable.getPageNumber
. The same result is also available through the Pageable.getOffset
method.
I know the main difference is Pageable.getOffset
returns a long
and the startRecord
assignment is an int
so there will be some Integer overflow issues to worry about (see Potential Concerns).
Reasoning
The reasoning for asking is we are using a custom Pageable
that works with limits and offsets (similar to https://stackoverflow.com/a/32763885). Unfortunately there does not seem to be an easy way to alter/override the referenced JestElasticsearchTemplate
line without making our own version of the class.
Potential Concerns
My main concerns with the change are:
- It would impact current users of the library that use a custom
Pageable
that doesn't calculate the offset [correctly]. That means this change will have to be communicated clearly in the the release notes. - Potential integer overflow issue during the downcast from
long
toint
. Though, this would be an issue in the current code ifpageSize * pageNumber
was high enough. Plus that would be well into the Elasticsearchmax_result_window
territory.
Way Ahead
If you agree with this change, I'd be happy to submit a PR for approval.
Hi @jbogan , It's really seem's to be the right way to implement this.
I will be very glad if we propose a PR.
Thanks a lot for your feedback.
Julien.