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

Refactor hybrid query pagination logic #1097

Open
vibrantvarun opened this issue Jan 14, 2025 · 3 comments
Open

Refactor hybrid query pagination logic #1097

vibrantvarun opened this issue Jan 14, 2025 · 3 comments

Comments

@vibrantvarun
Copy link
Member

Refactor hybrid query pagination logic to be consistent.

  1. Think of combining version check and objects.nonnull method in doXContent to define the existence of the field in the query builder.
  2. Add IndexUtil check suggested by @ryanbogan in Update pagination_depth datatype from int to Integer #1094 (comment)
@vibrantvarun
Copy link
Member Author

@martin-gaievski I tried to fit all the minclustercheck and isnull check of doXContent method under one but it breaks bwc in the edge cases.

@vibrantvarun
Copy link
Member Author

I did extensive testing and concluded that it is better now compared to one method for all conditions. Regarding your concern of people making the change unknowingly, we can add comments and also we are following the same pattern of put checks in StreamInput, StreamOutput and fromXContent. It won't be an issue.

@martin-gaievski
Copy link
Member

can you be more specific, what is the edge case that you couldn't handle properly in your local change? I don't think such code is not possible, it can be not the best from implementation or code organization point of view. You can do draft PR, this will make discussion more productive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants