udpated implementation: ignore reverse if no collation/@timestamp found

Signed-off-by: Kai Huang <[email protected]>

Author: ahkcs

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

@@ -669,8 +669,6 @@ public RelNode visitHead(Head node, CalcitePlanContext context) {
     return context.relBuilder.peek();
   }
 
-  private static final String REVERSE_ROW_NUM = "__reverse_row_num__";
-
   @Override
   public RelNode visitReverse(
       org.opensearch.sql.ast.tree.Reverse node, CalcitePlanContext context) {
@@ -686,17 +684,15 @@ public RelNode visitReverse(
       RelCollation reversedCollation = PlanUtils.reverseCollation(collation);
       context.relBuilder.sort(reversedCollation);
     } else {
-      // Fallback: use ROW_NUMBER approach when no existing sort
-      RexNode rowNumber =
-          context
-              .relBuilder
-              .aggregateCall(SqlStdOperatorTable.ROW_NUMBER)
-              .over()
-              .rowsTo(RexWindowBounds.CURRENT_ROW)
-              .as(REVERSE_ROW_NUM);
-      context.relBuilder.projectPlus(rowNumber);
-      context.relBuilder.sort(context.relBuilder.desc(context.relBuilder.field(REVERSE_ROW_NUM)));
-      context.relBuilder.projectExcept(context.relBuilder.field(REVERSE_ROW_NUM));
+      // Check if @timestamp field exists in the row type
+      List<String> fieldNames = context.relBuilder.peek().getRowType().getFieldNames();
+      if (fieldNames.contains(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP)) {
+        // If @timestamp exists, sort by it in descending order
+        context.relBuilder.sort(
+            context.relBuilder.desc(
+                context.relBuilder.field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP)));
+      }
+      // If neither collation nor @timestamp exists, ignore the reverse command (no-op)
     }
 
     return context.relBuilder.peek();

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

@@ -410,10 +410,11 @@ public void testFilterWithSearchCall() throws IOException {
   }
 
   @Test
-  public void testExplainWithReverse() throws IOException {
+  public void testExplainWithReverseIgnored() throws IOException {
+    // Reverse is ignored when there's no existing sort and no @timestamp field
     String query = "source=opensearch-sql_test_index_account | reverse | head 5";
     var result = explainQueryYaml(query);
-    String expected = loadExpectedPlan("explain_reverse_fallback.yaml");
+    String expected = loadExpectedPlan("explain_reverse_ignored.yaml");
     assertYamlEqualsIgnoreId(expected, result);
   }
 
@@ -434,10 +435,11 @@ public void testExplainWithReversePushdownMultipleFields() throws IOException {
   }
 
   @Test
-  public void testExplainWithDoubleReverse() throws IOException {
+  public void testExplainWithDoubleReverseIgnored() throws IOException {
+    // Double reverse is ignored when there's no existing sort and no @timestamp field
     String query = "source=opensearch-sql_test_index_account | reverse | reverse";
     var result = explainQueryYaml(query);
-    String expected = loadExpectedPlan("explain_double_reverse_fallback.yaml");
+    String expected = loadExpectedPlan("explain_double_reverse_ignored.yaml");
     assertYamlEqualsIgnoreId(expected, result);
   }
 
@@ -458,6 +460,15 @@ public void testExplainWithDoubleReversePushdownMultipleFields() throws IOExcept
     assertYamlEqualsIgnoreId(expected, result);
   }
 
+  @Test
+  public void testExplainReverseWithTimestamp() throws IOException {
+    // Test that reverse with @timestamp field sorts by @timestamp DESC
+    String query = "source=opensearch-sql_test_index_time_data | reverse | head 5";
+    var result = explainQueryYaml(query);
+    String expected = loadExpectedPlan("explain_reverse_with_timestamp.yaml");
+    assertYamlEqualsIgnoreId(expected, result);
+  }
+
   @Test
   public void testExplainWithTimechartAvg() throws IOException {
     var result = explainQueryYaml("source=events | timechart span=1m avg(cpu_usage) by host");

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

@@ -6,6 +6,7 @@
 package org.opensearch.sql.calcite.remote;
 
 import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK;
+import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_TIME_DATA;
 import static org.opensearch.sql.util.MatcherUtils.rows;
 import static org.opensearch.sql.util.MatcherUtils.schema;
 import static org.opensearch.sql.util.MatcherUtils.verifyDataRowsInOrder;
@@ -24,12 +25,16 @@ public void init() throws Exception {
     enableCalcite();
     disallowCalciteFallback();
     loadIndex(Index.BANK);
+    loadIndex(Index.TIME_TEST_DATA);
   }
 
   @Test
   public void testReverse() throws IOException {
     JSONObject result =
-        executeQuery(String.format("source=%s | fields account_number | reverse", TEST_INDEX_BANK));
+        executeQuery(
+            String.format(
+                "source=%s | fields account_number | sort account_number | reverse",
+                TEST_INDEX_BANK));
     verifySchema(result, schema("account_number", "bigint"));
     verifyDataRowsInOrder(
         result, rows(32), rows(25), rows(20), rows(18), rows(13), rows(6), rows(1));
@@ -40,7 +45,8 @@ public void testReverseWithFields() throws IOException {
     JSONObject result =
         executeQuery(
             String.format(
-                "source=%s | fields account_number, firstname | reverse", TEST_INDEX_BANK));
+                "source=%s | fields account_number, firstname | sort account_number | reverse",
+                TEST_INDEX_BANK));
     verifySchema(result, schema("account_number", "bigint"), schema("firstname", "string"));
     verifyDataRowsInOrder(
         result,
@@ -70,7 +76,8 @@ public void testDoubleReverse() throws IOException {
     JSONObject result =
         executeQuery(
             String.format(
-                "source=%s | fields account_number | reverse | reverse", TEST_INDEX_BANK));
+                "source=%s | fields account_number | sort account_number | reverse | reverse",
+                TEST_INDEX_BANK));
     verifySchema(result, schema("account_number", "bigint"));
     verifyDataRowsInOrder(
         result, rows(1), rows(6), rows(13), rows(18), rows(20), rows(25), rows(32));
@@ -80,7 +87,9 @@ public void testDoubleReverse() throws IOException {
   public void testReverseWithHead() throws IOException {
     JSONObject result =
         executeQuery(
-            String.format("source=%s | fields account_number | reverse | head 3", TEST_INDEX_BANK));
+            String.format(
+                "source=%s | fields account_number | sort account_number | reverse | head 3",
+                TEST_INDEX_BANK));
     verifySchema(result, schema("account_number", "bigint"));
     verifyDataRowsInOrder(result, rows(32), rows(25), rows(20));
   }
@@ -90,7 +99,8 @@ public void testReverseWithComplexPipeline() throws IOException {
     JSONObject result =
         executeQuery(
             String.format(
-                "source=%s | where account_number > 18 | fields account_number | reverse | head 2",
+                "source=%s | where account_number > 18 | fields account_number | sort"
+                    + " account_number | reverse | head 2",
                 TEST_INDEX_BANK));
     verifySchema(result, schema("account_number", "bigint"));
     verifyDataRowsInOrder(result, rows(32), rows(25));
@@ -163,4 +173,55 @@ public void testDoubleReverseWithMixedSortDirections() throws IOException {
         rows(6, "Hattie"),
         rows(1, "Amber JOHnny"));
   }
+
+  @Test
+  public void testReverseIgnoredWithoutSortOrTimestamp() throws IOException {
+    // Test that reverse is ignored when there's no explicit sort and no @timestamp field
+    // BANK index doesn't have @timestamp, so reverse should be ignored
+    JSONObject result =
+        executeQuery(
+            String.format("source=%s | fields account_number | reverse | head 3", TEST_INDEX_BANK));
+    verifySchema(result, schema("account_number", "bigint"));
+    // Without sort or @timestamp, reverse is ignored, so data comes in natural order
+    // The first 3 documents in natural order (ascending by account_number)
+    verifyDataRowsInOrder(result, rows(1), rows(6), rows(13));
+  }
+
+  @Test
+  public void testReverseWithTimestampField() throws IOException {
+    // Test that reverse with @timestamp field sorts by @timestamp DESC
+    // TIME_TEST_DATA index has @timestamp field
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | fields value, category, `@timestamp` | reverse | head 5",
+                TEST_INDEX_TIME_DATA));
+    verifySchema(
+        result,
+        schema("value", "int"),
+        schema("category", "string"),
+        schema("@timestamp", "timestamp"));
+    // Should return the latest 5 records (highest @timestamp values) in descending order
+    // Based on the test data, these are IDs 100, 99, 98, 97, 96
+    verifyDataRowsInOrder(
+        result,
+        rows(8762, "A", "2025-08-01 03:47:41"),
+        rows(7348, "C", "2025-08-01 02:00:56"),
+        rows(9015, "B", "2025-08-01 01:14:11"),
+        rows(6489, "D", "2025-08-01 00:27:26"),
+        rows(8676, "A", "2025-07-31 23:40:33"));
+  }
+
+  @Test
+  public void testReverseWithTimestampAndExplicitSort() throws IOException {
+    // Test that explicit sort takes precedence over @timestamp
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | fields value, category | sort value | reverse | head 3",
+                TEST_INDEX_TIME_DATA));
+    verifySchema(result, schema("value", "int"), schema("category", "string"));
+    // Should reverse the value sort, giving us the highest values
+    verifyDataRowsInOrder(result, rows(9521, "B"), rows(9367, "A"), rows(9321, "A"));
+  }
 }

integ-test/src/test/resources/expectedOutput/calcite/explain_double_reverse_fallback.yaml

@@ -0,0 +1,7 @@
+calcite:
+  logical: |
+    LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
+      LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])
+        CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
+  physical: |
+    CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","_source":{"includes":["account_number","firstname","address","balance","gender","city","employer","state","age","email","lastname"],"excludes":[]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)])

integ-test/src/test/resources/expectedOutput/calcite/explain_double_reverse_ignored.yaml

@@ -0,0 +1,8 @@
+calcite:
+  logical: |
+    LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
+      LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])
+        LogicalSort(fetch=[5])
+          CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
+  physical: |
+    CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], LIMIT->5, LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":5,"timeout":"1m","_source":{"includes":["account_number","firstname","address","balance","gender","city","employer","state","age","email","lastname"],"excludes":[]}}, requestedTotalSize=5, pageSize=null, startFrom=0)])

integ-test/src/test/resources/expectedOutput/calcite/explain_reverse_fallback.yaml

@@ -0,0 +1,13 @@
+calcite:
+  logical: |
+    LogicalSystemLimit(sort0=[$0], dir0=[DESC], fetch=[10000], type=[QUERY_SIZE_LIMIT])
+      LogicalProject(@timestamp=[$0], category=[$1], value=[$2], timestamp=[$3])
+        LogicalSort(sort0=[$0], dir0=[DESC], fetch=[5])
+          CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_time_data]])
+  physical: |
+    CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_time_data]], PushDownContext=[[PROJECT->[@timestamp, category, value, timestamp], SORT->[{
+      "@timestamp" : {
+        "order" : "desc",
+        "missing" : "_first"
+      }
+    }], LIMIT->5, LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":5,"timeout":"1m","_source":{"includes":["@timestamp","category","value","timestamp"],"excludes":[]},"sort":[{"@timestamp":{"order":"desc","missing":"_first"}}]}, requestedTotalSize=5, pageSize=null, startFrom=0)])

integ-test/src/test/resources/expectedOutput/calcite/explain_reverse_ignored.yaml

@@ -0,0 +1,9 @@
+calcite:
+  logical: |
+    LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
+      LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])
+        CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
+  physical: |
+    EnumerableLimit(fetch=[10000])
+      EnumerableCalc(expr#0..16=[{inputs}], proj#0..10=[{exprs}])
+        CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])