-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add missing refactoring of Scroll to PIT API calls for Joins and Pagination query #2981
Add missing refactoring of Scroll to PIT API calls for Joins and Pagination query #2981
Conversation
1d27eac
to
b50ec7c
Compare
legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/query/planner/logical/node/TableScan.java
Show resolved
Hide resolved
...main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java
Outdated
Show resolved
Hide resolved
@@ -182,7 +182,7 @@ public void validTotalResultWithAndWithoutPaginationOrderBy() throws IOException | |||
String selectQuery = | |||
StringUtils.format( | |||
"SELECT firstname, state FROM %s ORDER BY balance DESC ", TEST_INDEX_ACCOUNT); | |||
verifyWithAndWithoutPaginationResponse(selectQuery + " LIMIT 2000", selectQuery, 26, false); |
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.
I forgot whether this class is the main IT for pagination? Is the new PIT logic enabled to test now?
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.
This class is only responsible for legacy/v1 engine pagination queries. For v2 engine queries, there is a different base class. Currently, integ-test cluster does not set the feature flag settings and by default the cluster settings will have PIT feature flag enabled/set to true. Hence there are around 80+ tests testing new PIT changes.
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.
Not urgent but I think it would be helpful to get the IT run with PIT and Scroll both. In case we need to rollback to Scroll if anything wrong.
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.
Actually we are not disabling/removing Scroll implementation. There is a cluster level settings called SQL_PAGINATION_API_SEARCH_AFTER
which is set to true to use PIT or false to default to Scroll API. So current IT tests run based on the setting of the above key and it covers both Scroll and PIT.
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.
Do you mean the same IT set run 1 pass for PIT and another pass for Scroll?
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.
oh you meant running 2 passes of same set of tests for both PIT and Scroll? Ok I think that can be done by modifying the integ-test cluster settings as required. But currently we do not explicitly set the API settings in the test-cluster, so by default PIT changes are covered as part of IT tests and only 1 pass is run. I'll add this as part of the changes here - #2945
legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/Paginate.java
Outdated
Show resolved
Hide resolved
legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/Paginate.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
Show resolved
Hide resolved
...main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Manasvini B S <[email protected]>
…erator Signed-off-by: Manasvini B S <[email protected]>
Signed-off-by: Manasvini B S <[email protected]>
Signed-off-by: Manasvini B S <[email protected]>
Signed-off-by: Manasvini B S <[email protected]>
feae010
to
6202df0
Compare
Signed-off-by: Manasvini B S <[email protected]>
6202df0
to
770ae41
Compare
/** Indicate the search already done. */ | ||
private boolean searchDone = false; | ||
|
||
private String pitId; |
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.
Add a new PIT request class instead? I think all the conditional check added make this class hard to maintain.
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.
I've captured this improvement as part of this issue -#2945
} | ||
} | ||
|
||
private OpenSearchRequest buildRequestWithPit( |
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.
This method seems a copy of the original buildRequestWithScroll
? I think the only difference is at 119 and 134 which decides to create a Scroll request or PIT request (if you added a new class)
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.
I've an issue created related to the improvements in branching strategy we have today for PIT and Scroll and its related implementation. This and the above comment can be addressed as part of this task: #2945
Let me know if you think this is a blocking comment. If not I would like to get this addressed as part of the other task to reduce duplicate effort.
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.x
# Create a new branch
git switch --create backport/backport-2981-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ce17d0a385d2d231f405fb8fcd8c27cf046cf2ac
# Push it to GitHub
git push --set-upstream origin backport/backport-2981-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.x Then, create a pull request where the |
…nation query (opensearch-project#2981) * Adding PIT for pagination queries in new SQL engine code paths Signed-off-by: Manasvini B S <[email protected]> * Fix legacy code using scroll API instead of PIT for batch physical operator Signed-off-by: Manasvini B S <[email protected]> * Fix local debugger issue Signed-off-by: Manasvini B S <[email protected]> * Refactor integ-tests data for PIT and fix unit tests Signed-off-by: Manasvini B S <[email protected]> * Address feedback comments Signed-off-by: Manasvini B S <[email protected]> * Adding test coverage Signed-off-by: Manasvini B S <[email protected]> --------- Signed-off-by: Manasvini B S <[email protected]>
…nation query (opensearch-project#2981) * Adding PIT for pagination queries in new SQL engine code paths Signed-off-by: Manasvini B S <[email protected]> * Fix legacy code using scroll API instead of PIT for batch physical operator Signed-off-by: Manasvini B S <[email protected]> * Fix local debugger issue Signed-off-by: Manasvini B S <[email protected]> * Refactor integ-tests data for PIT and fix unit tests Signed-off-by: Manasvini B S <[email protected]> * Address feedback comments Signed-off-by: Manasvini B S <[email protected]> * Adding test coverage Signed-off-by: Manasvini B S <[email protected]> --------- Signed-off-by: Manasvini B S <[email protected]> Signed-off-by: Simeon Widdis <[email protected]>
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.18 2.18
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.18
# Create a new branch
git switch --create backport/backport-2981-to-2.18
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ce17d0a385d2d231f405fb8fcd8c27cf046cf2ac
# Push it to GitHub
git push --set-upstream origin backport/backport-2981-to-2.18
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.18 Then, create a pull request where the |
* Adding PIT for pagination queries in new SQL engine code paths * Fix legacy code using scroll API instead of PIT for batch physical operator * Fix local debugger issue * Refactor integ-tests data for PIT and fix unit tests * Address feedback comments * Adding test coverage --------- Signed-off-by: Manasvini B S <[email protected]> Signed-off-by: Simeon Widdis <[email protected]> Co-authored-by: Manasvini B Suryanarayana <[email protected]>
* Adding PIT for pagination queries in new SQL engine code paths * Fix legacy code using scroll API instead of PIT for batch physical operator * Fix local debugger issue * Refactor integ-tests data for PIT and fix unit tests * Address feedback comments * Adding test coverage --------- Signed-off-by: Manasvini B S <[email protected]> Signed-off-by: Simeon Widdis <[email protected]> Co-authored-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit f6ca54c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Adding PIT for pagination queries in new SQL engine code paths * Fix legacy code using scroll API instead of PIT for batch physical operator * Fix local debugger issue * Refactor integ-tests data for PIT and fix unit tests * Address feedback comments * Adding test coverage --------- (cherry picked from commit f6ca54c) Signed-off-by: Manasvini B S <[email protected]> Signed-off-by: Simeon Widdis <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Manasvini B Suryanarayana <[email protected]>
Description
OpenSearchQueryRequest
- https://github.com/opensearch-project/sql/pull/2981/files#diff-96f998c99595de19404d0dd5c38b71b5fb7c52160cf34a847c2785dbb05f1d03** https://github.com/opensearch-project/sql/pull/2981/files#diff-c1e49e7f69bbd6e7969071effcb3972827ed02ed86f552c03a710f548820fe3a
To-Do:
task :opensearch:jacocoTestCoverageVerification
is failing. Will add more tests to increase coverageRelated Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.