Skip to content

Commit 27bbb60

Browse files
committed
Fix v2 paginationIT
Signed-off-by: Lantao Jin <ltjin@amazon.com>
1 parent 1058b11 commit 27bbb60

2 files changed

Lines changed: 25 additions & 19 deletions

File tree

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

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,10 @@ public class OpenSearchQueryRequest implements OpenSearchRequest {
6969
/** List of includes expected in the response. */
7070
@EqualsAndHashCode.Exclude @ToString.Exclude private final List<String> includes;
7171

72-
@EqualsAndHashCode.Exclude @ToString.Exclude private boolean needClean = false;
72+
@EqualsAndHashCode.Exclude @ToString.Exclude private boolean needClean = true;
73+
74+
/** Indicate the search already done. */
75+
@EqualsAndHashCode.Exclude @ToString.Exclude private boolean searchDone = false;
7376

7477
private String pitId;
7578

@@ -145,6 +148,14 @@ public static OpenSearchQueryRequest pitOf(
145148
this.calciteEnabled = calciteEnabled;
146149
}
147150

151+
/** true if the request is a count aggregation request. */
152+
public boolean isCountAggRequest() {
153+
return !searchDone
154+
&& sourceBuilder.size() == 0
155+
&& sourceBuilder.trackTotalHitsUpTo() != null // only set in v3
156+
&& sourceBuilder.trackTotalHitsUpTo() == Integer.MAX_VALUE;
157+
}
158+
148159
/**
149160
* Constructs OpenSearchQueryRequest from serialized representation.
150161
*
@@ -194,25 +205,17 @@ public OpenSearchResponse search(
194205
}
195206
}
196207

197-
/** true if the request is a count aggregation request. */
198-
public boolean isCountAggRequest() {
199-
return !needClean
200-
&& sourceBuilder.size() == 0
201-
&& sourceBuilder.trackTotalHitsUpTo() != null // only set in v3
202-
&& sourceBuilder.trackTotalHitsUpTo() == Integer.MAX_VALUE;
203-
}
204-
205208
/** Call the old search logic for v2, since we don't support paginating aggregate in v2. */
206209
@Deprecated
207210
private OpenSearchResponse searchForV2(Function<SearchRequest, SearchResponse> searchAction) {
208211
// When SearchRequest doesn't contain PitId, fetch single page request
209-
if (needClean) {
212+
if (searchDone) {
210213
return new OpenSearchResponse(
211214
SearchHits.empty(), exprValueFactory, includes, isCountAggRequest());
212215
} else {
213-
// get the value before set needClean = true
216+
// get the value before set searchDone = true
214217
boolean isCountAggRequest = isCountAggRequest();
215-
needClean = true;
218+
searchDone = true;
216219
return new OpenSearchResponse(
217220
searchAction.apply(
218221
new SearchRequest().indices(indexName.getIndexNames()).source(sourceBuilder)),
@@ -224,7 +227,7 @@ private OpenSearchResponse searchForV2(Function<SearchRequest, SearchResponse> s
224227

225228
private OpenSearchResponse search(Function<SearchRequest, SearchResponse> searchAction) {
226229
OpenSearchResponse openSearchResponse;
227-
if (needClean) {
230+
if (searchDone) {
228231
openSearchResponse =
229232
new OpenSearchResponse(
230233
SearchHits.empty(), exprValueFactory, includes, isCountAggRequest());
@@ -248,6 +251,7 @@ private OpenSearchResponse search(Function<SearchRequest, SearchResponse> search
248251
this.searchResponse, exprValueFactory, includes, isCountAggRequest());
249252

250253
needClean = openSearchResponse.isEmpty();
254+
searchDone = openSearchResponse.isEmpty();
251255
// Get afterKey from response
252256
if (openSearchResponse.isAggregationResponse()) {
253257
openSearchResponse.getAggregations().asList().stream()
@@ -260,7 +264,7 @@ private OpenSearchResponse search(Function<SearchRequest, SearchResponse> search
260264

261265
public OpenSearchResponse searchWithPIT(Function<SearchRequest, SearchResponse> searchAction) {
262266
OpenSearchResponse openSearchResponse;
263-
if (needClean) {
267+
if (searchDone) {
264268
openSearchResponse =
265269
new OpenSearchResponse(
266270
SearchHits.empty(), exprValueFactory, includes, isCountAggRequest());
@@ -298,6 +302,7 @@ public OpenSearchResponse searchWithPIT(Function<SearchRequest, SearchResponse>
298302
this.searchResponse, exprValueFactory, includes, isCountAggRequest());
299303

300304
needClean = openSearchResponse.isEmpty();
305+
searchDone = openSearchResponse.isEmpty();
301306
SearchHit[] searchHits = this.searchResponse.getHits().getHits();
302307
if (searchHits != null && searchHits.length > 0) {
303308
searchAfter = searchHits[searchHits.length - 1].getSortValues();
@@ -316,6 +321,7 @@ public void clean(Consumer<String> cleanAction) {
316321
// clean on the last page only, to prevent deleting the PitId in the middle of paging.
317322
if (this.pitId != null && needClean) {
318323
cleanAction.accept(this.pitId);
324+
searchDone = true;
319325
}
320326
} finally {
321327
this.pitId = null;
@@ -329,7 +335,7 @@ public void forceClean(Consumer<String> cleanAction) {
329335
try {
330336
if (this.pitId != null) {
331337
cleanAction.accept(this.pitId);
332-
needClean = true;
338+
searchDone = true;
333339
}
334340
} finally {
335341
this.pitId = null;

opensearch/src/test/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequestTest.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ void has_another_batch() {
275275
new TimeValue(1000),
276276
"sample",
277277
false);
278-
assertTrue(request.hasAnotherBatch());
278+
assertFalse(request.hasAnotherBatch());
279279
}
280280

281281
@Test
@@ -366,7 +366,7 @@ void testCleanConditionTrue() {
366366
request.clean(cleanAction);
367367

368368
verify(cleanAction, times(1)).accept("samplePid");
369-
assertTrue(openSearchResponse.isEmpty());
369+
assertTrue(request.isSearchDone());
370370
assertNull(request.getPitId());
371371
}
372372

@@ -389,7 +389,7 @@ void testCleanConditionFalse_needCleanFalse() {
389389

390390
request.clean(cleanAction);
391391
verify(cleanAction, never()).accept(anyString());
392-
assertFalse(openSearchResponse.isEmpty());
392+
assertFalse(request.isSearchDone());
393393
assertNull(request.getPitId());
394394
}
395395

@@ -407,7 +407,7 @@ void testCleanConditionFalse_pidNull() {
407407

408408
request.clean(cleanAction);
409409
verify(cleanAction, never()).accept(anyString());
410-
assertFalse(request.isNeedClean());
410+
assertFalse(request.isSearchDone());
411411
assertNull(request.getPitId());
412412
}
413413

0 commit comments

Comments
 (0)