Add missing refactoring of Scroll to PIT API calls for Joins and Pagination query #2981

manasvinibs · 2024-09-04T23:26:02Z

Description

Adds new point in time API class for join queries request while doing table scan in legacy code - https://github.com/opensearch-project/sql/pull/2981/files#diff-445722c06193dfbd4b0c26d95df7961120b7769ef008a3582014c6d7ba494da1
Adds missing check for Pagination flag in Table scan operator - https://github.com/opensearch-project/sql/pull/2981/files#diff-039759a62de98418d87120c314e887f5fab1f5758be64cd09c8c5c4c6f884c0d
Refactors new sql engine code to add PIT support while building OpenSearchQueryRequest - https://github.com/opensearch-project/sql/pull/2981/files#diff-96f998c99595de19404d0dd5c38b71b5fb7c52160cf34a847c2785dbb05f1d03
Adds PIT flag check in places where scroll request was used default - https://github.com/opensearch-project/sql/pull/2981/files#diff-e4612a7ad8f1be8d78c421b592dfbf6a65dbcf11f00b858b8c41b8055b18dbb5
** https://github.com/opensearch-project/sql/pull/2981/files#diff-c1e49e7f69bbd6e7969071effcb3972827ed02ed86f552c03a710f548820fe3a

To-Do:

Currently task :opensearch:jacocoTestCoverageVerification is failing. Will add more tests to increase coverage

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java

legacy/src/main/java/org/opensearch/sql/legacy/query/planner/logical/node/TableScan.java

...main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java

dai-chen · 2024-09-17T23:34:04Z

integ-test/src/test/java/org/opensearch/sql/legacy/CursorIT.java

@@ -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);


I forgot whether this class is the main IT for pagination? Is the new PIT logic enabled to test now?

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.

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.

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.

Do you mean the same IT set run 1 pass for PIT and another pass for Scroll?

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

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java

...main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java

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]>

dai-chen · 2024-09-23T18:39:10Z

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java

  /** Indicate the search already done. */
  private boolean searchDone = false;

+  private String pitId;


Add a new PIT request class instead? I think all the conditional check added make this class hard to maintain.

I've captured this improvement as part of this issue -#2945

dai-chen · 2024-09-23T18:40:41Z

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java

+    }
+  }
+
+  private OpenSearchRequest buildRequestWithPit(


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)

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.

manasvinibs force-pushed the SQL-PIT-Refactor2 branch 3 times, most recently from 1d27eac to b50ec7c Compare September 17, 2024 16:52

manasvinibs marked this pull request as ready for review September 17, 2024 17:45

manasvinibs requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224 and LantaoJin as code owners September 17, 2024 17:45

dai-chen reviewed Sep 17, 2024

View reviewed changes

ykmr1224 reviewed Sep 18, 2024

View reviewed changes

manasvinibs added 4 commits September 20, 2024 15:56

Adding PIT for pagination queries in new SQL engine code paths

e25fb26

Signed-off-by: Manasvini B S <[email protected]>

Fix legacy code using scroll API instead of PIT for batch physical op…

fa00ea5

…erator Signed-off-by: Manasvini B S <[email protected]>

Fix local debugger issue

f364e52

Signed-off-by: Manasvini B S <[email protected]>

Refactor integ-tests data for PIT and fix unit tests

e70dc4f

Signed-off-by: Manasvini B S <[email protected]>

Address feedback comments

edbc586

Signed-off-by: Manasvini B S <[email protected]>

manasvinibs force-pushed the SQL-PIT-Refactor2 branch from feae010 to 6202df0 Compare September 20, 2024 23:18

manasvinibs requested review from ykmr1224 and dai-chen September 20, 2024 23:38

Adding test coverage

770ae41

Signed-off-by: Manasvini B S <[email protected]>

manasvinibs force-pushed the SQL-PIT-Refactor2 branch from 6202df0 to 770ae41 Compare September 23, 2024 00:41

dai-chen reviewed Sep 23, 2024

View reviewed changes

ykmr1224 approved these changes Sep 23, 2024

View reviewed changes

manasvinibs requested a review from dai-chen September 23, 2024 22:44

dai-chen approved these changes Sep 23, 2024

View reviewed changes

ykmr1224 added the feature label Sep 23, 2024

ykmr1224 merged commit ce17d0a into opensearch-project:main Sep 23, 2024
13 of 15 checks passed

manasvinibs added the v2.18.0 Issues targeting release v2.18.0 label Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

manasvinibs commented Sep 4, 2024 •

edited

Loading

dai-chen Sep 17, 2024

manasvinibs Sep 20, 2024

dai-chen Sep 23, 2024

manasvinibs Sep 23, 2024 •

edited

Loading

dai-chen Sep 23, 2024

manasvinibs Sep 23, 2024

dai-chen Sep 23, 2024

manasvinibs Sep 23, 2024

dai-chen Sep 23, 2024

manasvinibs Sep 23, 2024

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

manasvinibs commented Sep 4, 2024 • edited Loading

Description

Related Issues

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manasvinibs Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manasvinibs commented Sep 4, 2024 •

edited

Loading

manasvinibs Sep 23, 2024 •

edited

Loading