Extend query size limit using scroll #716

seankao-az · 2022-08-01T16:36:34Z

Description

#703 for non aggregation queries.

With this feature, we can

run source=index and get back query.size_limit results
run source=index | head x where x <= index.max_result_window and get back x results by regular OpenSearch query request
run source=index | head x where x > index.max_result_window and get back x results by scrolling, bypassing the OpenSearch restriction, essentially

This can be combined with other commands that post-processes the data rows fetched, e.g.

source=index | head 100000 | top <field>
source=index | head 100000 | <ml command>

Similar also works for SQL LIMIT operator.

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.

seankao-az · 2022-08-01T16:38:04Z

...search/src/main/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndex.java

+  public Integer getMaxResultWindow() {
+    // system index doesn't need this function
+    // the magic number is the number of fields of the mapping table
+    return 27;


Need a better way to do this

codecov-commenter · 2022-08-01T16:44:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.83%. Comparing base (792d3ff) to head (52ba001).
⚠️ Report is 1063 commits behind head on main.

❌ Your project status has failed because the head coverage (94.83%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #716      +/-   ##
============================================
- Coverage     97.76%   94.83%   -2.94%     
- Complexity     2880     2898      +18     
============================================
  Files           276      287      +11     
  Lines          7077     7802     +725     
  Branches        447      568     +121     
============================================
+ Hits           6919     7399     +480     
- Misses          157      349     +192     
- Partials          1       54      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`97.78% <100.00%> (+0.02%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

seankao-az · 2022-08-01T17:11:53Z

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java

    SearchSourceBuilder sourceBuilder = request.getSourceBuilder();
+    if (limit + offset > maxResultWindow) {
+      limit = maxResultWindow - offset;
+    }


This is for scrolling.
OpenSearch requires that from(offset) + size(limit) <= index.max_result_window. A limit operator with limit+offset > index.max_result_window will invoke scroll request multiple times, each with batch size index.max_result_window

the if condition limit + offset > maxResultWindow is used in different place, could we simply it?

penghuo · 2022-08-01T17:57:19Z

core/src/main/java/org/opensearch/sql/analysis/Analyzer.java

+    Integer limit = node.getLimit();
+    Integer offset = node.getOffset();
+    PlanContext planContext = context.getPlanContext();
+    if (limit + offset > planContext.getMaxResultWindow()) {
+      planContext.setIndexScanType(PlanContext.IndexScanType.SCROLL);
+    }


Try to reduce duplicate code. visitLimit and visitHead

seankao-az · 2022-08-01T17:57:39Z

opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScanTest.java

-                  when(response.isEmpty()).thenReturn(true);
                } else {
-                  fail("Search request after empty response returned already");
+                  when(response.isEmpty()).thenReturn(true);


Since we now fetch results in batches, additional search requests can be made when calling hasNext()

penghuo · 2022-08-01T18:03:06Z

opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java

  }

+  @Override
+  public Integer getIndexMaxResultWindow(String... indexExpression) {


add comments to explain the min(index...).

penghuo · 2022-08-01T18:06:02Z

opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchClient.java

   */
  Map<String, IndexMapping> getIndexMappings(String... indexExpression);

+  Integer getIndexMaxResultWindow(String... indexExpression);


penghuo · 2022-08-01T18:29:30Z

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java

-    this.request = new OpenSearchQueryRequest(indexName,
-        settings.getSettingValue(Settings.Key.QUERY_SIZE_LIMIT), exprValueFactory);
+    this.maxResultWindow = context.getMaxResultWindow();
+    switch (context.getIndexScanType()) {


i not sure how the PlanContext help in this case. seems we could build OpenSerachRequest when visit the logical plan tree.

penghuo · 2022-08-01T18:30:45Z

.../src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeLimitAndRelation.java


  @Override
-  public LogicalPlan apply(LogicalLimit plan, Captures captures) {
+  public LogicalPlan apply(LogicalLimit plan, Captures captures, PlanContext context) {


why keeping the Limit node?

Signed-off-by: Sean Kao <[email protected]>

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java

penghuo · 2022-08-03T17:32:56Z

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java


  @Override
  public boolean hasNext() {
+    if (isAggregation) {


aggregation should also support scroll, right?

yes. I'll work on it next. Aggregation works quite different from non-agg queries. If there's a lot to change I'll consider making a separate PR for it.

could open a issue if it is not covered in this PR?

Signed-off-by: Sean Kao <[email protected]>

penghuo · 2022-08-10T17:11:38Z

opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchRestClient.java

  }

+  @Override
+  public Map<String, Integer> getIndexMaxResultWindows(String... indexExpression) {


do we need mapping or just single maxResultWindow?

I approach this in a way similar to how getIndexMappings handles it.
For the client, getIndexMappings and getIndexMaxResultWindows return a Mapping from the index name to the corresponding result, without extra business logic.
In OpenSearchDescribeIndexRequest, getMaxResultWindow chooses the minimum of these values. This is specific to our need: if there's multiple indices, get the minimum max_result_window. Similarly, getFieldTypes unions the mappings of multiple indices here.

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

Signed-off-by: Sean Kao <[email protected]>

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

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

integ-test/src/test/java/org/opensearch/sql/ppl/HeadCommandIT.java

Signed-off-by: Sean Kao <[email protected]>

penghuo

LGTM

opensearch-trigger-bot · 2022-08-12T16:54:58Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-716-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f3c9c29538abdf93780e156edba3558fd43479da
# Push it to GitHub
git push --set-upstream origin backport/backport-716-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-716-to-2.x.

seankao-az commented Aug 1, 2022

View reviewed changes

penghuo reviewed Aug 1, 2022

View reviewed changes

seankao-az commented Aug 1, 2022

View reviewed changes

penghuo reviewed Aug 1, 2022

View reviewed changes

seankao31 added 14 commits August 1, 2022 14:54

add maxResultWindow to LogicalRelation

e2aac98

Signed-off-by: Sean Kao <[email protected]>

add maxResultWindow to OpenSearchLogicalIndexScan

289ff47

Signed-off-by: Sean Kao <[email protected]>

OpenSearchRequestBuilder init

f612a9e

Signed-off-by: Sean Kao <[email protected]>

request builder: push down and build

2201cc2

Signed-off-by: Sean Kao <[email protected]>

plan.build() for building request

a62b25d

Signed-off-by: Sean Kao <[email protected]>

maxResultWindow for test utils

e8abd9a

Signed-off-by: Sean Kao <[email protected]>

fix style

6691803

Signed-off-by: Sean Kao <[email protected]>

remove plan.build()

668b92b

Signed-off-by: Sean Kao <[email protected]>

fetch result in batches

fd477b0

Signed-off-by: Sean Kao <[email protected]>

get index.max_result_window settings

80d213c

Signed-off-by: Sean Kao <[email protected]>

use index.max_result_window to decide scroll

135c012

Signed-off-by: Sean Kao <[email protected]>

maxResultWindow for aggregation

4ec69f1

Signed-off-by: Sean Kao <[email protected]>

fix fetch size & for aggregation query

919ccb6

Signed-off-by: Sean Kao <[email protected]>

fix rest client get max result window

7b6b07f

Signed-off-by: Sean Kao <[email protected]>

penghuo reviewed Aug 3, 2022

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java Outdated Show resolved Hide resolved

penghuo reviewed Aug 3, 2022

View reviewed changes

seankao31 added 5 commits August 3, 2022 11:44

remove maxResultWindow from logical plan

b1bee34

Signed-off-by: Sean Kao <[email protected]>

get max result window when building physical plan

5b40987

Signed-off-by: Sean Kao <[email protected]>

move source builder init to request builder

4a585a9

Signed-off-by: Sean Kao <[email protected]>

fix max result window for test & rest client

edb2eb3

Signed-off-by: Sean Kao <[email protected]>

include request builder in equal comparison

aa4034d

Signed-off-by: Sean Kao <[email protected]>

seankao31 added 5 commits August 4, 2022 14:03

test index scan

17cf25c

Signed-off-by: Sean Kao <[email protected]>

integ test for head command

fd47234

Signed-off-by: Sean Kao <[email protected]>

keep request query size for aggregation

669d787

Signed-off-by: Sean Kao <[email protected]>

fix rest client test coverage

589a77b

Signed-off-by: Sean Kao <[email protected]>

fix highlight conflict

aa703b4

Signed-off-by: Sean Kao <[email protected]>

seankao-az marked this pull request as ready for review August 8, 2022 23:10

seankao-az requested a review from a team as a code owner August 8, 2022 23:10

seankao31 added 2 commits August 8, 2022 17:35

test for node client

8aed5cc

Signed-off-by: Sean Kao <[email protected]>

test node client default settings

1ace2de

Signed-off-by: Sean Kao <[email protected]>

seankao-az requested a review from penghuo August 9, 2022 21:14

penghuo reviewed Aug 10, 2022

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java Outdated Show resolved Hide resolved

change Elasticsearch to OpenSearch in comment

6ec8da0

Signed-off-by: Sean Kao <[email protected]>

penghuo reviewed Aug 10, 2022

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java Show resolved Hide resolved

penghuo reviewed Aug 10, 2022

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java Outdated Show resolved Hide resolved

penghuo reviewed Aug 10, 2022

View reviewed changes

integ-test/src/test/java/org/opensearch/sql/ppl/HeadCommandIT.java Show resolved Hide resolved

seankao31 added 3 commits August 10, 2022 13:56

fix comments

208921e

Signed-off-by: Sean Kao <[email protected]>

more test for Head IT

ff02665

Signed-off-by: Sean Kao <[email protected]>

ignore some head IT

52ba001

Signed-off-by: Sean Kao <[email protected]>

seankao-az requested a review from penghuo August 11, 2022 18:50

penghuo approved these changes Aug 12, 2022

View reviewed changes

penghuo added the backport 2.x label Aug 12, 2022

dai-chen approved these changes Aug 12, 2022

View reviewed changes

seankao-az merged commit f3c9c29 into opensearch-project:main Aug 12, 2022

dai-chen mentioned this pull request Aug 15, 2022

[BUG] GROUP BY Missing field returns error #743

Closed

Yury-Fridlyand mentioned this pull request Nov 1, 2022

Bump backport version. #1009

Merged

6 tasks

penghuo mentioned this pull request Nov 22, 2022

[BUG] Unknown index when querying data with fetch_size #947

Open

Yury-Fridlyand mentioned this pull request Jun 24, 2023

[BUG] aggregation works incorrectly with LIMIT #1774

Open

dai-chen mentioned this pull request Jul 3, 2024

[BUG] Inaccurate PPL query results due to plugins.query.size_limit restriction #2802

Closed

Extend query size limit using scroll #716

Extend query size limit using scroll #716

Uh oh!

Conversation

seankao-az commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 1, 2022 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

penghuo left a comment

Choose a reason for hiding this comment

Uh oh!

opensearch-trigger-bot bot commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seankao-az commented Aug 1, 2022 •

edited

Loading

codecov-commenter commented Aug 1, 2022 •

edited by codecov bot

Loading