Document pagination design changes based on review feedback #1644
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
May 19, 2023 03:28
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
MaxKsyunz
requested review from
Swiddis,
YANG-DB,
Yury-Fridlyand,
dai-chen,
derek-ho,
joshuali925,
kavithacm,
mengweieric,
penghuo,
pjfitzgibbons,
ps48,
rupal-bq,
seankao-az and
vamsimanohar
as code owners
Codecov Report
@@ Coverage Diff @@
## feature/pagination/integ #1644 +/- ##
===========================================================
Coverage 97.23% 97.23%
Complexity 4233 4233
===========================================================
Files 387 387
Lines 10628 10628
Branches 721 721
===========================================================
Hits 10334 10334
Misses 287 287
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. |
acarbonetto
approved these changes
May 19, 2023
Collaborator
acarbonetto
left a comment
acarbonetto
left a comment
There was a problem hiding this comment.
LGTM - some minor comments/typos
docs/dev/Pagination-v2.md
Outdated
|
|
||
| # Scope | ||
| Currenty, V2 engine supports pagination only for simple `SELECT * FROM <table>` queries without any other clauses like `WHERE` or `ORDER BY`. | ||
| At this time, V2 engine supports pagination only for simple `SELECT * FROM <table>` queries without any other clauses like `WHERE` or `ORDER BY`. |
Collaborator
There was a problem hiding this comment.
I feel like we're going to have update this line of the design each time we push a PR.
Maybe describe what the scope of this document contains. They will have to look at release notes and read issues to know what has actually been pushed to repo.
| 1. Execute paginated physical plan. | ||
| 4. in OpenSearch data source: | ||
| 1. Support pagination push down. | ||
| 2. Support other push down optimizations with pagination. |
Collaborator
There was a problem hiding this comment.
What about de-serialization and creating the cursor for the next requests?
docs/dev/Pagination-v2.md
Outdated
|
|
||
| ### Query Plan Changes | ||
|
|
||
| All three kinds of requests — non-paged, initial page, or subsequent page — are processed in the same way. Simplified workflow of query plan processing is shown below for reference. |
Collaborator
There was a problem hiding this comment.
Suggested change
| All three kinds of requests — non-paged, initial page, or subsequent page — are processed in the same way. Simplified workflow of query plan processing is shown below for reference. | |
| All three kinds of requests — non-paged, initial page, or subsequent page — are processed at a high-level in the same way. Simplified workflow of query plan processing is shown below for reference. |
Collaborator
There was a problem hiding this comment.
We want to say that all the requests go through the same high-level steps, although the details/execution of each step may differ.
penghuo
approved these changes
May 23, 2023
| participant ResourceMonitorPlan | ||
| participant OpenSearchPagedIndexScan | ||
| participant OpenSearchIndexScan | ||
| participant OpenSearchScrollRequest |
Collaborator
There was a problem hiding this comment.
a duplicate?
Suggested change
| participant OpenSearchScrollRequest |
Co-authored-by: Andrew Carbonetto <[email protected]> Signed-off-by: Max Ksyunz <[email protected]>
dai-chen
approved these changes
May 23, 2023
Signed-off-by: MaxKsyunz <[email protected]>
GumpacG
approved these changes
May 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates accounts for the following:
OpenSearchIndexScanandOpenSearchPagedIndexScan, related builder classes and changes to logical plan optimization.ContinuePaginatedPlanand how initial and subsequent paged requests are expressed in logical query plan.OpenSearchIndexScanandOpenSearchScrollRequestnecessary to support pagination.