From 4e71525d6d067984ad554e624a9430e68687084f Mon Sep 17 00:00:00 2001 From: Peng Huo Date: Tue, 10 Feb 2026 22:19:32 -0800 Subject: [PATCH 1/4] Fix bug 5114 Signed-off-by: Peng Huo --- .../sql/calcite/remote/CalciteExplainIT.java | 11 +++ ...xplain_issue_5114_sort_expr_head_push.yaml | 9 ++ .../rest-api-spec/test/issues/5114.yml | 97 +++++++++++++++++++ .../planner/rules/SortExprIndexScanRule.java | 23 ++++- 4 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 integ-test/src/test/resources/expectedOutput/calcite/explain_issue_5114_sort_expr_head_push.yaml create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5114.yml diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java index 0bbc25f1d7..8343b6c973 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java @@ -2033,6 +2033,17 @@ public void testTopKThenSortExplain() throws IOException { + "| fields age")); } + @Test + public void testIssue5114SortExprHeadExplain() throws IOException { + enabledOnlyWhenPushdownIsEnabled(); + String query = + "source=opensearch-sql_test_index_account | eval a = rand() | sort a | fields" + + " account_number | head 5"; + var result = explainQueryYaml(query); + String expected = loadExpectedPlan("explain_issue_5114_sort_expr_head_push.yaml"); + assertYamlEqualsIgnoreId(expected, result); + } + @Test public void testGeoIpPushedInAgg() throws IOException { // This explain IT verifies that externally registered UDF can be properly pushed down diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_issue_5114_sort_expr_head_push.yaml b/integ-test/src/test/resources/expectedOutput/calcite/explain_issue_5114_sort_expr_head_push.yaml new file mode 100644 index 0000000000..84d91a3885 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_issue_5114_sort_expr_head_push.yaml @@ -0,0 +1,9 @@ +calcite: + logical: | + LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) + LogicalProject(account_number=[$0]) + LogicalSort(sort0=[$17], dir0=[ASC-nulls-first], fetch=[5]) + LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10], _id=[$11], _index=[$12], _score=[$13], _maxscore=[$14], _sort=[$15], _routing=[$16], a=[RAND()]) + CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]]) + physical: | + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number], SORT_EXPR->[RAND() ASCENDING NULLS_FIRST], LIMIT->5, LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":5,"timeout":"1m","_source":{"includes":["account_number"],"excludes":[]},"sort":[{"_script":{"script":{"source":"{\"langType\":\"calcite\",\"script\":\"rO0ABXQAbnsKICAib3AiOiB7CiAgICAibmFtZSI6ICJSQU5EIiwKICAgICJraW5kIjogIk9USEVSX0ZVTkNUSU9OIiwKICAgICJzeW50YXgiOiAiRlVOQ1RJT04iCiAgfSwKICAib3BlcmFuZHMiOiBbXQp9\"}","lang":"opensearch_compounded_script","params":{"MISSING_MAX":false,"utcTimestamp": 0,"SOURCES":[],"DIGESTS":[]}},"type":"number","order":"asc"}}]}, requestedTotalSize=5, pageSize=null, startFrom=0)]) diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5114.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5114.yml new file mode 100644 index 0000000000..8b463de6b0 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5114.yml @@ -0,0 +1,97 @@ +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: true + + - do: + indices.create: + index: issue5114 + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + id: + type: integer + name: + type: keyword + reportsTo: + type: keyword + + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "issue5114", "_id": "1"}}' + - '{"id": 1, "name": "Dev", "reportsTo": "Eliot"}' + - '{"index": {"_index": "issue5114", "_id": "2"}}' + - '{"id": 2, "name": "Eliot", "reportsTo": "Ron"}' + - '{"index": {"_index": "issue5114", "_id": "3"}}' + - '{"id": 3, "name": "Ron", "reportsTo": "Andrew"}' + - '{"index": {"_index": "issue5114", "_id": "4"}}' + - '{"id": 4, "name": "Andrew", "reportsTo": null}' + - '{"index": {"_index": "issue5114", "_id": "5"}}' + - '{"id": 5, "name": "Asya", "reportsTo": "Ron"}' + - '{"index": {"_index": "issue5114", "_id": "6"}}' + - '{"id": 6, "name": "Dan", "reportsTo": "Andrew"}' + +--- +teardown: + - do: + indices.delete: + index: issue5114 + ignore_unavailable: true + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: false + +--- +"Issue 5114: head should be preserved for non-order-equivalent deterministic sort expression": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=issue5114 | eval a = abs(id) + 1 | sort a | fields id | head 5 + + - match: { total: 5 } + - match: { schema: [ { name: id, type: int } ] } + +--- +"Issue 5114: head should be preserved for non-deterministic sort expression": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=issue5114 | eval a = rand() | sort a | fields id | head 5 + + - match: { total: 5 } + - match: { schema: [ { name: id, type: int } ] } + +--- +"Issue 5114 control: order-equivalent expression remains correct": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=issue5114 | eval a = id + 1 | sort a | fields id | head 5 + + - match: { total: 5 } + - match: { schema: [ { name: id, type: int } ] } + - match: { datarows: [ [ 1 ], [ 2 ], [ 3 ], [ 4 ], [ 5 ] ] } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java index b7c25912bc..543e46c24c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java @@ -27,6 +27,7 @@ import org.immutables.value.Value; import org.opensearch.sql.calcite.plan.rule.OpenSearchRuleConfig; import org.opensearch.sql.calcite.utils.PlanUtils; +import org.opensearch.sql.opensearch.planner.physical.CalciteEnumerableTopK; import org.opensearch.sql.opensearch.storage.scan.AbstractCalciteIndexScan; import org.opensearch.sql.opensearch.storage.scan.CalciteLogicalIndexScan; import org.opensearch.sql.opensearch.storage.scan.context.SortExprDigest; @@ -47,6 +48,11 @@ protected SortExprIndexScanRule(SortExprIndexScanRule.Config config) { @Override protected void onMatchImpl(RelOptRuleCall call) { final Sort sort = call.rel(0); + // CalciteEnumerableTopK carries fetch semantics; this rule may collapse Sort/TopK into a + // project-on-scan shape, so skip TopK and let dedicated TopK/limit rules preserve semantics. + if (sort instanceof CalciteEnumerableTopK) { + return; + } final Project project = call.rel(1); final AbstractCalciteIndexScan scan = call.rel(2); @@ -102,14 +108,23 @@ protected void onMatchImpl(RelOptRuleCall call) { newScan = scan.pushdownSortExpr(sortExprDigests); } - // EnumerableSort won't have limit or offset - Integer limitValue = LimitIndexScanRule.extractLimitValue(sort.fetch); - Integer offsetValue = LimitIndexScanRule.extractOffsetValue(sort.offset); - if (newScan instanceof CalciteLogicalIndexScan && limitValue != null && offsetValue != null) { + // Keep top-k semantics intact: remove Sort only when limit/offset is also preserved in scan. + if (sort.fetch != null || sort.offset != null) { + Integer limitValue = LimitIndexScanRule.extractLimitValue(sort.fetch); + Integer offsetValue = LimitIndexScanRule.extractOffsetValue(sort.offset); + if (!(newScan instanceof CalciteLogicalIndexScan) + || !(sort instanceof LogicalSort) + || limitValue == null + || offsetValue == null) { + return; + } newScan = (CalciteLogicalIndexScan) ((CalciteLogicalIndexScan) newScan) .pushDownLimit((LogicalSort) sort, limitValue, offsetValue); + if (newScan == null) { + return; + } } if (newScan != null) { From 66711913ca8856e181202d12c50f4c857863a600 Mon Sep 17 00:00:00 2001 From: Peng Huo Date: Tue, 10 Feb 2026 22:27:02 -0800 Subject: [PATCH 2/4] Update Signed-off-by: Peng Huo --- .../planner/rules/SortExprIndexScanRule.java | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java index 543e46c24c..80b3c112a4 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java @@ -48,8 +48,8 @@ protected SortExprIndexScanRule(SortExprIndexScanRule.Config config) { @Override protected void onMatchImpl(RelOptRuleCall call) { final Sort sort = call.rel(0); - // CalciteEnumerableTopK carries fetch semantics; this rule may collapse Sort/TopK into a - // project-on-scan shape, so skip TopK and let dedicated TopK/limit rules preserve semantics. + // CalciteEnumerableTopK carries fetch semantics; this rule doesn't preserve it on physical + // scans because limit pushdown path is logical-only. if (sort instanceof CalciteEnumerableTopK) { return; } @@ -108,23 +108,14 @@ protected void onMatchImpl(RelOptRuleCall call) { newScan = scan.pushdownSortExpr(sortExprDigests); } - // Keep top-k semantics intact: remove Sort only when limit/offset is also preserved in scan. - if (sort.fetch != null || sort.offset != null) { - Integer limitValue = LimitIndexScanRule.extractLimitValue(sort.fetch); - Integer offsetValue = LimitIndexScanRule.extractOffsetValue(sort.offset); - if (!(newScan instanceof CalciteLogicalIndexScan) - || !(sort instanceof LogicalSort) - || limitValue == null - || offsetValue == null) { - return; - } + // EnumerableSort won't have limit or offset + Integer limitValue = LimitIndexScanRule.extractLimitValue(sort.fetch); + Integer offsetValue = LimitIndexScanRule.extractOffsetValue(sort.offset); + if (newScan instanceof CalciteLogicalIndexScan && limitValue != null && offsetValue != null) { newScan = (CalciteLogicalIndexScan) ((CalciteLogicalIndexScan) newScan) .pushDownLimit((LogicalSort) sort, limitValue, offsetValue); - if (newScan == null) { - return; - } } if (newScan != null) { From 9593b7ec610a89b775e3414a5b3fe5ad46f59781 Mon Sep 17 00:00:00 2001 From: Peng Huo Date: Thu, 12 Feb 2026 11:14:29 -0800 Subject: [PATCH 3/4] Address comments Signed-off-by: Peng Huo --- .../sql/opensearch/planner/rules/SortExprIndexScanRule.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java index 80b3c112a4..a2bcc6844c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java @@ -27,7 +27,7 @@ import org.immutables.value.Value; import org.opensearch.sql.calcite.plan.rule.OpenSearchRuleConfig; import org.opensearch.sql.calcite.utils.PlanUtils; -import org.opensearch.sql.opensearch.planner.physical.CalciteEnumerableTopK; +import org.apache.calcite.adapter.enumerable.EnumerableLimitSort; import org.opensearch.sql.opensearch.storage.scan.AbstractCalciteIndexScan; import org.opensearch.sql.opensearch.storage.scan.CalciteLogicalIndexScan; import org.opensearch.sql.opensearch.storage.scan.context.SortExprDigest; @@ -48,9 +48,9 @@ protected SortExprIndexScanRule(SortExprIndexScanRule.Config config) { @Override protected void onMatchImpl(RelOptRuleCall call) { final Sort sort = call.rel(0); - // CalciteEnumerableTopK carries fetch semantics; this rule doesn't preserve it on physical + // EnumerableLimitSort carries fetch semantics; this rule doesn't preserve it on physical // scans because limit pushdown path is logical-only. - if (sort instanceof CalciteEnumerableTopK) { + if (sort instanceof EnumerableLimitSort) { return; } final Project project = call.rel(1); From 451bba6ea52e7a14d218742af554125190321d59 Mon Sep 17 00:00:00 2001 From: Peng Huo Date: Fri, 13 Feb 2026 09:10:11 -0800 Subject: [PATCH 4/4] Fix spotless import ordering for EnumerableLimitSort Signed-off-by: Peng Huo --- .../sql/opensearch/planner/rules/SortExprIndexScanRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java index a2bcc6844c..46e40729d5 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java @@ -11,6 +11,7 @@ import java.util.Map; import java.util.Optional; import java.util.function.Predicate; +import org.apache.calcite.adapter.enumerable.EnumerableLimitSort; import org.apache.calcite.plan.RelOptRuleCall; import org.apache.calcite.rel.RelFieldCollation; import org.apache.calcite.rel.RelFieldCollation.Direction; @@ -27,7 +28,6 @@ import org.immutables.value.Value; import org.opensearch.sql.calcite.plan.rule.OpenSearchRuleConfig; import org.opensearch.sql.calcite.utils.PlanUtils; -import org.apache.calcite.adapter.enumerable.EnumerableLimitSort; import org.opensearch.sql.opensearch.storage.scan.AbstractCalciteIndexScan; import org.opensearch.sql.opensearch.storage.scan.CalciteLogicalIndexScan; import org.opensearch.sql.opensearch.storage.scan.context.SortExprDigest;