From 815124776e46182fdac853c3b1b0e425738bbdcd Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Thu, 25 Sep 2025 14:49:00 +0800 Subject: [PATCH] Change the default search sorting to _shard_doc Signed-off-by: Lantao Jin --- .../legacy/executor/ElasticHitsExecutor.java | 7 ++---- .../legacy/executor/join/ElasticUtils.java | 6 ++--- .../sql/legacy/query/DefaultQueryAction.java | 5 ++-- .../node/pointInTime/PointInTime.java | 6 ++--- .../planner/physical/node/scroll/Scroll.java | 6 ++--- .../query/DefaultQueryActionTest.java | 8 +++---- .../request/OpenSearchQueryRequest.java | 8 ++----- .../request/OpenSearchRequestBuilder.java | 6 ++--- .../opensearch/storage/OpenSearchIndex.java | 4 ++++ .../request/OpenSearchRequestBuilderTest.java | 23 ++++++++----------- .../storage/scan/OpenSearchIndexScanTest.java | 6 ++--- 11 files changed, 35 insertions(+), 50 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticHitsExecutor.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticHitsExecutor.java index 7ae114f3f10..9608fd257e2 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticHitsExecutor.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticHitsExecutor.java @@ -5,9 +5,8 @@ package org.opensearch.sql.legacy.executor; -import static org.opensearch.search.sort.FieldSortBuilder.DOC_FIELD_NAME; import static org.opensearch.search.sort.SortOrder.ASC; -import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import java.io.IOException; import org.apache.logging.log4j.LogManager; @@ -16,7 +15,6 @@ import org.opensearch.action.search.SearchResponse; import org.opensearch.search.SearchHits; import org.opensearch.search.builder.PointInTimeBuilder; -import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.legacy.domain.Select; import org.opensearch.sql.legacy.exception.SqlParseException; import org.opensearch.sql.legacy.pit.PointInTimeHandler; @@ -66,8 +64,7 @@ public SearchResponse getResponseWithHits( // Set sort field for search_after boolean ordered = select.isOrderdSelect(); if (!ordered) { - request.addSort(DOC_FIELD_NAME, ASC); - request.addSort(METADATA_FIELD_ID, SortOrder.ASC); + request.addSort(SORT_FIELD_SHARD_DOC, ASC); } // Set PIT request.setPointInTime(new PointInTimeBuilder(pit.getPitId())); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java index 257e8dc1952..3387262ada4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java @@ -6,7 +6,7 @@ package org.opensearch.sql.legacy.executor.join; import static org.opensearch.core.xcontent.ToXContent.EMPTY_PARAMS; -import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import com.google.common.collect.ImmutableMap; import java.io.IOException; @@ -24,7 +24,6 @@ import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; -import org.opensearch.search.sort.FieldSortBuilder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.legacy.domain.Select; import org.opensearch.sql.legacy.query.join.BackOffRetryStrategy; @@ -39,8 +38,7 @@ public static SearchResponse scrollOneTimeWithHits( requestBuilder.setScroll(new TimeValue(60000)).setSize(resultSize); boolean ordered = originalSelect.isOrderdSelect(); if (!ordered) { - scrollRequest.addSort(FieldSortBuilder.DOC_FIELD_NAME, SortOrder.ASC); - scrollRequest.addSort(METADATA_FIELD_ID, SortOrder.ASC); + scrollRequest.addSort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); } SearchResponse responseWithHits = scrollRequest.get(); // on ordered select - not using SCAN , elastic returns hits on first scroll diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/DefaultQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/DefaultQueryAction.java index f317d81f4c3..10824518199 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/DefaultQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/DefaultQueryAction.java @@ -5,7 +5,7 @@ package org.opensearch.sql.legacy.query; -import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import com.alibaba.druid.sql.ast.SQLExpr; import com.alibaba.druid.sql.ast.expr.SQLBinaryOpExpr; @@ -107,8 +107,7 @@ public void checkAndSetScroll() { // set sort field for search_after boolean ordered = select.isOrderdSelect(); if (!ordered) { - request.addSort(FieldSortBuilder.DOC_FIELD_NAME, SortOrder.ASC); - request.addSort(METADATA_FIELD_ID, SortOrder.ASC); + request.addSort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); } // Request also requires PointInTime, but we should create pit while execution. } else { diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java index 368a24a8bf1..d46e61ffcb3 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java @@ -5,14 +5,13 @@ package org.opensearch.sql.legacy.query.planner.physical.node.pointInTime; -import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.common.unit.TimeValue; import org.opensearch.search.SearchHit; import org.opensearch.search.builder.PointInTimeBuilder; -import org.opensearch.search.sort.FieldSortBuilder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.legacy.pit.PointInTimeHandlerImpl; import org.opensearch.sql.legacy.query.join.TableInJoinRequestBuilder; @@ -64,8 +63,7 @@ protected void loadFirstBatch() { searchResponse = request .getRequestBuilder() - .addSort(FieldSortBuilder.DOC_FIELD_NAME, SortOrder.ASC) - .addSort(METADATA_FIELD_ID, SortOrder.ASC) + .addSort(SORT_FIELD_SHARD_DOC, SortOrder.ASC) .setSize(pageSize) .setTimeout(TimeValue.timeValueSeconds(timeout)) .setPointInTime(new PointInTimeBuilder(pitId)) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java index 9a8deba46a7..7b646ffa195 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java @@ -5,11 +5,10 @@ package org.opensearch.sql.legacy.query.planner.physical.node.scroll; -import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import org.opensearch.action.search.ClearScrollResponse; import org.opensearch.common.unit.TimeValue; -import org.opensearch.search.sort.FieldSortBuilder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.legacy.query.join.TableInJoinRequestBuilder; import org.opensearch.sql.legacy.query.planner.physical.node.Paginate; @@ -41,8 +40,7 @@ protected void loadFirstBatch() { searchResponse = request .getRequestBuilder() - .addSort(FieldSortBuilder.DOC_FIELD_NAME, SortOrder.ASC) - .addSort(METADATA_FIELD_ID, SortOrder.ASC) + .addSort(SORT_FIELD_SHARD_DOC, SortOrder.ASC) .setSize(pageSize) .setScroll(TimeValue.timeValueSeconds(timeout)) .get(); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java index 575e7db7668..e786ce7282b 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java @@ -7,6 +7,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.*; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import java.util.*; import org.junit.After; @@ -17,7 +18,6 @@ import org.opensearch.action.search.SearchRequestBuilder; import org.opensearch.common.unit.TimeValue; import org.opensearch.script.Script; -import org.opensearch.search.sort.FieldSortBuilder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.legacy.domain.Field; @@ -142,7 +142,7 @@ public void testIfScrollShouldBeOpenWithDifferentFormats() { queryAction.setFormat(Format.JDBC); queryAction.checkAndSetScroll(); Mockito.verify(mockRequestBuilder).setSize(settingFetchSize); - Mockito.verify(mockRequestBuilder).addSort(FieldSortBuilder.DOC_FIELD_NAME, SortOrder.ASC); + Mockito.verify(mockRequestBuilder).addSort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); Mockito.verify(mockRequestBuilder, never()).setScroll(timeValue); } @@ -162,7 +162,7 @@ public void testIfScrollShouldBeOpen() { mockLocalClusterStateAndInitializeMetrics(timeValue); queryAction.checkAndSetScroll(); Mockito.verify(mockRequestBuilder).setSize(settingFetchSize); - Mockito.verify(mockRequestBuilder).addSort(FieldSortBuilder.DOC_FIELD_NAME, SortOrder.ASC); + Mockito.verify(mockRequestBuilder).addSort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); Mockito.verify(mockRequestBuilder, never()).setScroll(timeValue); } @@ -190,7 +190,7 @@ public void testIfScrollShouldBeOpenWithDifferentFetchSize() { doReturn(mockRequestBuilder).when(mockRequestBuilder).setSize(userFetchSize); queryAction.checkAndSetScroll(); Mockito.verify(mockRequestBuilder).setSize(20); - Mockito.verify(mockRequestBuilder).addSort(FieldSortBuilder.DOC_FIELD_NAME, SortOrder.ASC); + Mockito.verify(mockRequestBuilder).addSort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); Mockito.verify(mockRequestBuilder, never()).setScroll(timeValue); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java index c6a3e6197db..a9e281e898c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java @@ -6,9 +6,8 @@ package org.opensearch.sql.opensearch.request; import static org.opensearch.core.xcontent.DeprecationHandler.IGNORE_DEPRECATIONS; -import static org.opensearch.search.sort.FieldSortBuilder.DOC_FIELD_NAME; import static org.opensearch.search.sort.SortOrder.ASC; -import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import java.io.IOException; import java.util.Collections; @@ -205,10 +204,7 @@ public OpenSearchResponse searchWithPIT(Function } // Set sort field for search_after if (this.sourceBuilder.sorts() == null) { - this.sourceBuilder.sort(DOC_FIELD_NAME, ASC); - // Workaround to preserve sort location more exactly, - // see https://github.com/opensearch-project/sql/pull/3061 - this.sourceBuilder.sort(METADATA_FIELD_ID, ASC); + this.sourceBuilder.sort(SORT_FIELD_SHARD_DOC, ASC); } SearchRequest searchRequest = new SearchRequest().indices(indexName.getIndexNames()).source(this.sourceBuilder); 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 9cc9bafc531..c4fd49caf76 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 @@ -9,8 +9,8 @@ import static java.util.stream.Collectors.toList; import static org.opensearch.index.query.QueryBuilders.matchAllQuery; import static org.opensearch.index.query.QueryBuilders.nestedQuery; -import static org.opensearch.search.sort.FieldSortBuilder.DOC_FIELD_NAME; import static org.opensearch.search.sort.SortOrder.ASC; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import java.util.ArrayList; import java.util.Arrays; @@ -182,7 +182,7 @@ public void pushDownFilter(QueryBuilder query) { } if (sourceBuilder.sorts() == null) { - sourceBuilder.sort(DOC_FIELD_NAME, ASC); // Make sure consistent order + sourceBuilder.sort(SORT_FIELD_SHARD_DOC, ASC); // Make sure consistent order } } @@ -313,7 +313,7 @@ public void pushDownCollapse(String field) { private boolean isSortByDocOnly() { List> sorts = sourceBuilder.sorts(); if (sorts != null) { - return sorts.equals(List.of(SortBuilders.fieldSort(DOC_FIELD_NAME))); + return sorts.equals(List.of(SortBuilders.fieldSort(SORT_FIELD_SHARD_DOC))); } return false; } 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 c8b00c6daed..2cbde4bf7ca 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 @@ -57,6 +57,10 @@ public class OpenSearchIndex extends AbstractOpenSearchTable { public static final String METADATA_FIELD_ROUTING = "_routing"; + // _SHARD_DOC introduced since 3.3.0, and it is not a new metadate field. + // So we cannot read it directly, it can be used in sorting only. + public static final String SORT_FIELD_SHARD_DOC = "_shard_doc"; + public static final java.util.Map METADATAFIELD_TYPE_MAP = new LinkedHashMap<>() { { 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 af55c5255f5..c9ff9dcb032 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 @@ -9,11 +9,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.*; import static org.opensearch.index.query.QueryBuilders.*; -import static org.opensearch.search.sort.FieldSortBuilder.DOC_FIELD_NAME; import static org.opensearch.search.sort.SortOrder.ASC; import static org.opensearch.sql.data.type.ExprCoreType.INTEGER; import static org.opensearch.sql.data.type.ExprCoreType.STRING; -import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import java.util.Collections; import java.util.List; @@ -225,7 +224,7 @@ void test_push_down_query() { .size(MAX_RESULT_WINDOW) .timeout(DEFAULT_QUERY_TIMEOUT) .query(query) - .sort(DOC_FIELD_NAME, ASC), + .sort(SORT_FIELD_SHARD_DOC, ASC), searchRequest.source()); return mock(); }; @@ -296,7 +295,7 @@ void test_push_down_query_and_sort() { void test_push_down_query_not_null() { SearchSourceBuilder sourceBuilder = requestBuilder.getSourceBuilder(); sourceBuilder.query(QueryBuilders.termQuery("name", "John")); - sourceBuilder.sort(DOC_FIELD_NAME, SortOrder.ASC); + sourceBuilder.sort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); QueryBuilder query = QueryBuilders.termQuery("intA", 1); requestBuilder.pushDownFilter(query); @@ -310,7 +309,7 @@ void test_push_down_query_not_null() { .size(MAX_RESULT_WINDOW) .timeout(DEFAULT_QUERY_TIMEOUT) .query(expectedQuery) - .sort(DOC_FIELD_NAME, SortOrder.ASC); + .sort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); assertSearchSourceBuilder(expectedSourceBuilder, requestBuilder); } @@ -332,7 +331,7 @@ void test_push_down_query_with_bool_filter() { .size(MAX_RESULT_WINDOW) .timeout(DEFAULT_QUERY_TIMEOUT) .query(initialBoolQuery) - .sort(DOC_FIELD_NAME, SortOrder.ASC); + .sort(SORT_FIELD_SHARD_DOC, SortOrder.ASC); assertSearchSourceBuilder(expectedSourceBuilder, requestBuilder); } @@ -412,8 +411,7 @@ void test_push_down_project() { .from(DEFAULT_OFFSET) .size(MAX_RESULT_WINDOW) .timeout(DEFAULT_QUERY_TIMEOUT) - .sort(DOC_FIELD_NAME, ASC) - .sort(METADATA_FIELD_ID, ASC) + .sort(SORT_FIELD_SHARD_DOC, ASC) .pointInTimeBuilder(new PointInTimeBuilder("samplePITId")) .fetchSource(new String[] {"intA"}, new String[0]), requestBuilder); @@ -425,8 +423,7 @@ void test_push_down_project() { .from(DEFAULT_OFFSET) .size(MAX_RESULT_WINDOW) .timeout(DEFAULT_QUERY_TIMEOUT) - .sort(DOC_FIELD_NAME, ASC) - .sort(METADATA_FIELD_ID, ASC) + .sort(SORT_FIELD_SHARD_DOC, ASC) .pointInTimeBuilder(new PointInTimeBuilder("samplePITId")) .fetchSource("intA", null), exprValueFactory, @@ -540,8 +537,7 @@ void test_push_down_project_with_alias_type() { .from(DEFAULT_OFFSET) .size(MAX_RESULT_WINDOW) .timeout(DEFAULT_QUERY_TIMEOUT) - .sort(DOC_FIELD_NAME, ASC) - .sort(METADATA_FIELD_ID, ASC) + .sort(SORT_FIELD_SHARD_DOC) .pointInTimeBuilder(new PointInTimeBuilder("samplePITId")) .fetchSource(new String[] {"intA"}, new String[0]), requestBuilder); @@ -553,8 +549,7 @@ void test_push_down_project_with_alias_type() { .from(DEFAULT_OFFSET) .size(MAX_RESULT_WINDOW) .timeout(DEFAULT_QUERY_TIMEOUT) - .sort(DOC_FIELD_NAME, ASC) - .sort(METADATA_FIELD_ID, ASC) + .sort(SORT_FIELD_SHARD_DOC, ASC) .pointInTimeBuilder(new PointInTimeBuilder("samplePITId")) .fetchSource("intA", null), exprValueFactory, 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 4e44735ab2b..b87ec4d838c 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 @@ -12,9 +12,9 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.*; -import static org.opensearch.search.sort.FieldSortBuilder.DOC_FIELD_NAME; import static org.opensearch.search.sort.SortOrder.ASC; import static org.opensearch.sql.data.type.ExprCoreType.STRING; +import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.SORT_FIELD_SHARD_DOC; import java.io.*; import java.util.Arrays; @@ -397,7 +397,7 @@ PushDownAssertion shouldQueryHighlight(QueryBuilder query, HighlightBuilder high .query(query) .size(MAX_RESULT_WINDOW) .highlighter(highlight) - .sort(DOC_FIELD_NAME, ASC); + .sort(SORT_FIELD_SHARD_DOC, ASC); OpenSearchRequest request = new OpenSearchQueryRequest( EMPLOYEES_INDEX, sourceBuilder, factory, List.of(), CURSOR_KEEP_ALIVE, null); @@ -417,7 +417,7 @@ PushDownAssertion shouldQuery(QueryBuilder expected) { .query(expected) .size(MAX_RESULT_WINDOW) .timeout(CURSOR_KEEP_ALIVE) - .sort(DOC_FIELD_NAME, ASC); + .sort(SORT_FIELD_SHARD_DOC, ASC); OpenSearchRequest request = new OpenSearchQueryRequest( EMPLOYEES_INDEX, builder, factory, List.of(), CURSOR_KEEP_ALIVE, null);