Skip to content

Merge OpenSearchPagedIndexScan and OpenSearchIndexScan#1600

Merged
MaxKsyunz merged 40 commits intoopensearch-project:feature/pagination/integfrom
Bit-Quill:feature/pagination/integ_refactor
May 29, 2023
Merged

Merge OpenSearchPagedIndexScan and OpenSearchIndexScan#1600
MaxKsyunz merged 40 commits intoopensearch-project:feature/pagination/integfrom
Bit-Quill:feature/pagination/integ_refactor

Conversation

@MaxKsyunz
Copy link
Copy Markdown
Collaborator

@MaxKsyunz MaxKsyunz commented May 2, 2023 •
edited
Loading

High-level Summary

Core Module

  • Added PushDownPageSize optimization rule to LogicalPlanOptimizer.
  • Added TableScanBuilder.pushDownPageSize method.
  • Removed Table.createPagedScanBuilder.
  • Removed CreatePagingTableScanBuilder.

opensearch Module

  • Simplified OpenSearchIndexScanQueryBuilder and OpenSearchIndexScanAggregationBuilder by removing index scan dependency.
  • OpenSearchScrollRequest.toCursor does not generate a cursor if the request needs to be cleaned up -- means OpenSearch will not send any more data.
  • Added ExecutableRequestBuilder interface that represents operations of an executable request. Used by OpenSearchIndexScan.
  • Simplified ContinuePageRequest to only implement ExecutableRequestBuilder.
  • Removed
    • InitialPageRequestBuilder,
    • OpenSearchPagedIndexScan,
    • OpenSearchPagedIndexScanBuilder,
    • PagedRequestBuilder,
    • PushDownRequestBuilder

Issues Resolved

  • Review comments on pagination PR.

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • 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.

MaxKsyunz added 6 commits May 2, 2023 12:31
Remove OpenSearchPagedIndexScan and related classes.
68db706 
Unit tests pass, some integration tests fail.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Some bug fixes
ef93375 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Updating tests
306a7fe 
10000 is default query size set in `integ-test/build.gradle`

Previous request builder wasn't setting size correctly for explain requests so output didn't match what was executed.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Updating PaginationWindowIT
7d5b126 
Paginated response no longer sends an empty last page. Last page of the response now lacks cursor property. This matches legacy behaviour.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Addressing Code Analysis warnings
dc2c471 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Do not serialize OpenSearchScrollRequest when needClean is true.
91cea61 
When OpenSearchScrollRequest.needClean is true, the object represents the last request.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@MaxKsyunz MaxKsyunz force-pushed the feature/pagination/integ_refactor branch from 3a9c6e0 to 91cea61 Compare May 2, 2023 19:32
Fix checkstyle errors.
2ee810c 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 2, 2023 •
edited by codecov bot
Loading

Codecov Report

Merging #1600 (701bce7) into feature/pagination/integ (3614f88) will decrease coverage by 0.94%.
The diff coverage is 100.00%.

@@                      Coverage Diff                       @@
##             feature/pagination/integ    #1600      +/-   ##
==============================================================
- Coverage                       98.16%   97.22%   -0.94%     
- Complexity                       3306     4217     +911     
==============================================================
  Files                             287      381      +94     
  Lines                            8174    10596    +2422     
  Branches                          552      727     +175     
==============================================================
+ Hits                             8024    10302    +2278     
- Misses                            147      287     +140     
- Partials                            3        7       +4
Flag Coverage Δ
sql-engine 97.22% <100.00%> (-0.94%) ⬇️

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

Impacted Files Coverage Δ
...java/org/opensearch/sql/executor/QueryService.java 100.00% <ø> (ø)
...search/sql/executor/pagination/PlanSerializer.java 100.00% <ø> (ø)
...a/org/opensearch/sql/planner/SerializablePlan.java 100.00% <ø> (ø)
...ch/sql/planner/optimizer/LogicalPlanOptimizer.java 100.00% <ø> (ø)
...rc/main/java/org/opensearch/sql/storage/Table.java 100.00% <ø> (ø)
...ain/java/org/opensearch/sql/analysis/Analyzer.java 100.00% <100.00%> (ø)
...earch/sql/executor/execution/QueryPlanFactory.java 100.00% <100.00%> (ø)
...org/opensearch/sql/planner/DefaultImplementor.java 100.00% <100.00%> (ø)
...search/sql/planner/logical/LogicalFetchCursor.java 100.00% <100.00%> (ø)
...opensearch/sql/planner/logical/LogicalPlanDSL.java 100.00% <100.00%> (ø)
... and 16 more

... and 83 files with indirect coverage changes

MaxKsyunz added 2 commits May 3, 2023 22:26
Complete unit test coverage.
39ce902 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Code improvements
fabd4ee 
- Rename `getQuerySize` to `getMaxResponseSize` to reflect what
the value means.
- Create ExecutableRequestBuilder interface -- what OpenSearchIndexScan
requires from a request builder.
- Remove `PushDownRequestBuilder` interface as unnecessary

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@acarbonetto acarbonetto mentioned this pull request May 4, 2023
Closed
MaxKsyunz added 5 commits May 4, 2023 13:13
Checkstyle fixes
6eb0498 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Updating expected out in doctest
bdfe563 
Refactoring fixed a small bug in explain.

Previously, explain output did not show the correct `SearchSourceBuilder.size` value
. It now does and this affected some tests.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Refactoring
1f6b6f1 
- Inlined `OpenSearchIndexScan.create` that was only used in tests.
- Changed Function parameter of `OpenSearchIndexScanBuilder` to an
abstract method -- better describes intent.
- Renamed PushDownTranslator to PushDownQueryBuilder for better consistency.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Checkstyle fixes
a8295e4 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Rename createContinuePaginatedPlan in QueryPlanFactory to create
86429f8 
Old name doesn't quite make sense.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@MaxKsyunz MaxKsyunz marked this pull request as ready for review May 5, 2023 22:02
Yury-Fridlyand
Yury-Fridlyand reviewed May 26, 2023
View reviewed changes
core/src/main/java/org/opensearch/sql/planner/logical/LogicalFetchCursor.java

@EqualsAndHashCode(callSuper = false)
@ToString
public class LogicalFetchCursor extends LogicalPlan {
Copy link
Copy Markdown
Collaborator

@Yury-Fridlyand Yury-Fridlyand May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javadoc?

Max Ksyunz and others added 3 commits May 26, 2023 11:58
@Yury-Fridlyand
Update core/src/main/java/org/opensearch/sql/planner/logical/LogicalP…
4c9e958 
…lanDSL.java

Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Max Ksyunz <maxk@bitquilltech.com>
@Yury-Fridlyand
Update core/src/main/java/org/opensearch/sql/planner/logical/LogicalP…
102706b 
…lanNodeVisitor.java

Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Max Ksyunz <maxk@bitquilltech.com>
@Yury-Fridlyand
Update core/src/main/java/org/opensearch/sql/ast/tree/FetchCursor.java
2f4b48b 
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Max Ksyunz <maxk@bitquilltech.com>
Minor cleanup 2
33ad6dd 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
acarbonetto
acarbonetto approved these changes May 29, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please respond to Chen/Peng on this:
OpenSearchIndex.java

core/src/test/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java
relation, tableScanBuilder, write, tableWriteBuilder, filter, aggregation, rename, project,
remove, eval, sort, dedup, window, rareTopN, highlight, mlCommons, ad, ml, paginate, nested
remove, eval, sort, dedup, window, rareTopN, highlight, mlCommons, ad, ml, paginate, nested,
cursor
Copy link
Copy Markdown
Collaborator

@acarbonetto acarbonetto May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we should split this test up for each LogicalPlan node

Copy link
Copy Markdown
Collaborator

@Yury-Fridlyand Yury-Fridlyand May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged this recently to avoid duplicating code in tests...

core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java
assertEquals(
project(pagedTableScanBuilder),
LogicalPlanOptimizer.create().optimize(paginate(project(relation), 4)));
assertEquals(project(tableScanBuilder), optimized);
Copy link
Copy Markdown
Collaborator

@acarbonetto acarbonetto May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we should check pagesize of 4 explicitly
or is this done already?

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java
var builder = new OpenSearchRequestBuilder(
settings.getSettingValue(Settings.Key.QUERY_SIZE_LIMIT),
new OpenSearchExprValueFactory(allFields));
return new OpenSearchIndexScanBuilder(builder) {
Copy link
Copy Markdown
Collaborator

@acarbonetto acarbonetto May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaxKsyunz

MaxKsyunz added 2 commits May 29, 2023 12:39
Remove assertions that no longer apply
0abe298 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Minor cleanup
1dc3320 
Add comment and remove unused default constructor.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@Yury-Fridlyand
Copy link
Copy Markdown
Collaborator

Yury-Fridlyand commented May 29, 2023

Please, update PR description - for example, you don't have ExecutableRequestBuilder anymore.

Yury-Fridlyand
Yury-Fridlyand approved these changes May 29, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGFM
Last few minor things left.

Update test to account for prior changes
701bce7 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@MaxKsyunz MaxKsyunz merged commit 1c7233c into opensearch-project:feature/pagination/integ May 29, 2023
@penghuo penghuo mentioned this pull request May 30, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@penghuo penghuo penghuo left review comments

@dai-chen dai-chen dai-chen left review comments

@acarbonetto acarbonetto acarbonetto approved these changes

@pjfitzgibbons pjfitzgibbons Awaiting requested review from pjfitzgibbons

@ps48 ps48 Awaiting requested review from ps48

@kavithacm kavithacm Awaiting requested review from kavithacm

@derek-ho derek-ho Awaiting requested review from derek-ho

@joshuali925 joshuali925 Awaiting requested review from joshuali925

@YANG-DB YANG-DB Awaiting requested review from YANG-DB

@rupal-bq rupal-bq Awaiting requested review from rupal-bq

@mengweieric mengweieric Awaiting requested review from mengweieric

@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar

@Swiddis Swiddis Awaiting requested review from Swiddis

@seankao-az seankao-az Awaiting requested review from seankao-az

@anirudha anirudha Awaiting requested review from anirudha

@forestmvey forestmvey Awaiting requested review from forestmvey

@GumpacG GumpacG Awaiting requested review from GumpacG

+2 more reviewers

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes

@matthewryanwells matthewryanwells matthewryanwells approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

maintenance Improves code quality, but not the product pagination Pagination feature, ref #656

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@MaxKsyunz @codecov-commenter @Yury-Fridlyand @penghuo @dai-chen @acarbonetto @matthewryanwells