Merge OpenSearchPagedIndexScan and OpenSearchIndexScan #1600
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 6 commits
May 2, 2023 12:31
Unit tests pass, some integration tests fail. Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
When OpenSearchScrollRequest.needClean is true, the object represents the last request. Signed-off-by: MaxKsyunz <[email protected]>
MaxKsyunz
force-pushed
the
feature/pagination/integ_refactor
branch
from
3a9c6e0 to
91cea61
Compare
Signed-off-by: MaxKsyunz <[email protected]>
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
added 2 commits
May 3, 2023 22:26
Signed-off-by: MaxKsyunz <[email protected]>
- 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 <[email protected]>
added 5 commits
May 4, 2023 13:13
Signed-off-by: MaxKsyunz <[email protected]>
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 <[email protected]>
- 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 <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
Old name doesn't quite make sense. Signed-off-by: MaxKsyunz <[email protected]>
MaxKsyunz
requested review from
Swiddis,
YANG-DB,
dai-chen,
derek-ho,
joshuali925,
kavithacm,
mengweieric,
penghuo,
pjfitzgibbons,
ps48,
rupal-bq and
vamsimanohar
as code owners
core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/planner/logical/LogicalFetchCursor.java
Outdated
Show resolved
Hide resolved
|
|
||
| @EqualsAndHashCode(callSuper = false) | ||
| @ToString | ||
| public class LogicalFetchCursor extends LogicalPlan { |
core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanDSL.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizer.java
Show resolved
Hide resolved
…lanDSL.java Co-authored-by: Yury-Fridlyand <[email protected]> Signed-off-by: Max Ksyunz <[email protected]>
…lanNodeVisitor.java Co-authored-by: Yury-Fridlyand <[email protected]> Signed-off-by: Max Ksyunz <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]> Signed-off-by: Max Ksyunz <[email protected]>
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchScrollRequest.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchScrollRequest.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchScrollRequest.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java
Show resolved
Hide resolved
...n/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanAggregationBuilder.java
Show resolved
Hide resolved
plugin/src/main/java/org/opensearch/sql/plugin/config/OpenSearchPluginModule.java
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/planner/ExternalizablePlanTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java
Show resolved
Hide resolved
Signed-off-by: MaxKsyunz <[email protected]>
acarbonetto
approved these changes
May 29, 2023
Collaborator
acarbonetto
left a comment
acarbonetto
left a comment
There was a problem hiding this comment.
Please respond to Chen/Peng on this:
OpenSearchIndex.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 |
Collaborator
There was a problem hiding this comment.
nit: we should split this test up for each LogicalPlan node
Collaborator
There was a problem hiding this comment.
I merged this recently to avoid duplicating code in tests...
| assertEquals( | ||
| project(pagedTableScanBuilder), | ||
| LogicalPlanOptimizer.create().optimize(paginate(project(relation), 4))); | ||
| assertEquals(project(tableScanBuilder), optimized); |
Collaborator
There was a problem hiding this comment.
nit: we should check pagesize of 4 explicitly
or is this done already?
| var builder = new OpenSearchRequestBuilder( | ||
| settings.getSettingValue(Settings.Key.QUERY_SIZE_LIMIT), | ||
| new OpenSearchExprValueFactory(allFields)); | ||
| return new OpenSearchIndexScanBuilder(builder) { |
added 2 commits
May 29, 2023 12:39
Signed-off-by: MaxKsyunz <[email protected]>
Add comment and remove unused default constructor. Signed-off-by: MaxKsyunz <[email protected]>
Collaborator
|
Please, update PR description - for example, you don't have |
Yury-Fridlyand
approved these changes
May 29, 2023
Collaborator
Yury-Fridlyand
left a comment
Yury-Fridlyand
left a comment
There was a problem hiding this comment.
LGFM
Last few minor things left.
Signed-off-by: MaxKsyunz <[email protected]>
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 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.
High-level Summary
Core Module
PushDownPageSizeoptimization rule toLogicalPlanOptimizer.TableScanBuilder.pushDownPageSizemethod.Table.createPagedScanBuilder.CreatePagingTableScanBuilder.opensearch Module
OpenSearchIndexScanQueryBuilderandOpenSearchIndexScanAggregationBuilderby removing index scan dependency.OpenSearchScrollRequest.toCursordoes not generate a cursor if the request needs to be cleaned up -- means OpenSearch will not send any more data.ExecutableRequestBuilderinterface that represents operations of an executable request. Used byOpenSearchIndexScan.ContinuePageRequestto only implementExecutableRequestBuilder.InitialPageRequestBuilder,OpenSearchPagedIndexScan,OpenSearchPagedIndexScanBuilder,PagedRequestBuilder,PushDownRequestBuilderIssues Resolved
Check List
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.