Support Limit pushdown #3615

yuancu · 2025-05-12T09:38:22Z

Description

Implement LIMIT pushdown.

Related Issues

Resolves #3381

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: Yuanchun Shen <[email protected]>

...c/main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchLimitIndexScanRule.java

Signed-off-by: Yuanchun Shen <[email protected]>

...c/main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchLimitIndexScanRule.java

dai-chen · 2025-05-16T16:56:51Z

...c/main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchLimitIndexScanRule.java

+  @Override
+  public void onMatch(RelOptRuleCall call) {
+    final LogicalSort sort = call.rel(0);
+    final CalciteLogicalIndexScan scan = call.rel(1);


Does Calcite take care of the case that the child of sort is not index scan?

This rule only matches LogicalSort -> CalciteLogicalIndexScan, which is specified in OpenSearchLimitIndexScanRule.Config. When the child of sort is not index scan, the query will not be caught and pushed-down via this rule.

Do you mean "is it possible that a limit does not fall into the LogicalSort -> CalciteLogicalIndexScan pattern"?

I think the implementation here may have a bug:

source=t | where a=0 | head 10; source=t | head 10 | where a=0;

Above two PPLs will both push down the filter and limit to DSL. Please check are the DSLs generated same?
If they are same, the results are incorrect since the two PPL queries should have different behaviors.

Yes, the following two requests return the same DSL with the latest pushdown:

source=opensearch-sql_test_index_account | head 10 | where age = 20

source=opensearch-sql_test_index_account | where age = 20 | head 10

OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10,"timeout":"1m","query":{"term":{"age":{"value":20,"boost":1.0}}},"_source":{"includes":["account_number","firstname","address","balance","gender","city","employer","state","age","email","lastname"],"excludes":[]},"sort":[{"_doc":{"order":"asc"}}]}, requestedTotalSize=10, pageSize=null, startFrom=0)])

Besides, the same problem also exists in v2 -- when explaining the two queries, both of them return the same request:

OpenSearchQueryRequest(indexName=country, sourceBuilder={"from":0,"size":10,"timeout":"1m","query":{"term":{"age":{"value":20,"boost":1.0}}},"_source":{"includes":["name","country","state","month","year","age"],"excludes":[]},"sort":[{"_doc":{"order":"asc"}}]}, needClean=true, searchDone=false, pitId=null, cursorKeepAlive=null, searchAfter=null, searchResponse=null)

The DSL means sourceBuilder={...}. They are same. Please fix it by checking the size status in optimize rule

@LantaoJin Fixed

The size in the request builder is set to a default size at initialization (See OpenSearchIndex.java#L285). Therefore, we can not determine whether if there is already a limit by simply check whether size is set.

I created a flag in PushDownContext to log whether there was a limit pushdown.

Signed-off-by: Yuanchun Shen <[email protected]>

dai-chen

Thanks for the changes!

Signed-off-by: Yuanchun Shen <[email protected]>

LantaoJin · 2025-05-20T07:23:33Z

.../main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchFilterIndexScanRule.java

+                                        Predicate.not(OpenSearchIndexScanRule::isLimitPushed)
+                                            .and(OpenSearchIndexScanRule::test))


Would OpenSearchAggregateIndexScanRule be applied with the same fixing?

Yes, it should. The results are incorrect without it.

LantaoJin · 2025-05-20T07:25:36Z

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

+    // E.g. for `source=t | head 10 | head 5`, we take 5
+    // This also ensures that the limit won't exceed the initial default value. (set to
+    // Settings.Key.QUERY_SIZE_LIMIT in OpenSearchIndex)
+    requestedTotalSize = Math.min(limit, requestedTotalSize);
    startFrom = offset;
    sourceBuilder.from(offset).size(limit);


Yes, it was a mistake. Fixed.

It's not correct to set requestedTotalSize to the min here, I think.

source=t | head 10 from 1 | head 10 from 2 should return 8 rows, but it actually will return 10 rows under the current implementation.

LantaoJin · 2025-05-20T07:27:42Z

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

@@ -109,6 +109,36 @@ public void testLimitPushDownExplain() throws Exception {
                + "| fields ageMinus"));
  }

+  @Test
+  public void testLimitWithFilterPushdownExplain() throws Exception {
+    // TODO: Fix limit-then-filter pushdown without Calcite


how's the effort to fix the v2 bug together?

Fixed with minor effort.

Signed-off-by: Yuanchun Shen <[email protected]>

qianheng-aws · 2025-05-21T06:56:43Z

...c/main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchLimitIndexScanRule.java

+   * @param sort The LogicalSort to check.
+   * @return True if the LogicalSort is a LIMIT that can be pushed down, false otherwise.
+   */
+  private static boolean isLogicalSortLimit(LogicalSort sort) {


Could you move this condition to the RuleConfig, which will be more efficient?

Let's say RuleConfig find a LogicalSort node which is not limit, we should skip the step of identifying the pattern of its children and the above onMatch method immediately, instead of processing continuously until this place.

qianheng-aws · 2025-05-21T07:13:17Z

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

+    // E.g. for `source=t | head 10 | head 5`, we take 5
+    // This also ensures that the limit won't exceed the initial default value. (set to
+    // Settings.Key.QUERY_SIZE_LIMIT in OpenSearchIndex)
+    requestedTotalSize = Math.min(limit, requestedTotalSize);
    startFrom = offset;


I think it's not correct to override with the latest offset directly.

Let's say SQL like select * from (select * from table limit 10 offset 1) limit 5 offset 2, it's not correct to use 2 as the offset. The right offset should be 1 + 2 I think. Please add a test for this case.

You are right. Accumulating them should be the right behavior. Fixed.

Signed-off-by: Yuanchun Shen <[email protected]>

LantaoJin · 2025-05-22T02:17:54Z

...search/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java

+      if (LOG.isDebugEnabled()) {
+        LOG.debug("Cannot pushdown the limit {}", limit, e);
+      } else {
+        LOG.warn("Cannot pushdown the limit {}, ", limit);


Cannot pushdown the limit <Integer> is a meaningless message and warn level is too severe.

Changed the error message to LOG.debug("Cannot pushdown limit {} with offset {}. Exception: {}", limit, offset, e);

Checked with @qianheng-aws offline. Now I think the log won't be generated too many. So please added this else back and change it to info. Can you fix all the these warn logs in this class?

Signed-off-by: Yuanchun Shen <[email protected]>

LantaoJin · 2025-05-22T06:09:28Z

...c/main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchLimitIndexScanRule.java

+  private static Integer extractLimitValue(RexNode fetch) {
+    if (fetch instanceof RexLiteral) {
+      return ((RexLiteral) fetch).getValueAs(Integer.class);
+    }
+    return null;


would this fetch always a RexLiteral? how about head 1+1

Yes, as specified in OpenSearchPPLParser.g4#L149 and in OpenSearchSQLParser.g4#L144.

Signed-off-by: Yuanchun Shen <[email protected]>

yuancu 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, noCharger and qianheng-aws as code owners May 12, 2025 09:38

yuancu force-pushed the pushdown-limit branch 2 times, most recently from 0c14eae to 90b02e9 Compare May 12, 2025 09:42

Support Limit pushdown

43b5222

Signed-off-by: Yuanchun Shen <[email protected]>

yuancu force-pushed the pushdown-limit branch from 90b02e9 to 43b5222 Compare May 12, 2025 09:46

LantaoJin reviewed May 12, 2025

View reviewed changes

...c/main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchLimitIndexScanRule.java Show resolved Hide resolved

LantaoJin added the calcite calcite migration releated label May 12, 2025

yuancu added 2 commits May 12, 2025 22:05

Minor: add copyright header

f984d85

Signed-off-by: Yuanchun Shen <[email protected]>

Merge origin/main into pushdown-limit

8c92930

Signed-off-by: Yuanchun Shen <[email protected]>

yuancu force-pushed the pushdown-limit branch from 35f5ff3 to 8c92930 Compare May 14, 2025 06:54

dai-chen previously approved these changes May 16, 2025

View reviewed changes

...c/main/java/org/opensearch/sql/opensearch/planner/physical/OpenSearchLimitIndexScanRule.java Outdated Show resolved Hide resolved

dai-chen reviewed May 16, 2025

View reviewed changes

Refactor: create a isLogicalSortLimit method to enhance readability

bf19c6e

Signed-off-by: Yuanchun Shen <[email protected]>

yuancu dismissed dai-chen’s stale review via bf19c6e May 19, 2025 05:46

dai-chen previously approved these changes May 19, 2025

View reviewed changes

Disable limit-then-filter pushdown with Calcite

b1066f8

Signed-off-by: Yuanchun Shen <[email protected]>

yuancu dismissed dai-chen’s stale review via b1066f8 May 20, 2025 07:14

LantaoJin requested changes May 20, 2025

View reviewed changes

yuancu added 4 commits May 20, 2025 17:59

Disable limit-then-agg pushdown & add ITs for multiple head clauses

85af924

Signed-off-by: Yuanchun Shen <[email protected]>

Add limit-filter-limit pushdown IT

d5a9495

Signed-off-by: Yuanchun Shen <[email protected]>

Merge remote-tracking branch 'origin/main' into pushdown-limit

ed2d080

Disable limit-then-filter pushdown without Calcite

79c4303

Signed-off-by: Yuanchun Shen <[email protected]>

qianheng-aws reviewed May 21, 2025

View reviewed changes

yuancu added 2 commits May 21, 2025 18:12

Move predicates of limit pushdown ahead

fecea40

Signed-off-by: Yuanchun Shen <[email protected]>

Accumulate offsets when pushing down limit

eeae4e9

Signed-off-by: Yuanchun Shen <[email protected]>

LantaoJin reviewed May 22, 2025

View reviewed changes

Improve error message when failed to pushdown limit

08c4b06

Signed-off-by: Yuanchun Shen <[email protected]>

LantaoJin reviewed May 22, 2025

View reviewed changes

Update limit pushdown error message

51c1411

Signed-off-by: Yuanchun Shen <[email protected]>

LantaoJin previously approved these changes May 22, 2025

View reviewed changes

yuancu requested a review from qianheng-aws May 23, 2025 02:48

Merge origin/main into pushdown-limit

2100019

Signed-off-by: Yuanchun Shen <[email protected]>

yuancu dismissed LantaoJin’s stale review via 2100019 May 28, 2025 08:22

Fix multiple limit pushdown with shirinking caused by offset

998f552

Signed-off-by: Yuanchun Shen <[email protected]>

qianheng-aws approved these changes May 30, 2025

View reviewed changes

LantaoJin approved these changes May 30, 2025

View reviewed changes

LantaoJin merged commit df9f5dd into opensearch-project:main May 30, 2025
22 checks passed

		Predicate.not(OpenSearchIndexScanRule::isLimitPushed)
		.and(OpenSearchIndexScanRule::test))

Support Limit pushdown #3615

Support Limit pushdown #3615

Uh oh!

Conversation

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

Uh oh!

Description

Related Issues

Check List

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 May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dai-chen left a comment

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!

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

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!

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

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!

Uh oh!

yuancu commented May 12, 2025 •

edited by LantaoJin

Loading

LantaoJin May 20, 2025 •

edited

Loading

yuancu May 20, 2025 •

edited

Loading

LantaoJin May 20, 2025 •

edited

Loading

LantaoJin May 20, 2025 •

edited

Loading

qianheng-aws May 21, 2025 •

edited

Loading