From 2fca24a30b5e135185e342bdc64313053d52b3b6 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Fri, 30 May 2025 15:14:57 +0800 Subject: [PATCH 1/2] [BugFix] Prevent push down limit with offset no less than maxResultWindow Signed-off-by: Heng Qian --- .../rest-api-spec/test/issues/3102.yml | 53 +++++++++++++++++++ .../request/OpenSearchRequestBuilder.java | 41 +++++++++----- .../opensearch/storage/OpenSearchIndex.java | 14 ++--- .../scan/OpenSearchIndexScanQueryBuilder.java | 18 ++++++- .../OpenSearchExecutionEngineTest.java | 6 +-- .../OpenSearchExecutionProtectorTest.java | 8 +-- .../request/OpenSearchRequestBuilderTest.java | 46 +++++++++------- .../storage/OpenSearchIndexTest.java | 18 +++---- .../OpenSearchIndexScanPaginationTest.java | 11 ++-- .../storage/scan/OpenSearchIndexScanTest.java | 39 +++++++------- 10 files changed, 164 insertions(+), 90 deletions(-) create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml new file mode 100644 index 00000000000..63a3f31a375 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml @@ -0,0 +1,53 @@ +setup: + - skip: + features: + - headers + - do: + indices.create: + index: test + body: + settings: + max_result_window: 1 + - do: + bulk: + index: test + refresh: true + body: + - '{"index": {}}' + - '{"id": 1}' + - '{"index": {}}' + - '{"id": 2}' + - '{"index": {}}' + - '{"id": 3}' + + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : false + plugins.calcite.fallback.allowed : true + +--- +"Prevent push down limit if the offset exceeds max_result_window": + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: 'source=test | head 1 from 1 ' + - match: {"total": 1} + - match: {"schema": [{"name": "id", "type": "bigint"}]} + - match: {"datarows": [[2]]} + + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: 'source=test | head 2 | head 1 from 1 ' + - match: { "total": 1 } + - match: { "schema": [ { "name": "id", "type": "bigint" } ] } + - match: { "datarows": [ [ 2 ] ] } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java index b65ad21be3a..302751277ce 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java @@ -68,13 +68,23 @@ public class OpenSearchRequestBuilder { @EqualsAndHashCode.Exclude @ToString.Exclude private final OpenSearchExprValueFactory exprValueFactory; + @EqualsAndHashCode.Exclude @ToString.Exclude private final int maxResultWindow; + private int startFrom = 0; @ToString.Exclude private final Settings settings; + public static class PushDownUnSupportedException extends RuntimeException { + public PushDownUnSupportedException(String message) { + super(message); + } + } + /** Constructor. */ - public OpenSearchRequestBuilder(OpenSearchExprValueFactory exprValueFactory, Settings settings) { + public OpenSearchRequestBuilder( + OpenSearchExprValueFactory exprValueFactory, int maxResultWindow, Settings settings) { this.settings = settings; + this.maxResultWindow = maxResultWindow; this.sourceBuilder = new SearchSourceBuilder() .from(startFrom) @@ -89,16 +99,12 @@ public OpenSearchRequestBuilder(OpenSearchExprValueFactory exprValueFactory, Set * @return query request with PIT or scroll request */ public OpenSearchRequest build( - OpenSearchRequest.IndexName indexName, - int maxResultWindow, - TimeValue cursorKeepAlive, - OpenSearchClient client) { - return build(indexName, maxResultWindow, cursorKeepAlive, client, false); + OpenSearchRequest.IndexName indexName, TimeValue cursorKeepAlive, OpenSearchClient client) { + return build(indexName, cursorKeepAlive, client, false); } public OpenSearchRequest build( OpenSearchRequest.IndexName indexName, - int maxResultWindow, TimeValue cursorKeepAlive, OpenSearchClient client, boolean isMappingEmpty) { @@ -109,14 +115,11 @@ public OpenSearchRequest build( if (sourceBuilder.size() == 0 || isMappingEmpty) { return new OpenSearchQueryRequest(indexName, sourceBuilder, exprValueFactory, List.of()); } - return buildRequestWithPit(indexName, maxResultWindow, cursorKeepAlive, client); + return buildRequestWithPit(indexName, cursorKeepAlive, client); } private OpenSearchRequest buildRequestWithPit( - OpenSearchRequest.IndexName indexName, - int maxResultWindow, - TimeValue cursorKeepAlive, - OpenSearchClient client) { + OpenSearchRequest.IndexName indexName, TimeValue cursorKeepAlive, OpenSearchClient client) { int size = requestedTotalSize; FetchSourceContext fetchSource = this.sourceBuilder.fetchSource(); List includes = fetchSource != null ? Arrays.asList(fetchSource.includes()) : List.of(); @@ -218,10 +221,20 @@ public void pushDownLimit(Integer limit, Integer offset) { // Besides, there may be cases when the existing requestedTotalSize does not satisfy the // new limit and offset. E.g. for `head 11 | head 10 from 2`, the new requested total size // is 9. We need to adjust it accordingly. - requestedTotalSize = Math.min(limit, requestedTotalSize - offset); + int newRequestedTotalSize = Math.min(limit, requestedTotalSize - offset); // If there are multiple offset, we aggregate the offset // E.g. for `head 10 from 1 | head 5 from 2` equals to `head 5 from 3` - startFrom += offset; + int newStartFrom = startFrom + offset; + + if (newStartFrom >= maxResultWindow) { + throw new PushDownUnSupportedException( + String.format( + "Requested offset %d should be less than the max result window %d", + newStartFrom, maxResultWindow)); + } + + requestedTotalSize = newRequestedTotalSize; + startFrom = newStartFrom; sourceBuilder.from(startFrom).size(requestedTotalSize); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index 276dc893918..df5d7cef3e3 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -205,11 +205,7 @@ public TableScanBuilder createScanBuilder() { client, requestBuilder.getMaxResponseSize(), requestBuilder.build( - indexName, - getMaxResultWindow(), - cursorKeepAlive, - client, - cachedFieldOpenSearchTypes.isEmpty())); + indexName, cursorKeepAlive, client, cachedFieldOpenSearchTypes.isEmpty())); return new OpenSearchIndexScanBuilder(builder, createScanOperator); } @@ -258,16 +254,12 @@ public PhysicalPlan visitEval(LogicalEval node, OpenSearchIndexScan context) { } public OpenSearchRequestBuilder createRequestBuilder() { - return new OpenSearchRequestBuilder(createExprValueFactory(), settings); + return new OpenSearchRequestBuilder(createExprValueFactory(), getMaxResultWindow(), settings); } public OpenSearchRequest buildRequest(OpenSearchRequestBuilder requestBuilder) { final TimeValue cursorKeepAlive = settings.getSettingValue(Settings.Key.SQL_CURSOR_KEEP_ALIVE); return requestBuilder.build( - indexName, - getMaxResultWindow(), - cursorKeepAlive, - client, - cachedFieldOpenSearchTypes.isEmpty()); + indexName, cursorKeepAlive, client, cachedFieldOpenSearchTypes.isEmpty()); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanQueryBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanQueryBuilder.java index 6aba39c7c72..3976b7efc4e 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanQueryBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanQueryBuilder.java @@ -13,6 +13,8 @@ import java.util.stream.Collectors; import lombok.EqualsAndHashCode; import org.apache.commons.lang3.tuple.Pair; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.opensearch.index.query.QueryBuilder; import org.opensearch.sql.ast.tree.Sort; import org.opensearch.sql.common.utils.StringUtils; @@ -23,6 +25,7 @@ import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.expression.function.OpenSearchFunctions; import org.opensearch.sql.opensearch.request.OpenSearchRequestBuilder; +import org.opensearch.sql.opensearch.request.OpenSearchRequestBuilder.PushDownUnSupportedException; import org.opensearch.sql.opensearch.storage.script.filter.FilterQueryBuilder; import org.opensearch.sql.opensearch.storage.script.sort.SortQueryBuilder; import org.opensearch.sql.opensearch.storage.serialization.DefaultExpressionSerializer; @@ -41,6 +44,7 @@ @VisibleForTesting @EqualsAndHashCode class OpenSearchIndexScanQueryBuilder implements PushDownQueryBuilder { + private static final Logger LOG = LogManager.getLogger(OpenSearchIndexScanQueryBuilder.class); final OpenSearchRequestBuilder requestBuilder; @@ -71,8 +75,18 @@ public boolean pushDownSort(LogicalSort sort) { @Override public boolean pushDownLimit(LogicalLimit limit) { - requestBuilder.pushDownLimit(limit.getLimit(), limit.getOffset()); - return true; + try { + requestBuilder.pushDownLimit(limit.getLimit(), limit.getOffset()); + return true; + } catch (PushDownUnSupportedException e) { + if (LOG.isDebugEnabled()) { + LOG.debug( + "Cannot pushdown limit {} with offset {}", limit.getLimit(), limit.getOffset(), e); + } else { + LOG.info("Cannot pushdown limit {} with offset {}", limit.getLimit(), limit.getOffset()); + } + return false; + } } @Override diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java index 858a80b6803..01d61288173 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java @@ -178,12 +178,12 @@ void explain_successfully() { OpenSearchExprValueFactory exprValueFactory = mock(OpenSearchExprValueFactory.class); final var name = new OpenSearchRequest.IndexName("test"); final int maxResultWindow = 10000; - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = + new OpenSearchRequestBuilder(exprValueFactory, maxResultWindow, settings); PhysicalPlan plan = new OpenSearchIndexScan( mock(OpenSearchClient.class), - requestBuilder.build( - name, maxResultWindow, settings.getSettingValue(SQL_CURSOR_KEEP_ALIVE), client)); + requestBuilder.build(name, settings.getSettingValue(SQL_CURSOR_KEEP_ALIVE), client)); AtomicReference result = new AtomicReference<>(); executor.explain( diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java index 35d4b1e6d78..5bddb223f97 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java @@ -119,12 +119,8 @@ void test_protect_indexScan() { final var name = new OpenSearchRequest.IndexName(indexName); final var request = - new OpenSearchRequestBuilder(exprValueFactory, settings) - .build( - name, - maxResultWindow, - settings.getSettingValue(Settings.Key.SQL_CURSOR_KEEP_ALIVE), - client); + new OpenSearchRequestBuilder(exprValueFactory, maxResultWindow, settings) + .build(name, settings.getSettingValue(Settings.Key.SQL_CURSOR_KEEP_ALIVE), client); assertEquals( PhysicalPlanDSL.project( PhysicalPlanDSL.limit( diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilderTest.java index cd4b64cd164..af55c5255f5 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilderTest.java @@ -58,6 +58,7 @@ import org.opensearch.sql.opensearch.data.type.OpenSearchDataType; import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; import org.opensearch.sql.opensearch.data.value.OpenSearchExprValueFactory; +import org.opensearch.sql.opensearch.request.OpenSearchRequestBuilder.PushDownUnSupportedException; import org.opensearch.sql.opensearch.response.agg.CompositeAggregationParser; import org.opensearch.sql.opensearch.response.agg.OpenSearchAggregationResponseParser; import org.opensearch.sql.opensearch.response.agg.SinglePercentileParser; @@ -85,7 +86,7 @@ class OpenSearchRequestBuilderTest { @BeforeEach void setup() { - requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); lenient().when(settings.getSettingValue(Settings.Key.FIELD_TYPE_TOLERANCE)).thenReturn(false); } @@ -106,7 +107,7 @@ void build_query_request() { .trackScores(true), exprValueFactory, List.of()), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -116,8 +117,7 @@ void build_query_request_push_down_size() { requestBuilder.pushDownLimit(limit, offset); requestBuilder.pushDownTrackedScore(true); - assertNotNull( - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + assertNotNull(requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -136,7 +136,7 @@ void build_PIT_request_with_correct_size() { List.of(), TimeValue.timeValueMinutes(1), "samplePITId"), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -144,7 +144,7 @@ void buildRequestWithPit_pageSizeNull_sizeGreaterThanMaxResultWindow() { when(client.createPit(any(CreatePitRequest.class))).thenReturn("samplePITId"); Integer limit = 600; Integer offset = 0; - requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); requestBuilder.pushDownLimit(limit, offset); assertEquals( @@ -158,14 +158,14 @@ void buildRequestWithPit_pageSizeNull_sizeGreaterThanMaxResultWindow() { List.of(), TimeValue.timeValueMinutes(1), "samplePITId"), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test void buildRequestWithPit_pageSizeNull_sizeLessThanMaxResultWindow() { Integer limit = 400; Integer offset = 0; - requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); requestBuilder.pushDownLimit(limit, offset); assertEquals( @@ -174,7 +174,7 @@ void buildRequestWithPit_pageSizeNull_sizeLessThanMaxResultWindow() { new SearchSourceBuilder().from(offset).size(limit).timeout(DEFAULT_QUERY_TIMEOUT), exprValueFactory, List.of()), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -194,7 +194,7 @@ void buildRequestWithPit_pageSizeNotNull_startFromZero() { List.of(), TimeValue.timeValueMinutes(1), "samplePITId"), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -207,7 +207,7 @@ void buildRequestWithPit_pageSizeNotNull_startFromNonZero() { assertThrows( UnsupportedOperationException.class, () -> { - requestBuilder.build(indexName, 500, TimeValue.timeValueMinutes(1), client); + requestBuilder.build(indexName, TimeValue.timeValueMinutes(1), client); }); } @@ -216,7 +216,7 @@ void test_push_down_query() { QueryBuilder query = QueryBuilders.termQuery("intA", 1); requestBuilder.pushDownFilter(query); - var r = requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client); + var r = requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client); Function querySearch = searchRequest -> { assertEquals( @@ -354,7 +354,7 @@ void assertSearchSourceBuilder( throw new UnsupportedOperationException(); }; requestBuilder - .build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client) + .build(indexName, DEFAULT_QUERY_TIMEOUT, client) .search(querySearch, scrollSearch); } @@ -433,7 +433,7 @@ void test_push_down_project() { List.of("intA"), DEFAULT_QUERY_TIMEOUT, "samplePITId"), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -463,7 +463,7 @@ void test_push_down_project_limit() { .fetchSource("intA", null), exprValueFactory, List.of("intA")), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -493,7 +493,7 @@ void test_push_down_project_limit_and_offset() { .fetchSource("intA", null), exprValueFactory, List.of("intA")), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -561,7 +561,7 @@ void test_push_down_project_with_alias_type() { List.of("intA"), DEFAULT_QUERY_TIMEOUT, "samplePITId"), - requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -725,7 +725,7 @@ void exception_when_non_zero_offset_and_page_size() { requestBuilder.pushDownLimit(300, 2); assertThrows( UnsupportedOperationException.class, - () -> requestBuilder.build(indexName, MAX_RESULT_WINDOW, DEFAULT_QUERY_TIMEOUT, client)); + () -> requestBuilder.build(indexName, DEFAULT_QUERY_TIMEOUT, client)); } @Test @@ -739,4 +739,14 @@ void maxResponseSize_is_limit() { requestBuilder.pushDownLimit(100, 0); assertEquals(100, requestBuilder.getMaxResponseSize()); } + + @Test + void exception_when_pushDown_limit_with_offset_exceed_maxResultWindow() { + Exception e = + assertThrows( + PushDownUnSupportedException.class, + () -> requestBuilder.pushDownLimit(100, MAX_RESULT_WINDOW)); + assertEquals( + "Requested offset 500 should be less than the max result window 500", e.getMessage()); + } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexTest.java index d85a9bba6ee..6a4713dc917 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexTest.java @@ -201,11 +201,11 @@ void implementRelationOperatorOnly() { when(client.getIndexMaxResultWindows("test")).thenReturn(Map.of("test", 10000)); LogicalPlan plan = index.createScanBuilder(); Integer maxResultWindow = index.getMaxResultWindow(); - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = + new OpenSearchRequestBuilder(exprValueFactory, maxResultWindow, settings); assertEquals( new OpenSearchIndexScan( - client, - requestBuilder.build(INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client, true)), + client, requestBuilder.build(INDEX_NAME, SCROLL_TIMEOUT, client, true)), index.implement(index.optimize(plan))); } @@ -214,11 +214,11 @@ void implementRelationOperatorWithOptimization() { when(client.getIndexMaxResultWindows("test")).thenReturn(Map.of("test", 10000)); LogicalPlan plan = index.createScanBuilder(); Integer maxResultWindow = index.getMaxResultWindow(); - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = + new OpenSearchRequestBuilder(exprValueFactory, maxResultWindow, settings); assertEquals( new OpenSearchIndexScan( - client, - requestBuilder.build(INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client, true)), + client, requestBuilder.build(INDEX_NAME, SCROLL_TIMEOUT, client, true)), index.implement(plan)); } @@ -246,7 +246,8 @@ void implementOtherLogicalOperators() { include); Integer maxResultWindow = index.getMaxResultWindow(); - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = + new OpenSearchRequestBuilder(exprValueFactory, maxResultWindow, settings); assertEquals( PhysicalPlanDSL.project( PhysicalPlanDSL.dedupe( @@ -256,8 +257,7 @@ void implementOtherLogicalOperators() { PhysicalPlanDSL.rename( new OpenSearchIndexScan( client, - requestBuilder.build( - INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client, true)), + requestBuilder.build(INDEX_NAME, SCROLL_TIMEOUT, client, true)), mappings), exclude), newEvalField), diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanPaginationTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanPaginationTest.java index ee1576f9ccf..0a15df95b29 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanPaginationTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanPaginationTest.java @@ -8,7 +8,6 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.CALLS_REAL_METHODS; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -70,10 +69,9 @@ void setup() { @Test void query_empty_result() { mockResponse(client); - var builder = new OpenSearchRequestBuilder(exprValueFactory, settings); + var builder = new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); try (var indexScan = - new OpenSearchIndexScan( - client, builder.build(INDEX_NAME, MAX_RESULT_WINDOW, SCROLL_TIMEOUT, client))) { + new OpenSearchIndexScan(client, builder.build(INDEX_NAME, SCROLL_TIMEOUT, client))) { indexScan.open(); assertFalse(indexScan.hasNext()); } @@ -95,11 +93,10 @@ void dont_serialize_if_no_cursor() { OpenSearchRequestBuilder builder = mock(); OpenSearchRequest request = mock(); OpenSearchResponse response = mock(); - when(builder.build(any(), anyInt(), any(), any())).thenReturn(request); + when(builder.build(any(), any(), any())).thenReturn(request); when(client.search(any())).thenReturn(response); try (var indexScan = - new OpenSearchIndexScan( - client, builder.build(INDEX_NAME, MAX_RESULT_WINDOW, SCROLL_TIMEOUT, client))) { + new OpenSearchIndexScan(client, builder.build(INDEX_NAME, SCROLL_TIMEOUT, client))) { indexScan.open(); when(request.hasAnotherBatch()).thenReturn(false); diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java index a49617185ad..4e44735ab2b 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java @@ -169,10 +169,10 @@ void plan_for_serialization() { void query_empty_result() { mockResponse(client); final var name = new OpenSearchRequest.IndexName("test"); - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = + new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); try (OpenSearchIndexScan indexScan = - new OpenSearchIndexScan( - client, requestBuilder.build(name, MAX_RESULT_WINDOW, CURSOR_KEEP_ALIVE, client))) { + new OpenSearchIndexScan(client, requestBuilder.build(name, CURSOR_KEEP_ALIVE, client))) { indexScan.open(); assertFalse(indexScan.hasNext()); } @@ -187,10 +187,10 @@ void query_all_results_with_query() { employee(1, "John", "IT"), employee(2, "Smith", "HR"), employee(3, "Allen", "IT") }); - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, 10000, settings); try (OpenSearchIndexScan indexScan = new OpenSearchIndexScan( - client, requestBuilder.build(INDEX_NAME, 10000, CURSOR_KEEP_ALIVE, client))) { + client, requestBuilder.build(INDEX_NAME, CURSOR_KEEP_ALIVE, client))) { indexScan.open(); assertAll( @@ -215,10 +215,10 @@ void query_all_results_with_scroll() { new ExprValue[] {employee(1, "John", "IT"), employee(2, "Smith", "HR")}, new ExprValue[] {employee(3, "Allen", "IT")}); - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, 10000, settings); try (OpenSearchIndexScan indexScan = new OpenSearchIndexScan( - client, requestBuilder.build(INDEX_NAME, 10000, CURSOR_KEEP_ALIVE, client))) { + client, requestBuilder.build(INDEX_NAME, CURSOR_KEEP_ALIVE, client))) { indexScan.open(); assertAll( @@ -241,11 +241,11 @@ void query_some_results_with_query() { employee(1, "John", "IT"), employee(2, "Smith", "HR"), employee(3, "Allen", "IT"), }); - OpenSearchRequestBuilder builder = new OpenSearchRequestBuilder(exprValueFactory, settings); + OpenSearchRequestBuilder builder = + new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); builder.pushDownLimit(3, 0); try (OpenSearchIndexScan indexScan = - new OpenSearchIndexScan( - client, builder.build(INDEX_NAME, MAX_RESULT_WINDOW, CURSOR_KEEP_ALIVE, client))) { + new OpenSearchIndexScan(client, builder.build(INDEX_NAME, CURSOR_KEEP_ALIVE, client))) { indexScan.open(); assertAll( @@ -263,13 +263,12 @@ void query_some_results_with_query() { @Test void query_some_results_with_scroll() { mockTwoPageResponse(client); - final var requestuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestuilder = + new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); requestuilder.pushDownLimit(3, 0); try (OpenSearchIndexScan indexScan = new OpenSearchIndexScan( - client, - 3, - requestuilder.build(INDEX_NAME, MAX_RESULT_WINDOW, CURSOR_KEEP_ALIVE, client))) { + client, 3, requestuilder.build(INDEX_NAME, CURSOR_KEEP_ALIVE, client))) { indexScan.open(); assertAll( @@ -299,12 +298,12 @@ void query_results_limited_by_query_size() { employee(1, "John", "IT"), employee(2, "Smith", "HR"), }); - final var requestBuilder = new OpenSearchRequestBuilder(exprValueFactory, settings); + final var requestBuilder = + new OpenSearchRequestBuilder(exprValueFactory, MAX_RESULT_WINDOW, settings); requestBuilder.pushDownLimit(2, 0); try (OpenSearchIndexScan indexScan = new OpenSearchIndexScan( - client, - requestBuilder.build(INDEX_NAME, MAX_RESULT_WINDOW, CURSOR_KEEP_ALIVE, client))) { + client, requestBuilder.build(INDEX_NAME, CURSOR_KEEP_ALIVE, client))) { indexScan.open(); assertAll( @@ -373,7 +372,7 @@ private static class PushDownAssertion { public PushDownAssertion( OpenSearchClient client, OpenSearchExprValueFactory valueFactory, Settings settings) { this.client = client; - this.requestBuilder = new OpenSearchRequestBuilder(valueFactory, settings); + this.requestBuilder = new OpenSearchRequestBuilder(valueFactory, 10000, settings); this.response = mock(OpenSearchResponse.class); this.factory = valueFactory; @@ -406,7 +405,7 @@ PushDownAssertion shouldQueryHighlight(QueryBuilder query, HighlightBuilder high when(client.search(request)).thenReturn(response); var indexScan = new OpenSearchIndexScan( - client, requestBuilder.build(EMPLOYEES_INDEX, 10000, CURSOR_KEEP_ALIVE, client)); + client, requestBuilder.build(EMPLOYEES_INDEX, CURSOR_KEEP_ALIVE, client)); indexScan.open(); return this; } @@ -425,7 +424,7 @@ PushDownAssertion shouldQuery(QueryBuilder expected) { when(client.search(request)).thenReturn(response); var indexScan = new OpenSearchIndexScan( - client, requestBuilder.build(EMPLOYEES_INDEX, 10000, CURSOR_KEEP_ALIVE, client)); + client, requestBuilder.build(EMPLOYEES_INDEX, CURSOR_KEEP_ALIVE, client)); indexScan.open(); return this; } From 7b4ee6db48a63cd40fd2f73864c1fcd54a9a8b46 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Tue, 10 Jun 2025 12:00:13 +0800 Subject: [PATCH 2/2] Address comments Signed-off-by: Heng Qian --- .../resources/rest-api-spec/test/issues/3102.yml | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml index 63a3f31a375..08d0400f3c1 100644 --- a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3102.yml @@ -20,18 +20,8 @@ setup: - '{"index": {}}' - '{"id": 3}' - ---- -teardown: - - do: - query.settings: - body: - transient: - plugins.calcite.enabled : false - plugins.calcite.fallback.allowed : true - --- -"Prevent push down limit if the offset exceeds max_result_window": +"Prevent push down limit if the offset reach max_result_window": - do: headers: Content-Type: 'application/json'