Skip to content

Commit 18f2280

Browse files
authored
Prevent limit pushdown before action building instead of in action executing (opensearch-project#4377)
* The limit pushdown check logic should be in action building instead of waiting until action is executed Signed-off-by: Lantao Jin <ltjin@amazon.com> * Add an explain IT Signed-off-by: Lantao Jin <ltjin@amazon.com> --------- Signed-off-by: Lantao Jin <ltjin@amazon.com>
1 parent 37197e5 commit 18f2280

5 files changed

Lines changed: 44 additions & 0 deletions

File tree

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,17 @@ public void testMvjoinExplain() throws IOException {
596596
assertJsonEqualsIgnoreId(expected, result);
597597
}
598598

599+
@Test
600+
public void testPreventLimitPushdown() throws IOException {
601+
enabledOnlyWhenPushdownIsEnabled();
602+
setMaxResultWindow("opensearch-sql_test_index_account", 1);
603+
String query = "source=opensearch-sql_test_index_account | head 1 from 1";
604+
var result = explainQueryToString(query);
605+
String expected = loadExpectedPlan("explain_prevent_limit_push.yaml");
606+
assertYamlEqualsJsonIgnoreId(expected, result);
607+
resetMaxResultWindow("opensearch-sql_test_index_account");
608+
}
609+
599610
@Test
600611
public void testPushdownLimitIntoAggregation() throws IOException {
601612
enabledOnlyWhenPushdownIsEnabled();
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
calcite:
2+
logical: |
3+
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
4+
LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])
5+
LogicalSort(offset=[1], fetch=[1])
6+
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
7+
physical: |
8+
EnumerableLimit(fetch=[10000])
9+
EnumerableLimit(offset=[1], fetch=[1])
10+
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname]], OpenSearchRequestBuilder(sourceBuilder={"from":0,"timeout":"1m","_source":{"includes":["account_number","firstname","address","balance","gender","city","employer","state","age","email","lastname"],"excludes":[]}}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)])

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@ setup:
33
features:
44
- headers
55
- allowed_warnings
6+
- do:
7+
query.settings:
8+
body:
9+
transient:
10+
plugins.calcite.enabled: true
611
- do:
712
indices.create:
813
index: test
@@ -21,6 +26,14 @@ setup:
2126
- '{"index": {}}'
2227
- '{"id": 3}'
2328

29+
---
30+
teardown:
31+
- do:
32+
query.settings:
33+
body:
34+
transient:
35+
plugins.calcite.enabled : false
36+
2437
---
2538
"Prevent push down limit if the offset reach max_result_window":
2639
- do:

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ public static class PushDownContext extends ArrayDeque<PushDownAction> {
170170
@Getter private AggPushDownAction aggPushDownAction;
171171
@Getter private boolean isLimitPushed = false;
172172
@Getter private boolean isProjectPushed = false;
173+
@Getter private int startFrom = 0;
173174

174175
@Override
175176
public PushDownContext clone() {
@@ -184,6 +185,7 @@ public boolean add(PushDownAction pushDownAction) {
184185
}
185186
if (pushDownAction.type == PushDownType.LIMIT) {
186187
isLimitPushed = true;
188+
startFrom += ((LimitDigest) pushDownAction.digest).offset();
187189
}
188190
if (pushDownAction.type == PushDownType.PROJECT) {
189191
isProjectPushed = true;

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import org.opensearch.sql.opensearch.planner.physical.EnumerableIndexScanRule;
5050
import org.opensearch.sql.opensearch.planner.physical.OpenSearchIndexRules;
5151
import org.opensearch.sql.opensearch.request.AggregateAnalyzer;
52+
import org.opensearch.sql.opensearch.request.OpenSearchRequestBuilder;
5253
import org.opensearch.sql.opensearch.request.PredicateAnalyzer;
5354
import org.opensearch.sql.opensearch.request.PredicateAnalyzer.QueryExpression;
5455
import org.opensearch.sql.opensearch.response.agg.OpenSearchAggregationResponseParser;
@@ -348,6 +349,13 @@ public AbstractRelNode pushDownLimit(LogicalSort sort, Integer limit, Integer of
348349
return offset > 0 ? sort.copy(sort.getTraitSet(), List.of(newScan)) : newScan;
349350
} else {
350351
CalciteLogicalIndexScan newScan = this.copyWithNewSchema(getRowType());
352+
int newStartFrom = newScan.pushDownContext.getStartFrom() + offset;
353+
if (newStartFrom >= newScan.osIndex.getMaxResultWindow()) {
354+
throw new OpenSearchRequestBuilder.PushDownUnSupportedException(
355+
String.format(
356+
"Requested offset %d should be less than the max result window %d",
357+
newStartFrom, newScan.osIndex.getMaxResultWindow()));
358+
}
351359
newScan.pushDownContext.add(
352360
PushDownAction.of(
353361
PushDownType.LIMIT,

0 commit comments

Comments
 (0)