Make query.size_limit only affect the final results #3623

qianheng-aws · 2025-05-14T10:17:34Z

Description

Make query.size_limit only affect the final results

Related Issues

Resolves
#3595,
#703,
#3607,
#3637,
#3638

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.

Signed-off-by: Heng Qian <[email protected]>

penghuo · 2025-05-14T20:54:27Z

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

-        settings.getSettingValue(Settings.Key.QUERY_SIZE_LIMIT),
-        this.createExprValueFactory(),
-        settings);
+    return new OpenSearchRequestBuilder(getMaxResultWindow(), createExprValueFactory(), settings);


OpenSearchRequestBuilder.build() and buildRequestWithPit() already has maxResultWindow as paramater, do we still need in constructor? or consider remove it from build and buildRequestWithPit?

There's another bug around #3607. Seems can be fixed in this PR.

OpenSearchRequestBuilder.build() and buildRequestWithPit() already has maxResultWindow as paramater, do we still need in constructor? or consider remove it from build and buildRequestWithPit?

Both is required in the API, and they play as different roles.

The parameter in constructor is stored as field requestedTotalSize which decides how many rows will be returned, and it will be updated by the limit value in LIMIT(limit, offset). In initialization, we expect requestedTotalSize should be set by maxResultWindow.

The parameter in build or buildRequestWithPit is actually the real place used to restrict the max window of index.
So it has logic size = (startFrom + requestedTotalSize > maxResultWindow) ? maxResultWindow - startFrom) : requestedTotalSize.

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

Line 111 in 1329e02

if (startFrom + size > maxResultWindow) {

Correct it, after looking into the issue #3607. Seems we should never pass in that parameter in the constructor to initialize requestedTotalSize. Without limit pushdown, we actually never know how many rows should be returned. It should be initialized with -1, which means no restriction on the total size.

core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java

Signed-off-by: Heng Qian <[email protected]>

…imit # Conflicts: # integ-test/src/test/resources/expectedOutput/calcite/explain_filter_agg_push.json # integ-test/src/test/resources/expectedOutput/calcite/explain_output.json

Signed-off-by: Heng Qian <[email protected]>

…imit # Conflicts: # opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java

Signed-off-by: Heng Qian <[email protected]>

LantaoJin · 2025-05-22T02:21:39Z

core/src/main/java/org/opensearch/sql/executor/ExecutionContext.java

  @Getter private final Optional<Split> split;
+  private final Integer querySizeLimit;
+
+  public Integer getQuerySizeLimit() {


how about using @Getter private final Optional<Integer> querySizeLimit; to keep style consistency

LantaoJin · 2025-05-22T02:28:37Z

docs/user/optimization/optimization.rst

@@ -280,7 +280,7 @@ The Aggregation operator will merge into OpenSearch Aggregation::
          {
            "name": "OpenSearchIndexScan",
            "description": {
-              "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)"
+              "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)"


question: does "size":0 mean fetching without limitation?

For aggregation, size is meaningless here, so I don't set a new value to the size of sourceBuilder, then it keeps using the value when pushing down aggregation, which is 0.

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

Line 178 in 730f066

sourceBuilder.size(0);

size define how many search hits will be returned alone with aggregation. size = 0 is fine.

@LantaoJin set size=0, also solve #3528?

@penghuo no, the benchmark was already set size=0. The performance issue is not related to this. But yes for a general improving for aggregation.

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

integ-test/src/test/java/org/opensearch/sql/sql/PaginationIT.java

integ-test/src/test/java/org/opensearch/sql/util/StandaloneModule.java

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

.../java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java

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

Signed-off-by: Heng Qian <[email protected]>

penghuo · 2025-05-27T17:04:33Z

docs/user/optimization/optimization.rst

@@ -280,7 +280,7 @@ The Aggregation operator will merge into OpenSearch Aggregation::
          {
            "name": "OpenSearchIndexScan",
            "description": {
-              "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)"
+              "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)"


size define how many search hits will be returned alone with aggregation. size = 0 is fine.

penghuo · 2025-05-27T17:06:31Z

docs/user/optimization/optimization.rst

@@ -280,7 +280,7 @@ The Aggregation operator will merge into OpenSearch Aggregation::
          {
            "name": "OpenSearchIndexScan",
            "description": {
-              "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)"
+              "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)"


@LantaoJin set size=0, also solve #3528?

LantaoJin · 2025-05-28T01:30:11Z

@qianheng-aws please update the doc https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/admin/settings.rst#plugins-query-size-limit

The size configure the maximum amount of documents to be pull from OpenSearch. The default value is: 10000

Notes: This setting will impact the correctness of the aggregation operation, for example, there are 1000 docs in the index, if you change the value to 200, only 200 docs will be extract from index and do aggregation.

Sounds out of date.

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws · 2025-05-28T02:40:43Z

@qianheng-aws please update the doc https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/admin/settings.rst#plugins-query-size-limit

The size configure the maximum amount of documents to be pull from OpenSearch. The default value is: 10000
Notes: This setting will impact the correctness of the aggregation operation, for example, there are 1000 docs in the index, if you change the value to 200, only 200 docs will be extract from index and do aggregation.

Sounds out of date.

done

…imit

Signed-off-by: Heng Qian <[email protected]>

LantaoJin · 2025-05-28T05:04:55Z

integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3655.yml

-          query: "source=test | where like(body, '%field 2%') | fields body"
-  - match: { root.children.0.description.request: "OpenSearchQueryRequest(indexName=test, sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"query\":{\"wildcard\":{\"body\":{\"wildcard\":\"*field 2*\",\"case_insensitive\":true,\"boost\":1.0}}},\"_source\":{\"includes\":[\"body\"],\"excludes\":[]},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}]}, needClean=true, searchDone=false, pitId=null, cursorKeepAlive=null, searchAfter=null, searchResponse=null)"}
+# ---
+# TODO: pitId is dynamic, try to match the request body more precisely by using regex match


could you open an issue for this temporary workaround?

#3692, tracked here

Make query.size_limit only affect the final results

999d6a2

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, mengweieric, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224, LantaoJin and noCharger as code owners May 14, 2025 10:17

penghuo reviewed May 14, 2025

View reviewed changes

qianheng-aws added 2 commits May 16, 2025 15:18

Fix 3607

e15c177

Signed-off-by: Heng Qian <[email protected]>

Fix IT/UT

8756d23

Signed-off-by: Heng Qian <[email protected]>

This was referenced May 20, 2025

[BUG] PIT search response missing some metadata information like _score, _maxscore #3637

Closed

[BUG] Incorrect results when having NestedLoopJoin with PIT search #3638

Closed

qianheng-aws added 5 commits May 20, 2025 16:36

Fix issue 3638

adebf68

Signed-off-by: Heng Qian <[email protected]>

Fix issue 3637

5481842

Signed-off-by: Heng Qian <[email protected]>

Merge remote-tracking branch 'refs/remotes/origin/main' into fixSizeL…

dc3cc2d

…imit # Conflicts: # integ-test/src/test/resources/expectedOutput/calcite/explain_filter_agg_push.json # integ-test/src/test/resources/expectedOutput/calcite/explain_output.json

Fix IT

44c9ca0

Signed-off-by: Heng Qian <[email protected]>

Fix IT after merging main

866deb2

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws added 4 commits May 20, 2025 23:41

Fix UT

9f924fa

Signed-off-by: Heng Qian <[email protected]>

Fix doctest

ad223de

Signed-off-by: Heng Qian <[email protected]>

Merge remote-tracking branch 'refs/remotes/origin/main' into fixSizeL…

b160833

…imit # Conflicts: # opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java

Fix IT after merging main

bb26676

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws requested a review from penghuo May 21, 2025 08:23

LantaoJin reviewed May 22, 2025

View reviewed changes

qianheng-aws added 3 commits May 22, 2025 11:59

Address comments

7acc219

Signed-off-by: Heng Qian <[email protected]>

Fix CI

5af0a29

Signed-off-by: Heng Qian <[email protected]>

Fix doctest

4bc5616

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws requested a review from LantaoJin May 22, 2025 08:20

LantaoJin previously approved these changes May 27, 2025

View reviewed changes

penghuo previously approved these changes May 27, 2025

View reviewed changes

Update settings doc

48274ec

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws dismissed stale reviews from penghuo and LantaoJin via 48274ec May 28, 2025 02:40

qianheng-aws requested a review from LantaoJin May 28, 2025 02:40

qianheng-aws added 2 commits May 28, 2025 11:39

Merge remote-tracking branch 'refs/remotes/origin/main' into fixSizeL…

17ea73f

…imit

Ignore the explain test in 3655.yml

8a35539

Signed-off-by: Heng Qian <[email protected]>

LantaoJin approved these changes May 28, 2025

View reviewed changes

penghuo approved these changes May 28, 2025

View reviewed changes

qianheng-aws merged commit 66e230e into opensearch-project:main May 28, 2025
22 checks passed

Make query.size_limit only affect the final results #3623

Make query.size_limit only affect the final results #3623

Conversation

qianheng-aws commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qianheng-aws commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

qianheng-aws commented May 14, 2025 •

edited

Loading

LantaoJin commented May 28, 2025 •

edited

Loading