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 index 62752efe52..d9956609ec 100644 --- 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 sortOptions() { } public static List 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/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index 5b9166e2b5..89dfd4dbdb 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/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 index 3b2fce4f1c..053ec530db 100644 --- 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 index fd18b06aa6..27f7e4014b 100644 --- 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 index d18d74e40a..5a7522683a 100644 --- 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 index cd79ccef79..f89ecf9c6e 100644 --- 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 getArgumentList(DedupCommandContext ctx) { * @return the list of arguments fetched from the sort field in sort command */ public static List 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 index 0d2541c8f9..c6f4ed2044 100644 --- 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 index e4cee77013..fbb25549ab 100644 --- 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,18 +325,13 @@ 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 @@ -343,6 +339,54 @@ 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 index f736921877..761dbe2997 100644 --- 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