-
Notifications
You must be signed in to change notification settings - Fork 803
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
Deep paging #1623
Deep paging #1623
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good to merge this PR, since iteration with PIT and search_after is widely uses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While iterating throw the generator, I get such error
pit = search._using.open_point_in_time(
AttributeError: 'str' object has no attribute 'open_point_in_time'
Looks like using is set to 'default'
Looks like _using defualts to the string 'default'. I'll update the PR |
Co-authored-by: Rostyslav Khudov <59306666+rkhudov@users.noreply.github.com>
Co-authored-by: Rostyslav Khudov <59306666+rkhudov@users.noreply.github.com>
I merged in your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will be interesting as well is to add some sort of pagination with search_after and PIT
Yeah, I suppose it would be good to use search_after to page forward and back rather than just scrolling through all results. |
The only way I think this would be possible is if we save |
Hi @reese-allison, I did a question on the issue #1329. |
Hi! This looks really great, and I was hoping to use it in a project I'm working on. Are there plans to merge this sometime soon? Anything I can do to help push it across the finish line? :) |
I've merged main into this pull request and fixed the conflicts. The only CI failure is due to the usage of the walrus operator. It will go away when #1717 is merged, after which we can review this. Thank you! |
@pquentin, thanks for getting this updated for me! I haven't looked at it in a while. |
I will be looking at this PR along with #806 to try to come up with a general approach to pagination. Thanks. |
@reese-allison Thank you so much. Based on your work I have added |
Implements deep paging using search_after and Point in Time (PIT). This can be used to page through all results much more cheaply than using the Elasticsearch scan method. PITs are cheaper to open, so this should be safe for user requests, and can be used as a drop in replacement for scan in many cases.
Closes #1329