Fix asc/desc keyword behavior for sort command

docs/user/ppl/cmd/sort.rst

@@ -16,13 +16,16 @@ Description
 
 Syntax
 ============
-sort [count] <[+|-] sort-field>... [asc|a|desc|d]
+sort [count] <[+|-] sort-field | sort-field [asc|a|desc|d]>...
 
 
 * count (Since 3.3): optional. The number of results to return. **Default:** returns all results. Specifying a count of 0 or less than 0 also returns all results.
 * [+|-]: optional. The plus [+] stands for ascending order and NULL/MISSING first and a minus [-] stands for descending order and NULL/MISSING last. **Default:** ascending order and NULL/MISSING first.
+* [asc|a|desc|d]: optional. asc/a stands for ascending order and NULL/MISSING first. desc/d stands for descending order and NULL/MISSING last. **Default:** ascending order and NULL/MISSING first.
 * sort-field: mandatory. The field used to sort. Can use ``auto(field)``, ``str(field)``, ``ip(field)``, or ``num(field)`` to specify how to interpret field values.
-* [asc|a|desc|d] (Since 3.3): optional. asc/a keeps the sort order as specified. desc/d reverses the sort results. If multiple fields are specified with desc/d, reverses order of the first field then for all duplicate values of the first field, reverses the order of the values of the second field and so on. **Default:** asc.
+
+.. note::
+   You cannot mix +/- and asc/desc in the same sort command. Choose one approach for all fields in a single sort command.
 
 
 Example 1: Sort by one field
@@ -63,10 +66,10 @@ PPL query::
     +----------------+-----+
 
 
-Example 3: Sort by one field in descending order
-================================================
+Example 3: Sort by one field in descending order (using -)
+==========================================================
 
-The example show sort all the document with age field in descending order.
+The example show sort all the document with age field in descending order using the - operator.
 
 PPL query::
 
@@ -81,10 +84,28 @@ PPL query::
     | 13             | 28  |
     +----------------+-----+
 
-Example 4: Sort by multiple field
-=============================
+Example 4: Sort by one field in descending order (using desc)
+==============================================================
 
-The example show sort all the document with gender field in ascending order and age field in descending.
+The example show sort all the document with age field in descending order using the desc keyword.
+
+PPL query::
+
+    os> source=accounts | sort age desc | fields account_number, age;
+    fetched rows / total rows = 4/4
+    +----------------+-----+
+    | account_number | age |
+    |----------------+-----|
+    | 6              | 36  |
+    | 18             | 33  |
+    | 1              | 32  |
+    | 13             | 28  |
+    +----------------+-----+
+
+Example 5: Sort by multiple fields (using +/-)
+==============================================
+
+The example show sort all the document with gender field in ascending order and age field in descending using +/- operators.
 
 PPL query::
 
@@ -99,10 +120,28 @@ PPL query::
     | 1              | M      | 32  |
     +----------------+--------+-----+
 
-Example 4: Sort by field include null value
+Example 6: Sort by multiple fields (using asc/desc)
+====================================================
+
+The example show sort all the document with gender field in ascending order and age field in descending using asc/desc keywords.
+
+PPL query::
+
+    os> source=accounts | sort gender asc, age desc | fields account_number, gender, age;
+    fetched rows / total rows = 4/4
+    +----------------+--------+-----+
+    | account_number | gender | age |
+    |----------------+--------+-----|
+    | 13             | F      | 28  |
+    | 6              | M      | 36  |
+    | 18             | M      | 33  |
+    | 1              | M      | 32  |
+    +----------------+--------+-----+
+
+Example 7: Sort by field include null value
 ===========================================
 
-The example show sort employer field by default option (ascending order and null first), the result show that null value is in the first row.
+The example shows sorting the employer field by the default option (ascending order and null first), the result shows that the null value is in the first row.
 
 PPL query::
 
@@ -117,7 +156,7 @@ PPL query::
     | Quility  |
     +----------+
 
-Example 5: Specify the number of sorted documents to return
+Example 8: Specify the number of sorted documents to return
 ============================================================
 
 The example shows sorting all the document and returning 2 documents.
@@ -133,7 +172,7 @@ PPL query::
     | 1              | 32  |
     +----------------+-----+
 
-Example 6: Sort with desc modifier
+Example 9: Sort with desc modifier
 ===================================
 
 The example shows sorting with the desc modifier to reverse sort order.
@@ -151,26 +190,7 @@ PPL query::
     | 13             | 28  |
     +----------------+-----+
 
-Example 7: Sort by multiple fields with desc modifier
-======================================================
-
-The example shows sorting by multiple fields using desc, which reverses the sort order for all specified fields. Gender is reversed from ascending to descending, and the descending age sort is reversed to ascending within each gender group.
-
-PPL query::
-
-    os> source=accounts | sort gender, -age desc | fields account_number, gender, age;
-    fetched rows / total rows = 4/4
-    +----------------+--------+-----+
-    | account_number | gender | age |
-    |----------------+--------+-----|
-    | 1              | M      | 32  |
-    | 18             | M      | 33  |
-    | 6              | M      | 36  |
-    | 13             | F      | 28  |
-    +----------------+--------+-----+
-
-
-Example 8: Sort with specifying field type
+Example 10: Sort with specifying field type
 ==================================
 
 The example shows sorting with str() to sort numeric values lexicographically.

integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java

@@ -183,7 +183,7 @@ public void testSortWithDescPushDownExplain() throws IOException {
     assertJsonEqualsIgnoreId(
         expected,
         explainQueryToString(
-            "source=opensearch-sql_test_index_account | sort age, - firstname desc | fields age,"
+            "source=opensearch-sql_test_index_account | sort age desc, firstname | fields age,"
                 + " firstname"));
   }
 

integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java

@@ -195,9 +195,9 @@ public void testSortWithDescMultipleFields() throws IOException {
     JSONObject result =
         executeQuery(
             String.format(
-                "source=%s | sort 4 age, - account_number desc | fields age, account_number",
+                "source=%s | sort 4 age desc, account_number desc | fields age, account_number",
                 TEST_INDEX_BANK));
-    verifyOrder(result, rows(39, 25), rows(36, 6), rows(36, 20), rows(34, 32));
+    verifyOrder(result, rows(39, 25), rows(36, 20), rows(36, 6), rows(34, 32));
   }
 
   @Test
@@ -241,7 +241,63 @@ public void testSortWithAscMultipleFields() throws IOException {
     JSONObject result =
         executeQuery(
             String.format(
-                "source=%s | sort age, account_number asc | fields age, account_number",
+                "source=%s | sort age asc, account_number asc | fields age, account_number",
+                TEST_INDEX_BANK));
+    verifyOrder(
+        result,
+        rows(28, 13),
+        rows(32, 1),
+        rows(33, 18),
+        rows(34, 32),
+        rows(36, 6),
+        rows(36, 20),
+        rows(39, 25));
+  }
+
+  @Test
+  public void testSortMixingPrefixWithDefault() throws IOException {
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | sort +age, account_number, -balance | fields age, account_number,"
+                    + " balance",
+                TEST_INDEX_BANK));
+    verifyOrder(
+        result,
+        rows(28, 13, 32838),
+        rows(32, 1, 39225),
+        rows(33, 18, 4180),
+        rows(34, 32, 48086),
+        rows(36, 6, 5686),
+        rows(36, 20, 16418),
+        rows(39, 25, 40540));
+  }
+
+  @Test
+  public void testSortMixingSuffixWithDefault() throws IOException {
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | sort age, account_number desc, balance | fields age,"
+                    + " account_number, balance",
+                TEST_INDEX_BANK));
+    verifyOrder(
+        result,
+        rows(28, 13, 32838),
+        rows(32, 1, 39225),
+        rows(33, 18, 4180),
+        rows(34, 32, 48086),
+        rows(36, 20, 16418),
+        rows(36, 6, 5686),
+        rows(39, 25, 40540));
+  }
+
+  @Test
+  public void testSortAllDefaultFields() throws IOException {
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | sort age, account_number | fields age, account_number",
                 TEST_INDEX_BANK));
     verifyOrder(
         result,

ppl/src/main/antlr/OpenSearchPPLParser.g4

@@ -251,7 +251,7 @@ dedupCommand
    ;
 
 sortCommand
-   : SORT (count = integerLiteral)? sortbyClause (ASC | A | DESC | D)?
+   : SORT (count = integerLiteral)? sortbyClause
    ;
 
 reverseCommand
@@ -819,7 +819,10 @@ fieldList
    ;
 
 sortField
-   : (PLUS | MINUS)? sortFieldExpression
+   : (PLUS | MINUS) sortFieldExpression (ASC | A | DESC | D)  # invalidMixedSortField
+   | (PLUS | MINUS) sortFieldExpression                  # prefixSortField
+   | sortFieldExpression (ASC | A | DESC | D)            # suffixSortField
+   | sortFieldExpression                                 # defaultSortField
    ;
 
 sortFieldExpression

ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java

@@ -7,7 +7,6 @@
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.emptyMap;
-import static org.opensearch.sql.ast.dsl.AstDSL.booleanLiteral;
 import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName;
 import static org.opensearch.sql.calcite.utils.CalciteUtils.getOnlyForCalciteException;
 import static org.opensearch.sql.lang.PPLLangSpec.PPL_SPEC;
@@ -586,29 +585,31 @@ public UnresolvedPlan visitBinCommand(BinCommandContext ctx) {
   @Override
   public UnresolvedPlan visitSortCommand(SortCommandContext ctx) {
     Integer count = ctx.count != null ? Math.max(0, Integer.parseInt(ctx.count.getText())) : 0;
-    boolean desc = ctx.DESC() != null || ctx.D() != null;
+
+    List<OpenSearchPPLParser.SortFieldContext> sortFieldContexts = ctx.sortbyClause().sortField();
+    validateSortDirectionSyntax(sortFieldContexts);
 
     List<Field> sortFields =
-        ctx.sortbyClause().sortField().stream()
+        sortFieldContexts.stream()
             .map(sort -> (Field) internalVisitExpression(sort))
-            .map(field -> desc ? reverseSortDirection(field) : field)
             .collect(Collectors.toList());
 
     return new Sort(count, sortFields);
   }
 
-  private Field reverseSortDirection(Field field) {
-    List<Argument> updatedArgs =
-        field.getFieldArgs().stream()
-            .map(
-                arg ->
-                    "asc".equals(arg.getArgName())
-                        ? new Argument(
-                            "asc", booleanLiteral(!((Boolean) arg.getValue().getValue())))
-                        : arg)
-            .collect(Collectors.toList());
+  private void validateSortDirectionSyntax(List<OpenSearchPPLParser.SortFieldContext> sortFields) {
+    boolean hasPrefix =
+        sortFields.stream()
+            .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.PrefixSortFieldContext);
+    boolean hasSuffix =
+        sortFields.stream()
+            .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.SuffixSortFieldContext);
 
-    return new Field(field.getField(), updatedArgs);
+    if (hasPrefix && hasSuffix) {
+      throw new SemanticCheckException(
+          "Cannot mix prefix (+/-) and suffix (asc/desc) sort direction syntax in the same"
+              + " command.");
+    }
   }
 
   /** Reverse command. */

ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java

@@ -31,6 +31,7 @@
 import org.opensearch.sql.calcite.plan.OpenSearchConstants;
 import org.opensearch.sql.common.antlr.SyntaxCheckException;
 import org.opensearch.sql.common.utils.StringUtils;
+import org.opensearch.sql.exception.SemanticCheckException;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.BinaryArithmeticContext;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.BooleanLiteralContext;
@@ -64,7 +65,6 @@
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PerFunctionCallContext;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.RenameFieldExpressionContext;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SingleFieldRelevanceFunctionContext;
-import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SpanClauseContext;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.StatsFunctionCallContext;
 import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.StringLiteralContext;
@@ -226,20 +226,54 @@ public UnresolvedExpression visitRenameFieldExpression(RenameFieldExpressionCont
   }
 
   @Override
-  public UnresolvedExpression visitSortField(SortFieldContext ctx) {
+  public UnresolvedExpression visitPrefixSortField(OpenSearchPPLParser.PrefixSortFieldContext ctx) {
+    return buildSortField(ctx.sortFieldExpression(), ctx);
+  }
+
+  @Override
+  public UnresolvedExpression visitSuffixSortField(OpenSearchPPLParser.SuffixSortFieldContext ctx) {
+    return buildSortField(ctx.sortFieldExpression(), ctx);
+  }
+
+  @Override
+  public UnresolvedExpression visitDefaultSortField(
+      OpenSearchPPLParser.DefaultSortFieldContext ctx) {
+    return buildSortField(ctx.sortFieldExpression(), ctx);
+  }
+
+  @Override
+  public UnresolvedExpression visitInvalidMixedSortField(
+      OpenSearchPPLParser.InvalidMixedSortFieldContext ctx) {
+    String prefixOperator = ctx.PLUS() != null ? "+" : "-";
+    String suffixKeyword =
+        ctx.ASC() != null ? "asc" : ctx.A() != null ? "a" : ctx.DESC() != null ? "desc" : "d";
+
+    throw new SemanticCheckException(
+        String.format(
+            "Cannot use both prefix (%s) and suffix (%s) sort direction syntax on the same field. "
+                + "Use either '%s%s' or '%s %s', not both.",
+            prefixOperator,
+            suffixKeyword,
+            prefixOperator,
+            ctx.sortFieldExpression().getText(),
+            ctx.sortFieldExpression().getText(),
+            suffixKeyword));
+  }
 
-    UnresolvedExpression fieldExpression =
-        visit(ctx.sortFieldExpression().fieldExpression().qualifiedName());
+  private Field buildSortField(
+      OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr,
+      OpenSearchPPLParser.SortFieldContext parentCtx) {
+    UnresolvedExpression fieldExpression = visit(sortFieldExpr.fieldExpression().qualifiedName());
 
-    if (ctx.sortFieldExpression().IP() != null) {
+    if (sortFieldExpr.IP() != null) {
       fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("ip"));
-    } else if (ctx.sortFieldExpression().NUM() != null) {
+    } else if (sortFieldExpr.NUM() != null) {
       fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("double"));
-    } else if (ctx.sortFieldExpression().STR() != null) {
+    } else if (sortFieldExpr.STR() != null) {
       fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("string"));
     }
     // AUTO() case uses the field expression as-is
-    return new Field(fieldExpression, ArgumentFactory.getArgumentList(ctx));
+    return new Field(fieldExpression, ArgumentFactory.getArgumentList(parentCtx));
   }
 
   @Override