Pagination, phase 1: Add unit tests for SQL module with coverage. #239

Yury-Fridlyand · 2023-03-07T02:50:16Z

Description

Add unit tests for SQL module with coverage.
The only uncovered thing remains in SQLService.java is

opensearch-project-sql/sql/src/main/java/org/opensearch/sql/sql/SQLService.java

Lines 71 to 75 in 2019e84

    
           if (queryListener.isEmpty() && explainListener.isPresent()) { // explain request 
        
             // TODO explain should be processed inside the plan 
        
             explainListener.get().onFailure(new UnsupportedOperationException( 
        
                 "`explain` request for cursor requests is not supported. " 
        
                 + "Use `explain` for the initial query request."));

This entire block will be reworked soon

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --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.

Signed-off-by: Yury-Fridlyand <[email protected]>

codecov · 2023-03-07T03:03:53Z

Codecov Report

Merging #239 (35aa3b3) into dev-pagination-v2_revC (a3ef2bf) will increase coverage by 0.08%.
The diff coverage is 93.33%.

@@                     Coverage Diff                      @@
##             dev-pagination-v2_revC     #239      +/-   ##
============================================================
+ Coverage                     95.12%   95.21%   +0.08%     
- Complexity                     3700     3712      +12     
============================================================
  Files                           359      359              
  Lines                          9420     9423       +3     
  Branches                        612      614       +2     
============================================================
+ Hits                           8961     8972      +11     
+ Misses                          443      440       -3     
+ Partials                         16       11       -5

Flag	Coverage Δ
sql-engine	`95.21% <93.33%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...c/main/java/org/opensearch/sql/sql/SQLService.java	`92.30% <66.66%> (+9.69%)`	⬆️
...org/opensearch/sql/sql/domain/SQLQueryRequest.java	`100.00% <100.00%> (+17.64%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java

…t.java Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]>

GumpacG · 2023-03-08T00:16:25Z

sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java

  public boolean isSupported() {
-    return (isCursor() || isOnlySupportedFieldInPayload())
-        && isSupportedFormat();
+    var noCursor = !isCursor();


Should these be booleans instead of var?

var could be anything, even boolean as here. My favorite lazy developer feature.

GumpacG · 2023-03-08T00:19:23Z

sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java

-    assertTrue(request.isSupported());
-    assertEquals(request.format(), Format.JDBC);
+    assertAll(
+        () -> assertTrue(request.isSupported()),


Why change to this? Are they not the same?

The same, but assertAll always runs all assertions, regardless of failures.

* Fixing integration tests broken during POC Signed-off-by: MaxKsyunz <[email protected]> * Comment to clarify an exception. Signed-off-by: MaxKsyunz <[email protected]> * Add support for paginated scroll request, first page. Implement PaginatedPlanCache.convertToPlan for second page to work. Signed-off-by: MaxKsyunz <[email protected]> * Progress on paginated scroll request, subsequent page. Signed-off-by: MaxKsyunz <[email protected]> * Move `ExpressionSerializer` from `opensearch` to `core`. Signed-off-by: Yury-Fridlyand <[email protected]> * Rename `Cursor` `asString` to `toString`. Signed-off-by: Yury-Fridlyand <[email protected]> * Disable scroll cleaning. Signed-off-by: Yury-Fridlyand <[email protected]> * Add full cursor serialization and deserialization. Signed-off-by: Yury-Fridlyand <[email protected]> * Misc fixes. Signed-off-by: Yury-Fridlyand <[email protected]> * Further work on pagination. * Added push down page size from `LogicalPaginate` to `LogicalRelation`. * Improved cursor encoding and decoding. * Added cursor compression. * Fixed issuing `SearchScrollRequest`. * Fixed returning last empty page. * Minor code grooming/commenting. Signed-off-by: Yury-Fridlyand <[email protected]> * Pagination fix for empty indices. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix error reporting on wrong cursor. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor comments and error reporting improvement. Signed-off-by: Yury-Fridlyand <[email protected]> * Add an end-to-end integration test. Signed-off-by: Yury-Fridlyand <[email protected]> * Add `explain` request handlers. Signed-off-by: Yury-Fridlyand <[email protected]> * Add IT for explain. Signed-off-by: Yury-Fridlyand <[email protected]> * Address issues flagged by checkstyle build step (#229) Signed-off-by: MaxKsyunz <[email protected]> * Pagination, phase 1: Add unit tests for `:core` module with coverage. (#230) * Add unit tests for `:core` module with coverage. Uncovered: `toCursor`, because it is will be changed soon. Signed-off-by: Yury-Fridlyand <[email protected]> * Pagination, phase 1: Add unit tests for SQL module with coverage. (#239) * Add unit tests for SQL module with coverage. Signed-off-by: Yury-Fridlyand <[email protected]> * Update sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]> * Pagination, phase 1: Add unit tests for `:opensearch` module with coverage. (#233) * Add UT for `:opensearch` module with full coverage, except `toCursor`. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix checkstyle. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> * Fix the merges. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix explain. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix scroll cleaning. Signed-off-by: Yury-Fridlyand <[email protected]> * Store `TotalHits` and use it to report `total` in response. Signed-off-by: Yury-Fridlyand <[email protected]> * Add missing UT for `:protocol` module. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix PPL UTs damaged in f4ea4ad. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor checkstyle fixes. Signed-off-by: Yury-Fridlyand <[email protected]> * Fallback to v1 engine for pagination (#245) * Pagination fallback integration tests. Signed-off-by: MaxKsyunz <[email protected]> * Add UT with coverage for `toCursor` serialization. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix broken tests in `legacy`. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix getting `total` from non-paged requests and from queries without `FROM` clause. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix scroll cleaning. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix cursor request processing. Signed-off-by: Yury-Fridlyand <[email protected]> * Update ITs. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix (again) TotalHits feature. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix typo in prometheus config. Signed-off-by: Yury-Fridlyand <[email protected]> * Recover commented logging. Signed-off-by: Yury-Fridlyand <[email protected]> * Move `test_pagination_blackbox` to a separate class and add logging. Signed-off-by: Yury-Fridlyand <[email protected]> * Address some PR feedbacks: rename some classes and revert unnecessary whitespace changed. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor commenting. Signed-off-by: Yury-Fridlyand <[email protected]> * Address PR comments. * Add javadocs * Renames * Cleaning up some comments * Remove unused code * Speed up IT Signed-off-by: Yury-Fridlyand <[email protected]> * Minor missing changes. Signed-off-by: Yury-Fridlyand <[email protected]> * Integration tests for fetch_size, max_result_window, and query.size_limit (#248) Signed-off-by: MaxKsyunz <[email protected]> * Remove `PaginatedQueryService`, extend `QueryService` to hold two planners and use them. Signed-off-by: Yury-Fridlyand <[email protected]> * Move push down functions from request builders to a new interface. Signed-off-by: Yury-Fridlyand <[email protected]> * Some file moves. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor clean-up according to PR review. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: MaxKsyunz <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: MaxKsyunz <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]> Co-authored-by: Max Ksyunz <[email protected]>

* Support pagination in V2 engine, phase 1 (#226) * Fixing integration tests broken during POC Signed-off-by: MaxKsyunz <[email protected]> * Comment to clarify an exception. Signed-off-by: MaxKsyunz <[email protected]> * Add support for paginated scroll request, first page. Implement PaginatedPlanCache.convertToPlan for second page to work. Signed-off-by: MaxKsyunz <[email protected]> * Progress on paginated scroll request, subsequent page. Signed-off-by: MaxKsyunz <[email protected]> * Move `ExpressionSerializer` from `opensearch` to `core`. Signed-off-by: Yury-Fridlyand <[email protected]> * Rename `Cursor` `asString` to `toString`. Signed-off-by: Yury-Fridlyand <[email protected]> * Disable scroll cleaning. Signed-off-by: Yury-Fridlyand <[email protected]> * Add full cursor serialization and deserialization. Signed-off-by: Yury-Fridlyand <[email protected]> * Misc fixes. Signed-off-by: Yury-Fridlyand <[email protected]> * Further work on pagination. * Added push down page size from `LogicalPaginate` to `LogicalRelation`. * Improved cursor encoding and decoding. * Added cursor compression. * Fixed issuing `SearchScrollRequest`. * Fixed returning last empty page. * Minor code grooming/commenting. Signed-off-by: Yury-Fridlyand <[email protected]> * Pagination fix for empty indices. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix error reporting on wrong cursor. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor comments and error reporting improvement. Signed-off-by: Yury-Fridlyand <[email protected]> * Add an end-to-end integration test. Signed-off-by: Yury-Fridlyand <[email protected]> * Add `explain` request handlers. Signed-off-by: Yury-Fridlyand <[email protected]> * Add IT for explain. Signed-off-by: Yury-Fridlyand <[email protected]> * Address issues flagged by checkstyle build step (#229) Signed-off-by: MaxKsyunz <[email protected]> * Pagination, phase 1: Add unit tests for `:core` module with coverage. (#230) * Add unit tests for `:core` module with coverage. Uncovered: `toCursor`, because it is will be changed soon. Signed-off-by: Yury-Fridlyand <[email protected]> * Pagination, phase 1: Add unit tests for SQL module with coverage. (#239) * Add unit tests for SQL module with coverage. Signed-off-by: Yury-Fridlyand <[email protected]> * Update sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]> * Pagination, phase 1: Add unit tests for `:opensearch` module with coverage. (#233) * Add UT for `:opensearch` module with full coverage, except `toCursor`. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix checkstyle. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> * Fix the merges. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix explain. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix scroll cleaning. Signed-off-by: Yury-Fridlyand <[email protected]> * Store `TotalHits` and use it to report `total` in response. Signed-off-by: Yury-Fridlyand <[email protected]> * Add missing UT for `:protocol` module. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix PPL UTs damaged in f4ea4ad. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor checkstyle fixes. Signed-off-by: Yury-Fridlyand <[email protected]> * Fallback to v1 engine for pagination (#245) * Pagination fallback integration tests. Signed-off-by: MaxKsyunz <[email protected]> * Add UT with coverage for `toCursor` serialization. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix broken tests in `legacy`. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix getting `total` from non-paged requests and from queries without `FROM` clause. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix scroll cleaning. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix cursor request processing. Signed-off-by: Yury-Fridlyand <[email protected]> * Update ITs. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix (again) TotalHits feature. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix typo in prometheus config. Signed-off-by: Yury-Fridlyand <[email protected]> * Recover commented logging. Signed-off-by: Yury-Fridlyand <[email protected]> * Move `test_pagination_blackbox` to a separate class and add logging. Signed-off-by: Yury-Fridlyand <[email protected]> * Address some PR feedbacks: rename some classes and revert unnecessary whitespace changed. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor commenting. Signed-off-by: Yury-Fridlyand <[email protected]> * Address PR comments. * Add javadocs * Renames * Cleaning up some comments * Remove unused code * Speed up IT Signed-off-by: Yury-Fridlyand <[email protected]> * Minor missing changes. Signed-off-by: Yury-Fridlyand <[email protected]> * Integration tests for fetch_size, max_result_window, and query.size_limit (#248) Signed-off-by: MaxKsyunz <[email protected]> * Remove `PaginatedQueryService`, extend `QueryService` to hold two planners and use them. Signed-off-by: Yury-Fridlyand <[email protected]> * Move push down functions from request builders to a new interface. Signed-off-by: Yury-Fridlyand <[email protected]> * Some file moves. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor clean-up according to PR review. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: MaxKsyunz <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: MaxKsyunz <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]> Co-authored-by: Max Ksyunz <[email protected]> * Make scroll timeout configurable. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix IT to set cursor keep alive parameter. Signed-off-by: Yury-Fridlyand <[email protected]> * Remove `QueryId.None`. Signed-off-by: Yury-Fridlyand <[email protected]> * Rename according to PR feedback. Signed-off-by: Yury-Fridlyand <[email protected]> * Remove default implementations of `PushDownRequestBuilder`. Signed-off-by: Yury-Fridlyand <[email protected]> * Merge paginated plan optimizer into the regular optimizer. (opensearch-project#1516) Merge paginated plan optimizer into the regular optimizer. --------- Signed-off-by: MaxKsyunz <[email protected]> Co-authored-by: Yury-Fridlyand <[email protected]> * Complete rework on serialization and deserialization. (opensearch-project#1498) Signed-off-by: Yury-Fridlyand <[email protected]> * Resolve merge conflicts and fix tests. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor cleanup. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor cleanup - missing changes for the previous commit. Signed-off-by: Yury-Fridlyand <[email protected]> * Remove paginate operator (opensearch-project#1528) * Remove PaginateOperator class since it is no longer used. --------- Signed-off-by: MaxKsyunz <[email protected]> * Remove `PaginatedPlan` - move logic to `QueryPlan`. Signed-off-by: Yury-Fridlyand <[email protected]> * Remove default implementations from `SerializablePlan`. Signed-off-by: Yury-Fridlyand <[email protected]> * Add a doc. Signed-off-by: Yury-Fridlyand <[email protected]> * Update design graphs. Signed-off-by: Yury-Fridlyand <[email protected]> * More fixes for merge from upstream/main. Signed-off-by: MaxKsyunz <[email protected]> --------- Signed-off-by: MaxKsyunz <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: MaxKsyunz <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]> Co-authored-by: Max Ksyunz <[email protected]>

Add unit tests for SQL module with coverage.

2019e84

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand requested review from GabeFernandez310, GumpacG, MaxKsyunz, acarbonetto, forestmvey and matthewh-BQ March 7, 2023 02:50

GabeFernandez310 reviewed Mar 7, 2023

View reviewed changes

sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java Outdated Show resolved Hide resolved

GabeFernandez310 approved these changes Mar 7, 2023

View reviewed changes

Update sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryReques…

35aa3b3

…t.java Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: GabeFernandez310 <[email protected]>

Yury-Fridlyand changed the title ~~Add unit tests for SQL module with coverage.~~ Pagination, phase 1: Add unit tests for SQL module with coverage. Mar 7, 2023

GumpacG reviewed Mar 8, 2023

View reviewed changes

GumpacG approved these changes Mar 8, 2023

View reviewed changes

MaxKsyunz approved these changes Mar 9, 2023

View reviewed changes

GabeFernandez310 approved these changes Mar 9, 2023

View reviewed changes

Yury-Fridlyand merged commit 70ccfcb into dev-pagination-v2_revC Mar 10, 2023

Yury-Fridlyand deleted the dev-pagination-v2_revC-sql-UT-coverage branch March 10, 2023 02:41

Yury-Fridlyand mentioned this pull request Mar 10, 2023

Support pagination in V2 engine, phase 1 #226

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pagination, phase 1: Add unit tests for SQL module with coverage. #239

Pagination, phase 1: Add unit tests for SQL module with coverage. #239

Uh oh!

Yury-Fridlyand commented Mar 7, 2023

Uh oh!

codecov bot commented Mar 7, 2023 •

edited

Loading

Uh oh!

Uh oh!

GumpacG Mar 8, 2023

Uh oh!

Yury-Fridlyand Mar 8, 2023

Uh oh!

GumpacG Mar 8, 2023

Uh oh!

Yury-Fridlyand Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	if (queryListener.isEmpty() && explainListener.isPresent()) { // explain request
	// TODO explain should be processed inside the plan
	explainListener.get().onFailure(new UnsupportedOperationException(
	"`explain` request for cursor requests is not supported. "
	+ "Use `explain` for the initial query request."));

Pagination, phase 1: Add unit tests for SQL module with coverage. #239

Pagination, phase 1: Add unit tests for SQL module with coverage. #239

Uh oh!

Conversation

Yury-Fridlyand commented Mar 7, 2023

Description

Issues Resolved

Check List

Uh oh!

codecov bot commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

GumpacG Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

GumpacG Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Mar 7, 2023 •

edited

Loading