Skip to content
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

Use Pageable.getOffset instead of calculating the start record in JestElasticsearchTemplate #122

Closed
jbogan opened this issue Aug 15, 2019 · 3 comments
Labels

Comments

@jbogan
Copy link

jbogan commented Aug 15, 2019

Request

Could the following line of code in JestElasticsearchTemplate be changed to use Pageable.getOffset instead?

startRecord = query.getPageable().getPageNumber() * query.getPageable().getPageSize();

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:

  1. 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.
  2. Potential integer overflow issue during the downcast from long to int. Though, this would be an issue in the current code if pageSize * pageNumber was high enough. Plus that would be well into the Elasticsearch max_result_window territory.

Way Ahead

If you agree with this change, I'd be happy to submit a PR for approval.

@VanRoy
Copy link
Owner

VanRoy commented Aug 24, 2019

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.

@VanRoy VanRoy added the bug label Aug 24, 2019
@jbogan
Copy link
Author

jbogan commented Aug 27, 2019

Hi @VanRoy, sounds good! I'll find some time to submit the PR within the next week.

@VanRoy
Copy link
Owner

VanRoy commented Aug 31, 2019

Hi @jbogan , Thanks a lot for your help.
You PR is merged and released in 3.2.5.RELEASE version.

@VanRoy VanRoy closed this as completed Aug 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants