Revert sort syntax changes

Signed-off-by: currantw <[email protected]>
opensearch-project · Dec 18, 2024 · 6355b2d · 6355b2d
1 parent ac9c300
commit 6355b2d
Show file tree

Hide file tree

Showing 9 changed files with 123 additions and 29 deletions.
diff --git a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java
@@ -431,7 +431,7 @@ public static List<Argument> sortOptions() {
   }
 
   public static List<Argument> defaultSortFieldArgs() {
-    return exprList(argument("asc", booleanLiteral(true)));
+    return exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()));
   }
 
   public static Span span(UnresolvedExpression field, UnresolvedExpression value, SpanUnit unit) {

diff --git a/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java
@@ -688,8 +688,9 @@ public void constructArrayOfIPsReturnsAll() {
     final String ipv6String = "2001:db7::ff00:42:8329";
 
     assertEquals(
-        new ExprCollectionValue(List.of(ipValue("1.2.3.4"), ipValue("2001:db7::ff00:42:8329"))),
-        tupleValue("{\"ipV\":[" + "\"1.2.3.4\"," + "\"2001:db7::ff00:42:8329\"" + "]}").get("ipV"));
+        new ExprCollectionValue(List.of(ipValue(ipv4String), ipValue(ipv6String))),
+        tupleValue(String.format("{\"%s\":[\"%s\",\"%s\"]}", fieldIp, ipv4String, ipv6String))
+            .get(fieldIp));
   }
 
   @Test

diff --git a/ppl/src/main/antlr/OpenSearchPPLLexer.g4 b/ppl/src/main/antlr/OpenSearchPPLLexer.g4
@@ -52,6 +52,12 @@ WITH:                               'WITH';
 // CLAUSE KEYWORDS
 SORTBY:                             'SORTBY';
 
+// SORT FIELD KEYWORDS
+// TODO #3180: Fix broken sort functionality
+AUTO:                               'AUTO';
+STR:                                'STR';
+NUM:                                'NUM';
+
 // TRENDLINE KEYWORDS
 SMA:                                'SMA';
 

diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4
@@ -363,7 +363,15 @@ wcFieldList
    ;
 
 sortField
-   : (PLUS | MINUS)? fieldExpression
+   : (PLUS | MINUS)? sortFieldExpression
+   ;
+
+sortFieldExpression
+   : fieldExpression
+   | AUTO LT_PRTHS fieldExpression RT_PRTHS
+   | STR LT_PRTHS fieldExpression RT_PRTHS
+   | IP LT_PRTHS fieldExpression RT_PRTHS
+   | NUM LT_PRTHS fieldExpression RT_PRTHS
    ;
 
 fieldExpression
@@ -890,6 +898,11 @@ keywordsCanBeId
    | DATASOURCES
    // CLAUSEKEYWORDS
    | SORTBY
+   // SORT FIELD KEYWORDS
+   | AUTO
+   | STR
+   | IP
+   | NUM
    // ARGUMENT KEYWORDS
    | KEEPEMPTY
    | CONSECUTIVE

diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
@@ -161,8 +161,11 @@ public UnresolvedExpression visitWcFieldExpression(WcFieldExpressionContext ctx)
 
   @Override
   public UnresolvedExpression visitSortField(SortFieldContext ctx) {
+
+    // TODO #3180: Fix broken sort functionality
     return new Field(
-        visit(ctx.fieldExpression().qualifiedName()), ArgumentFactory.getArgumentList(ctx));
+        visit(ctx.sortFieldExpression().fieldExpression().qualifiedName()),
+        ArgumentFactory.getArgumentList(ctx));
   }
 
   /** Aggregation function. */

diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java b/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java
@@ -87,10 +87,19 @@ public static List<Argument> getArgumentList(DedupCommandContext ctx) {
    * @return the list of arguments fetched from the sort field in sort command
    */
   public static List<Argument> getArgumentList(SortFieldContext ctx) {
-    return List.of(
+    return Arrays.asList(
         ctx.MINUS() != null
             ? new Argument("asc", new Literal(false, DataType.BOOLEAN))
-            : new Argument("asc", new Literal(true, DataType.BOOLEAN)));
+            : new Argument("asc", new Literal(true, DataType.BOOLEAN)),
+        ctx.sortFieldExpression().AUTO() != null
+            ? new Argument("type", new Literal("auto", DataType.STRING))
+            : ctx.sortFieldExpression().IP() != null
+                ? new Argument("type", new Literal("ip", DataType.STRING))
+                : ctx.sortFieldExpression().NUM() != null
+                    ? new Argument("type", new Literal("num", DataType.STRING))
+                    : ctx.sortFieldExpression().STR() != null
+                        ? new Argument("type", new Literal("str", DataType.STRING))
+                        : new Argument("type", new Literal(null, DataType.NULL)));
   }
 
   /**

diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java
@@ -29,6 +29,7 @@
 import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral;
 import static org.opensearch.sql.ast.dsl.AstDSL.let;
 import static org.opensearch.sql.ast.dsl.AstDSL.map;
+import static org.opensearch.sql.ast.dsl.AstDSL.nullLiteral;
 import static org.opensearch.sql.ast.dsl.AstDSL.parse;
 import static org.opensearch.sql.ast.dsl.AstDSL.projectWithArg;
 import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName;
@@ -432,7 +433,9 @@ public void testSortCommandWithOptions() {
         "source=t | sort - f1, + f2",
         sort(
             relation("t"),
-            field("f1", exprList(argument("asc", booleanLiteral(false)))),
+            field(
+                "f1",
+                exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))),
             field("f2", defaultSortFieldArgs())));
   }
 
@@ -712,7 +715,11 @@ public void testTrendlineSort() {
         "source=t | trendline sort test_field sma(5, test_field)",
         trendline(
             relation("t"),
-            Optional.of(field("test_field", argument("asc", booleanLiteral(true)))),
+            Optional.of(
+                field(
+                    "test_field",
+                    argument("asc", booleanLiteral(true)),
+                    argument("type", nullLiteral()))),
             computation(5, field("test_field"), "test_field_trendline", SMA)));
   }
 
@@ -722,7 +729,11 @@ public void testTrendlineSortDesc() {
         "source=t | trendline sort - test_field sma(5, test_field)",
         trendline(
             relation("t"),
-            Optional.of(field("test_field", argument("asc", booleanLiteral(false)))),
+            Optional.of(
+                field(
+                    "test_field",
+                    argument("asc", booleanLiteral(false)),
+                    argument("type", nullLiteral()))),
             computation(5, field("test_field"), "test_field_trendline", SMA)));
   }
 
@@ -732,7 +743,11 @@ public void testTrendlineSortAsc() {
         "source=t | trendline sort + test_field sma(5, test_field)",
         trendline(
             relation("t"),
-            Optional.of(field("test_field", argument("asc", booleanLiteral(true)))),
+            Optional.of(
+                field(
+                    "test_field",
+                    argument("asc", booleanLiteral(true)),
+                    argument("type", nullLiteral()))),
             computation(5, field("test_field"), "test_field_trendline", SMA)));
   }
 

diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java
@@ -32,6 +32,7 @@
 import static org.opensearch.sql.ast.dsl.AstDSL.let;
 import static org.opensearch.sql.ast.dsl.AstDSL.longLiteral;
 import static org.opensearch.sql.ast.dsl.AstDSL.not;
+import static org.opensearch.sql.ast.dsl.AstDSL.nullLiteral;
 import static org.opensearch.sql.ast.dsl.AstDSL.or;
 import static org.opensearch.sql.ast.dsl.AstDSL.projectWithArg;
 import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName;
@@ -324,25 +325,68 @@ public void testFieldExpr() {
     assertEqual("source=t | sort + f", sort(relation("t"), field("f", defaultSortFieldArgs())));
   }
 
-  @Test
-  public void testSortFieldWithPlusKeyword() {
-    assertEqual(
-        "source=t | sort + f",
-        sort(relation("t"), field("f", argument("asc", booleanLiteral(true)))));
-  }
-
   @Test
   public void testSortFieldWithMinusKeyword() {
     assertEqual(
         "source=t | sort - f",
-        sort(relation("t"), field("f", argument("asc", booleanLiteral(false)))));
+        sort(
+            relation("t"),
+            field("f", argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))));
   }
 
   @Test
   public void testSortFieldWithBackticks() {
     assertEqual("source=t | sort `f`", sort(relation("t"), field("f", defaultSortFieldArgs())));
   }
 
+  @Test
+  public void testSortFieldWithAutoKeyword() {
+    assertEqual(
+        "source=t | sort auto(f)",
+        sort(
+            relation("t"),
+            field(
+                "f",
+                argument("asc", booleanLiteral(true)),
+                argument("type", stringLiteral("auto")))));
+  }
+
+  @Test
+  public void testSortFieldWithIpKeyword() {
+    assertEqual(
+        "source=t | sort ip(f)",
+        sort(
+            relation("t"),
+            field(
+                "f",
+                argument("asc", booleanLiteral(true)),
+                argument("type", stringLiteral("ip")))));
+  }
+
+  @Test
+  public void testSortFieldWithNumKeyword() {
+    assertEqual(
+        "source=t | sort num(f)",
+        sort(
+            relation("t"),
+            field(
+                "f",
+                argument("asc", booleanLiteral(true)),
+                argument("type", stringLiteral("num")))));
+  }
+
+  @Test
+  public void testSortFieldWithStrKeyword() {
+    assertEqual(
+        "source=t | sort str(f)",
+        sort(
+            relation("t"),
+            field(
+                "f",
+                argument("asc", booleanLiteral(true)),
+                argument("type", stringLiteral("str")))));
+  }
+
   @Test
   public void testAggFuncCallExpr() {
     assertEqual(

diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java
@@ -81,18 +81,21 @@ public void testDedupCommandDefaultArgument() {
   }
 
   @Test
-  public void testSortCommand() {
-    assertEqual(
-        "source=t | sort field0",
-        sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(true))))));
-
-    assertEqual(
-        "source=t | sort + field0",
-        sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(true))))));
+  public void testSortCommandDefaultArgument() {
+    assertEqual("source=t | sort field0", "source=t | sort field0");
+  }
 
+  @Test
+  public void testSortFieldArgument() {
     assertEqual(
-        "source=t | sort - field0",
-        sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(false))))));
+        "source=t | sort - auto(field0)",
+        sort(
+            relation("t"),
+            field(
+                "field0",
+                exprList(
+                    argument("asc", booleanLiteral(false)),
+                    argument("type", stringLiteral("auto"))))));
   }
 
   @Test