From e4f55cfdcdab6ee24449083610156af429e2c521 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Thu, 23 Oct 2025 16:41:32 -0700 Subject: [PATCH 1/9] update asc/desc behavior for sort Signed-off-by: Ritvi Bhatt --- docs/user/ppl/cmd/sort.rst | 76 ++++++++++++------- ppl/src/main/antlr/OpenSearchPPLParser.g4 | 5 +- .../opensearch/sql/ppl/parser/AstBuilder.java | 30 ++++---- .../sql/ppl/parser/AstExpressionBuilder.java | 17 ++++- .../sql/ppl/utils/ArgumentFactory.java | 59 +++++++++++--- .../sql/ppl/parser/AstBuilderTest.java | 45 ++++++++++- 6 files changed, 174 insertions(+), 58 deletions(-) diff --git a/docs/user/ppl/cmd/sort.rst b/docs/user/ppl/cmd/sort.rst index c9750bab079..fd386f48c87 100644 --- a/docs/user/ppl/cmd/sort.rst +++ b/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 3b: 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 4: 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,6 +120,24 @@ PPL query:: | 1 | M | 32 | +----------------+--------+-----+ +Example 4b: 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 4: Sort by field include null value =========================================== @@ -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 7: Sort with specifying field type ================================== The example shows sorting with str() to sort numeric values lexicographically. diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index 35b81bbd348..d44ce6bcf5b 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/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,8 @@ fieldList ; sortField - : (PLUS | MINUS)? sortFieldExpression + : (PLUS | MINUS)? sortFieldExpression # prefixSortField + | sortFieldExpression (ASC | A | DESC | D) # suffixSortField ; sortFieldExpression diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java index f6ce4b10933..6c8517cf1bb 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java @@ -586,31 +586,33 @@ 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; + + // Check for mixed syntax usage and validate + List sortFieldContexts = ctx.sortbyClause().sortField(); + validateSortSyntax(sortFieldContexts); List 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 updatedArgs = - field.getFieldArgs().stream() - .map( - arg -> - "asc".equals(arg.getArgName()) - ? new Argument( - "asc", booleanLiteral(!((Boolean) arg.getValue().getValue()))) - : arg) - .collect(Collectors.toList()); + private void validateSortSyntax(List 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 syntax in the same command. " + + "Use either 'sort +field1, -field2' or 'sort field1 asc, field2 desc'."); + } } + /** Reverse command. */ @Override public UnresolvedPlan visitReverseCommand(OpenSearchPPLParser.ReverseCommandContext ctx) { 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 9850231463f..36253ff5066 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 @@ -226,8 +226,23 @@ public UnresolvedExpression visitRenameFieldExpression(RenameFieldExpressionCont } @Override - public UnresolvedExpression visitSortField(SortFieldContext ctx) { + public UnresolvedExpression visitPrefixSortField(OpenSearchPPLParser.PrefixSortFieldContext ctx) { + UnresolvedExpression fieldExpression = + visit(ctx.sortFieldExpression().fieldExpression().qualifiedName()); + + if (ctx.sortFieldExpression().IP() != null) { + fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("ip")); + } else if (ctx.sortFieldExpression().NUM() != null) { + fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("double")); + } else if (ctx.sortFieldExpression().STR() != null) { + fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("string")); + } + // AUTO() case uses the field expression as-is + return new Field(fieldExpression, ArgumentFactory.getArgumentList(ctx)); + } + @Override + public UnresolvedExpression visitSuffixSortField(OpenSearchPPLParser.SuffixSortFieldContext ctx) { UnresolvedExpression fieldExpression = visit(ctx.sortFieldExpression().fieldExpression().qualifiedName()); 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 e1d892fdfce..1288d998bc3 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 @@ -25,6 +25,8 @@ import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.IntegerLiteralContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.RareCommandContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext; +import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PrefixSortFieldContext; +import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SuffixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.TopCommandContext; /** Util class to get all arguments as a list from the PPL command. */ @@ -112,19 +114,58 @@ 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) { + if (ctx instanceof PrefixSortFieldContext) { + return getArgumentList((PrefixSortFieldContext) ctx); + } else if (ctx instanceof SuffixSortFieldContext) { + return getArgumentList((SuffixSortFieldContext) ctx); + } else { + throw new SemanticCheckException("Unsupported sort field context: " + ctx.getClass()); + } + } + + /** + * Get list of {@link Argument} for prefix sort field (+/- syntax). + * + * @param ctx PrefixSortFieldContext instance + * @return the list of arguments fetched from the prefix sort field + */ + public static List getArgumentList(PrefixSortFieldContext ctx) { return Arrays.asList( ctx.MINUS() != null ? new Argument("asc", new Literal(false, 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))); + getTypeArgument(ctx.sortFieldExpression())); + } + + /** + * Get list of {@link Argument} for suffix sort field (asc/desc syntax). + * + * @param ctx SuffixSortFieldContext instance + * @return the list of arguments fetched from the suffix sort field + */ + public static List getArgumentList(SuffixSortFieldContext ctx) { + return Arrays.asList( + (ctx.DESC() != null || ctx.D() != null) + ? new Argument("asc", new Literal(false, DataType.BOOLEAN)) + : new Argument("asc", new Literal(true, DataType.BOOLEAN)), + getTypeArgument(ctx.sortFieldExpression())); + } + + /** + * Helper method to get type argument from sortFieldExpression. + */ + private static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) { + if (ctx.AUTO() != null) { + return new Argument("type", new Literal("auto", DataType.STRING)); + } else if (ctx.IP() != null) { + return new Argument("type", new Literal("ip", DataType.STRING)); + } else if (ctx.NUM() != null) { + return new Argument("type", new Literal("num", DataType.STRING)); + } else if (ctx.STR() != null) { + return new Argument("type", new Literal("str", DataType.STRING)); + } else { + return 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 b9948e6abe2..ca8e113c4fd 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 @@ -80,6 +80,7 @@ import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.common.setting.Settings.Key; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.ppl.antlr.PPLSyntaxParser; import org.opensearch.sql.utils.SystemIndexUtils; @@ -520,9 +521,9 @@ public void testSortCommandWithD() { } @Test - public void testSortCommandWithMultipleFieldsAndDesc() { + public void testSortCommandWithMixedSuffixSyntax() { assertEqual( - "source=t | sort f1, -f2 desc", + "source=t | sort f1 desc, f2 asc", sort( relation("t"), field( @@ -556,9 +557,9 @@ public void testSortCommandWithA() { } @Test - public void testSortCommandWithMultipleFieldsAndAsc() { + public void testSortCommandWithMixedPrefixSyntax() { assertEqual( - "source=t | sort f1, f2 asc", + "source=t | sort +f1, -f2", sort( relation("t"), field( @@ -566,6 +567,42 @@ public void testSortCommandWithMultipleFieldsAndAsc() { exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), field( "f2", + exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))))); + } + + @Test + public void testSortCommandMixedSyntaxValidation() { + // Test that mixing +/- with asc/desc throws exception + assertThrows( + SemanticCheckException.class, + () -> plan("source=t | sort +f1, f2 desc")); + + // Test reverse mixing (asc/desc first, then +/-) + assertThrows( + SemanticCheckException.class, + () -> plan("source=t | sort f1 asc, +f2")); + + // Test multiple mixed cases + assertThrows( + SemanticCheckException.class, + () -> plan("source=t | sort -f1, f2 asc, f3")); + } + + @Test + public void testSortCommandMultipleSuffixSyntax() { + // Test multiple fields with asc/desc keywords + assertEqual( + "source=t | sort f1 asc, f2 desc, f3 asc", + sort( + relation("t"), + field( + "f1", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), + field( + "f2", + exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))), + field( + "f3", exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))))); } From 3619cc36f79d39753817a778ebbe9c5fe7baad76 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Thu, 23 Oct 2025 17:08:41 -0700 Subject: [PATCH 2/9] fix both sort directions on same field error Signed-off-by: Ritvi Bhatt --- docs/user/ppl/cmd/sort.rst | 14 ++-- ppl/src/main/antlr/OpenSearchPPLParser.g4 | 4 +- .../opensearch/sql/ppl/parser/AstBuilder.java | 10 +-- .../sql/ppl/parser/AstExpressionBuilder.java | 49 +++++++----- .../sql/ppl/utils/ArgumentFactory.java | 15 ++++ .../sql/ppl/parser/AstBuilderTest.java | 76 ++++++++++++++++++- 6 files changed, 132 insertions(+), 36 deletions(-) diff --git a/docs/user/ppl/cmd/sort.rst b/docs/user/ppl/cmd/sort.rst index fd386f48c87..12c2ad166ac 100644 --- a/docs/user/ppl/cmd/sort.rst +++ b/docs/user/ppl/cmd/sort.rst @@ -84,7 +84,7 @@ PPL query:: | 13 | 28 | +----------------+-----+ -Example 3b: Sort by one field in descending order (using desc) +Example 4: Sort by one field in descending order (using desc) ============================================================== The example show sort all the document with age field in descending order using the desc keyword. @@ -102,7 +102,7 @@ PPL query:: | 13 | 28 | +----------------+-----+ -Example 4: Sort by multiple fields (using +/-) +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. @@ -120,7 +120,7 @@ PPL query:: | 1 | M | 32 | +----------------+--------+-----+ -Example 4b: Sort by multiple fields (using asc/desc) +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. @@ -138,7 +138,7 @@ PPL query:: | 1 | M | 32 | +----------------+--------+-----+ -Example 4: Sort by field include null value +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. @@ -156,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. @@ -172,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. @@ -190,7 +190,7 @@ PPL query:: | 13 | 28 | +----------------+-----+ -Example 7: Sort with specifying field type +Example 10: Sort with specifying field type ================================== The example shows sorting with str() to sort numeric values lexicographically. diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index d44ce6bcf5b..50dbbb57843 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -819,8 +819,10 @@ fieldList ; sortField - : (PLUS | MINUS)? sortFieldExpression # prefixSortField + : (PLUS | MINUS) sortFieldExpression (ASC | A | DESC | D) # invalidMixedSortField + | (PLUS | MINUS) sortFieldExpression # prefixSortField | sortFieldExpression (ASC | A | DESC | D) # suffixSortField + | sortFieldExpression # defaultSortField ; sortFieldExpression diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java index 6c8517cf1bb..b975bc7eeab 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java @@ -587,9 +587,8 @@ public UnresolvedPlan visitBinCommand(BinCommandContext ctx) { public UnresolvedPlan visitSortCommand(SortCommandContext ctx) { Integer count = ctx.count != null ? Math.max(0, Integer.parseInt(ctx.count.getText())) : 0; - // Check for mixed syntax usage and validate List sortFieldContexts = ctx.sortbyClause().sortField(); - validateSortSyntax(sortFieldContexts); + validateSortDirectionSyntax(sortFieldContexts); List sortFields = sortFieldContexts.stream() @@ -599,20 +598,17 @@ public UnresolvedPlan visitSortCommand(SortCommandContext ctx) { return new Sort(count, sortFields); } - private void validateSortSyntax(List sortFields) { + private void validateSortDirectionSyntax(List sortFields) { boolean hasPrefix = sortFields.stream() .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.PrefixSortFieldContext); boolean hasSuffix = sortFields.stream() .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.SuffixSortFieldContext); if (hasPrefix && hasSuffix) { - throw new SemanticCheckException( - "Cannot mix prefix (+/-) and suffix (asc/desc) sort syntax in the same command. " + - "Use either 'sort +field1, -field2' or 'sort field1 asc, field2 desc'."); + throw new SemanticCheckException("Cannot mix prefix (+/-) and suffix (asc/desc) sort direction syntax in the same command."); } } - /** Reverse command. */ @Override public UnresolvedPlan visitReverseCommand(OpenSearchPPLParser.ReverseCommandContext ctx) { 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 36253ff5066..bcc00906128 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 @@ -30,6 +30,7 @@ import org.opensearch.sql.ast.tree.Trendline; import org.opensearch.sql.calcite.plan.OpenSearchConstants; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.common.utils.StringUtils; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.BinaryArithmeticContext; @@ -227,34 +228,48 @@ public UnresolvedExpression visitRenameFieldExpression(RenameFieldExpressionCont @Override public UnresolvedExpression visitPrefixSortField(OpenSearchPPLParser.PrefixSortFieldContext ctx) { - UnresolvedExpression fieldExpression = - visit(ctx.sortFieldExpression().fieldExpression().qualifiedName()); - - if (ctx.sortFieldExpression().IP() != null) { - fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("ip")); - } else if (ctx.sortFieldExpression().NUM() != null) { - fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("double")); - } else if (ctx.sortFieldExpression().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 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)); + } + + private Field buildSortField(OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr, + OpenSearchPPLParser.SortFieldContext parentCtx) { UnresolvedExpression fieldExpression = - visit(ctx.sortFieldExpression().fieldExpression().qualifiedName()); + 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 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 1288d998bc3..78dfefd771e 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 @@ -27,6 +27,7 @@ import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PrefixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SuffixSortFieldContext; +import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.DefaultSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.TopCommandContext; /** Util class to get all arguments as a list from the PPL command. */ @@ -118,6 +119,8 @@ public static List getArgumentList(SortFieldContext ctx) { return getArgumentList((PrefixSortFieldContext) ctx); } else if (ctx instanceof SuffixSortFieldContext) { return getArgumentList((SuffixSortFieldContext) ctx); + } else if (ctx instanceof DefaultSortFieldContext) { + return getArgumentList((DefaultSortFieldContext) ctx); } else { throw new SemanticCheckException("Unsupported sort field context: " + ctx.getClass()); } @@ -151,6 +154,18 @@ public static List getArgumentList(SuffixSortFieldContext ctx) { getTypeArgument(ctx.sortFieldExpression())); } + /** + * Get list of {@link Argument} for default sort field (no direction specified). + * + * @param ctx DefaultSortFieldContext instance + * @return the list of arguments fetched from the default sort field + */ + public static List getArgumentList(DefaultSortFieldContext ctx) { + return Arrays.asList( + new Argument("asc", new Literal(true, DataType.BOOLEAN)), + getTypeArgument(ctx.sortFieldExpression())); + } + /** * Helper method to get type argument from sortFieldExpression. */ 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 ca8e113c4fd..02c27143585 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 @@ -8,6 +8,7 @@ import static java.util.Collections.emptyList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; import static org.opensearch.sql.ast.dsl.AstDSL.agg; import static org.opensearch.sql.ast.dsl.AstDSL.aggregate; @@ -572,20 +573,33 @@ public void testSortCommandWithMixedPrefixSyntax() { @Test public void testSortCommandMixedSyntaxValidation() { - // Test that mixing +/- with asc/desc throws exception + // Test that mixing explicit +/- with explicit asc/desc throws exception assertThrows( SemanticCheckException.class, () -> plan("source=t | sort +f1, f2 desc")); - // Test reverse mixing (asc/desc first, then +/-) + // Test reverse mixing (explicit asc/desc with explicit +/-) assertThrows( SemanticCheckException.class, () -> plan("source=t | sort f1 asc, +f2")); - // Test multiple mixed cases + // Test mixing explicit prefix and suffix with default fields assertThrows( SemanticCheckException.class, - () -> plan("source=t | sort -f1, f2 asc, f3")); + () -> plan("source=t | sort -f1, f2 asc")); + } + + @Test + public void testSortCommandSingleFieldMixedSyntaxError() { + // Test descriptive error for mixing prefix and suffix on same field + SemanticCheckException exception = assertThrows( + SemanticCheckException.class, + () -> plan("source=t | sort -salary desc")); + + assertTrue("Error message should mention both prefix and suffix sort direction syntax", + exception.getMessage().contains("Cannot use both prefix (-) and suffix (desc) sort direction syntax")); + assertTrue("Error message should suggest alternatives", + exception.getMessage().contains("Use either '-salary' or 'salary desc'")); } @Test @@ -606,6 +620,60 @@ public void testSortCommandMultipleSuffixSyntax() { exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))))); } + @Test + public void testSortCommandMixingPrefixWithDefault() { + // Test mixing +/- syntax with unspecified direction (should work) + assertEqual( + "source=t | sort +f1, f2, -f3", + sort( + relation("t"), + field( + "f1", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), + field( + "f2", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), + field( + "f3", + exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))))); + } + + @Test + public void testSortCommandMixingSuffixWithDefault() { + // Test mixing asc/desc syntax with unspecified direction (should work) + assertEqual( + "source=t | sort f1, f2 desc, f3 asc", + sort( + relation("t"), + field( + "f1", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), + field( + "f2", + exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))), + field( + "f3", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))))); + } + + @Test + public void testSortCommandAllDefaultFields() { + // Test all fields with no direction specified (should default to ascending) + assertEqual( + "source=t | sort f1, f2, f3", + sort( + relation("t"), + field( + "f1", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), + field( + "f2", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), + field( + "f3", + exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))))); + } + @Test public void testEvalCommand() { assertEqual( From eb6536e5c2eb8e75d3f2d143667e3ccb0c0e08fa Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Thu, 23 Oct 2025 17:11:51 -0700 Subject: [PATCH 3/9] fix formatting Signed-off-by: Ritvi Bhatt --- .../opensearch/sql/ppl/parser/AstBuilder.java | 15 ++++--- .../sql/ppl/parser/AstExpressionBuilder.java | 40 ++++++++++--------- .../sql/ppl/utils/ArgumentFactory.java | 8 ++-- .../sql/ppl/parser/AstBuilderTest.java | 35 ++++++++-------- 4 files changed, 51 insertions(+), 47 deletions(-) diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java index b975bc7eeab..b7e33246027 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java +++ b/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; @@ -599,13 +598,17 @@ public UnresolvedPlan visitSortCommand(SortCommandContext ctx) { } private void validateSortDirectionSyntax(List sortFields) { - boolean hasPrefix = sortFields.stream() - .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.PrefixSortFieldContext); - boolean hasSuffix = sortFields.stream() - .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.SuffixSortFieldContext); + boolean hasPrefix = + sortFields.stream() + .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.PrefixSortFieldContext); + boolean hasSuffix = + sortFields.stream() + .anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.SuffixSortFieldContext); if (hasPrefix && hasSuffix) { - throw new SemanticCheckException("Cannot mix prefix (+/-) and suffix (asc/desc) sort direction syntax in the same command."); + throw new SemanticCheckException( + "Cannot mix prefix (+/-) and suffix (asc/desc) sort direction syntax in the same" + + " command."); } } 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 bcc00906128..4a5230d356e 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 @@ -30,8 +30,8 @@ import org.opensearch.sql.ast.tree.Trendline; import org.opensearch.sql.calcite.plan.OpenSearchConstants; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; 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; @@ -65,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; @@ -237,29 +236,34 @@ public UnresolvedExpression visitSuffixSortField(OpenSearchPPLParser.SuffixSortF } @Override - public UnresolvedExpression visitDefaultSortField(OpenSearchPPLParser.DefaultSortFieldContext ctx) { + public UnresolvedExpression visitDefaultSortField( + OpenSearchPPLParser.DefaultSortFieldContext ctx) { return buildSortField(ctx.sortFieldExpression(), ctx); } @Override - public UnresolvedExpression visitInvalidMixedSortField(OpenSearchPPLParser.InvalidMixedSortFieldContext ctx) { + 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"; + 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)); - } - - private Field buildSortField(OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr, - OpenSearchPPLParser.SortFieldContext parentCtx) { - UnresolvedExpression fieldExpression = - visit(sortFieldExpr.fieldExpression().qualifiedName()); + 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)); + } + + private Field buildSortField( + OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr, + OpenSearchPPLParser.SortFieldContext parentCtx) { + UnresolvedExpression fieldExpression = visit(sortFieldExpr.fieldExpression().qualifiedName()); if (sortFieldExpr.IP() != null) { fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("ip")); 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 78dfefd771e..cc203a724d0 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 @@ -21,13 +21,13 @@ import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.BooleanLiteralContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.DecimalLiteralContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.DedupCommandContext; +import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.DefaultSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.FieldsCommandContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.IntegerLiteralContext; +import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PrefixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.RareCommandContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext; -import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PrefixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SuffixSortFieldContext; -import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.DefaultSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.TopCommandContext; /** Util class to get all arguments as a list from the PPL command. */ @@ -166,9 +166,7 @@ public static List getArgumentList(DefaultSortFieldContext ctx) { getTypeArgument(ctx.sortFieldExpression())); } - /** - * Helper method to get type argument from sortFieldExpression. - */ + /** Helper method to get type argument from sortFieldExpression. */ private static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) { if (ctx.AUTO() != null) { return new Argument("type", new Literal("auto", DataType.STRING)); 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 02c27143585..34a7a0e26ea 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 @@ -568,37 +568,35 @@ public void testSortCommandWithMixedPrefixSyntax() { exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), field( "f2", - exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))))); + exprList( + argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))))); } @Test public void testSortCommandMixedSyntaxValidation() { // Test that mixing explicit +/- with explicit asc/desc throws exception - assertThrows( - SemanticCheckException.class, - () -> plan("source=t | sort +f1, f2 desc")); + assertThrows(SemanticCheckException.class, () -> plan("source=t | sort +f1, f2 desc")); // Test reverse mixing (explicit asc/desc with explicit +/-) - assertThrows( - SemanticCheckException.class, - () -> plan("source=t | sort f1 asc, +f2")); + assertThrows(SemanticCheckException.class, () -> plan("source=t | sort f1 asc, +f2")); // Test mixing explicit prefix and suffix with default fields - assertThrows( - SemanticCheckException.class, - () -> plan("source=t | sort -f1, f2 asc")); + assertThrows(SemanticCheckException.class, () -> plan("source=t | sort -f1, f2 asc")); } @Test public void testSortCommandSingleFieldMixedSyntaxError() { // Test descriptive error for mixing prefix and suffix on same field - SemanticCheckException exception = assertThrows( - SemanticCheckException.class, - () -> plan("source=t | sort -salary desc")); - - assertTrue("Error message should mention both prefix and suffix sort direction syntax", - exception.getMessage().contains("Cannot use both prefix (-) and suffix (desc) sort direction syntax")); - assertTrue("Error message should suggest alternatives", + SemanticCheckException exception = + assertThrows(SemanticCheckException.class, () -> plan("source=t | sort -salary desc")); + + assertTrue( + "Error message should mention both prefix and suffix sort direction syntax", + exception + .getMessage() + .contains("Cannot use both prefix (-) and suffix (desc) sort direction syntax")); + assertTrue( + "Error message should suggest alternatives", exception.getMessage().contains("Use either '-salary' or 'salary desc'")); } @@ -635,7 +633,8 @@ public void testSortCommandMixingPrefixWithDefault() { exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral()))), field( "f3", - exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))))); + exprList( + argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))))); } @Test From 2c8f5eade1e8ed33124b602e2d40df4221c30001 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Thu, 23 Oct 2025 17:37:02 -0700 Subject: [PATCH 4/9] fix tests Signed-off-by: Ritvi Bhatt --- .../org/opensearch/sql/ppl/ExplainIT.java | 2 +- .../org/opensearch/sql/ppl/SortCommandIT.java | 61 ++++++++++++++++++- .../sql/ppl/utils/ArgumentFactory.java | 4 +- .../sql/ppl/calcite/CalcitePPLBasicTest.java | 2 +- .../sql/ppl/parser/AstBuilderTest.java | 19 +----- 5 files changed, 66 insertions(+), 22 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java index 33ec09765d9..63ee0687b8f 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java +++ b/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")); } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java index be2cae9c843..bef363c11df 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java @@ -195,7 +195,7 @@ 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)); } @@ -253,4 +253,63 @@ public void testSortWithAscMultipleFields() throws IOException { rows(36, 20), rows(39, 25)); } + + @Test + public void testSortMixingPrefixWithDefault() throws IOException { + // Test mixing +/- with unspecified direction fields + 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, 14068), + rows(32, 1, 39225), + rows(33, 18, 4180), + rows(34, 32, 48086), + rows(36, 20, 16418), + rows(36, 6, 5686), + rows(39, 25, 32838)); + } + + @Test + public void testSortMixingSuffixWithDefault() throws IOException { + // Test mixing asc/desc with unspecified direction fields + JSONObject result = + executeQuery( + String.format( + "source=%s | sort age, account_number desc, balance asc | fields age," + + " account_number, balance", + TEST_INDEX_BANK)); + verifyOrder( + result, + rows(28, 13, 14068), + rows(32, 1, 39225), + rows(33, 18, 4180), + rows(34, 32, 48086), + rows(36, 20, 16418), + rows(36, 6, 5686), + rows(39, 25, 32838)); + } + + @Test + public void testSortAllDefaultFields() throws IOException { + // Test all fields with no direction specified (should default to ascending) + JSONObject result = + executeQuery( + String.format( + "source=%s | sort age, account_number | 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)); + } } 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 cc203a724d0..8f58e41f5e3 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 @@ -119,10 +119,8 @@ public static List getArgumentList(SortFieldContext ctx) { return getArgumentList((PrefixSortFieldContext) ctx); } else if (ctx instanceof SuffixSortFieldContext) { return getArgumentList((SuffixSortFieldContext) ctx); - } else if (ctx instanceof DefaultSortFieldContext) { - return getArgumentList((DefaultSortFieldContext) ctx); } else { - throw new SemanticCheckException("Unsupported sort field context: " + ctx.getClass()); + return getArgumentList((DefaultSortFieldContext) ctx); } } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java index f1a8f85d46b..46c33dcbe84 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java @@ -317,7 +317,7 @@ public void testSortWithCountZero() { @Test public void testSortWithDescReversal() { - String ppl = "source=EMP | sort + DEPTNO, - SAL desc"; + String ppl = "source=EMP | sort DEPTNO desc, SAL"; RelNode root = getRelNode(ppl); String expectedLogical = "LogicalSort(sort0=[$7], sort1=[$5], dir0=[DESC-nulls-last], dir1=[ASC-nulls-first])\n" 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 34a7a0e26ea..ab2a4ef9513 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 @@ -574,35 +574,25 @@ public void testSortCommandWithMixedPrefixSyntax() { @Test public void testSortCommandMixedSyntaxValidation() { - // Test that mixing explicit +/- with explicit asc/desc throws exception assertThrows(SemanticCheckException.class, () -> plan("source=t | sort +f1, f2 desc")); - - // Test reverse mixing (explicit asc/desc with explicit +/-) assertThrows(SemanticCheckException.class, () -> plan("source=t | sort f1 asc, +f2")); - - // Test mixing explicit prefix and suffix with default fields - assertThrows(SemanticCheckException.class, () -> plan("source=t | sort -f1, f2 asc")); } @Test public void testSortCommandSingleFieldMixedSyntaxError() { - // Test descriptive error for mixing prefix and suffix on same field SemanticCheckException exception = assertThrows(SemanticCheckException.class, () -> plan("source=t | sort -salary desc")); assertTrue( - "Error message should mention both prefix and suffix sort direction syntax", exception .getMessage() - .contains("Cannot use both prefix (-) and suffix (desc) sort direction syntax")); - assertTrue( - "Error message should suggest alternatives", - exception.getMessage().contains("Use either '-salary' or 'salary desc'")); + .contains( + "Cannot use both prefix (-) and suffix (desc) sort direction syntax on the same" + + " field")); } @Test public void testSortCommandMultipleSuffixSyntax() { - // Test multiple fields with asc/desc keywords assertEqual( "source=t | sort f1 asc, f2 desc, f3 asc", sort( @@ -620,7 +610,6 @@ public void testSortCommandMultipleSuffixSyntax() { @Test public void testSortCommandMixingPrefixWithDefault() { - // Test mixing +/- syntax with unspecified direction (should work) assertEqual( "source=t | sort +f1, f2, -f3", sort( @@ -639,7 +628,6 @@ public void testSortCommandMixingPrefixWithDefault() { @Test public void testSortCommandMixingSuffixWithDefault() { - // Test mixing asc/desc syntax with unspecified direction (should work) assertEqual( "source=t | sort f1, f2 desc, f3 asc", sort( @@ -657,7 +645,6 @@ public void testSortCommandMixingSuffixWithDefault() { @Test public void testSortCommandAllDefaultFields() { - // Test all fields with no direction specified (should default to ascending) assertEqual( "source=t | sort f1, f2, f3", sort( From aeb9fea7fc8c5d68c4cbf1da7b6284bbd673c504 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Thu, 23 Oct 2025 18:17:35 -0700 Subject: [PATCH 5/9] fix tests Signed-off-by: Ritvi Bhatt --- .../org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java index 46c33dcbe84..26783296f1c 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java @@ -327,7 +327,7 @@ public void testSortWithDescReversal() { @Test public void testSortWithDReversal() { - String ppl = "source=EMP | sort + DEPTNO, - SAL d"; + String ppl = "source=EMP | sort DEPTNO d, SAL"; RelNode root = getRelNode(ppl); String expectedLogical = "LogicalSort(sort0=[$7], sort1=[$5], dir0=[DESC-nulls-last], dir1=[ASC-nulls-first])\n" From 647cc901281ecce4577ac599fd22f4246b775cd9 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Thu, 23 Oct 2025 20:01:36 -0700 Subject: [PATCH 6/9] update failing tests Signed-off-by: Ritvi Bhatt --- .../org/opensearch/sql/ppl/SortCommandIT.java | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java index bef363c11df..b9d53c846d8 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java @@ -197,7 +197,7 @@ public void testSortWithDescMultipleFields() throws IOException { String.format( "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,7 @@ 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, @@ -256,7 +256,6 @@ public void testSortWithAscMultipleFields() throws IOException { @Test public void testSortMixingPrefixWithDefault() throws IOException { - // Test mixing +/- with unspecified direction fields JSONObject result = executeQuery( String.format( @@ -265,38 +264,36 @@ public void testSortMixingPrefixWithDefault() throws IOException { TEST_INDEX_BANK)); verifyOrder( result, - rows(28, 13, 14068), + 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, 32838)); + rows(36, 20, 16418), + rows(39, 25, 40540)); } @Test public void testSortMixingSuffixWithDefault() throws IOException { - // Test mixing asc/desc with unspecified direction fields JSONObject result = executeQuery( String.format( - "source=%s | sort age, account_number desc, balance asc | fields age," + "source=%s | sort age, account_number desc, balance | fields age," + " account_number, balance", TEST_INDEX_BANK)); verifyOrder( result, - rows(28, 13, 14068), + 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, 32838)); + rows(39, 25, 40540)); } @Test public void testSortAllDefaultFields() throws IOException { - // Test all fields with no direction specified (should default to ascending) JSONObject result = executeQuery( String.format( From 428c47b0bc312c82211123334c29533f144f1c47 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Tue, 28 Oct 2025 13:32:38 -0700 Subject: [PATCH 7/9] update docs and casting refactor Signed-off-by: Ritvi Bhatt --- docs/user/ppl/cmd/sort.rst | 2 +- .../sql/ppl/utils/ArgumentFactory.java | 20 +++++++++---------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/docs/user/ppl/cmd/sort.rst b/docs/user/ppl/cmd/sort.rst index 12c2ad166ac..e02a8fdae8d 100644 --- a/docs/user/ppl/cmd/sort.rst +++ b/docs/user/ppl/cmd/sort.rst @@ -141,7 +141,7 @@ PPL query:: 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:: 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 8f58e41f5e3..9982d0f790e 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 @@ -166,17 +166,15 @@ public static List getArgumentList(DefaultSortFieldContext ctx) { /** Helper method to get type argument from sortFieldExpression. */ private static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) { - if (ctx.AUTO() != null) { - return new Argument("type", new Literal("auto", DataType.STRING)); - } else if (ctx.IP() != null) { - return new Argument("type", new Literal("ip", DataType.STRING)); - } else if (ctx.NUM() != null) { - return new Argument("type", new Literal("num", DataType.STRING)); - } else if (ctx.STR() != null) { - return new Argument("type", new Literal("str", DataType.STRING)); - } else { - return new Argument("type", new Literal(null, DataType.NULL)); - } + if (ctx.AUTO() != null) return createTypeArgument("auto"); + if (ctx.IP() != null) return createTypeArgument("ip"); + if (ctx.NUM() != null) return createTypeArgument("num"); + if (ctx.STR() != null) return createTypeArgument("str"); + return createTypeArgument(null); + } + private static Argument createTypeArgument(String value) { + DataType dataType = value != null ? DataType.STRING : DataType.NULL; + return new Argument("type", new Literal(value, dataType)); } /** From 00ddb417fcce07687a9970c6a22ecf00a8823e4d Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Tue, 28 Oct 2025 13:49:32 -0700 Subject: [PATCH 8/9] fix formatting Signed-off-by: Ritvi Bhatt --- .../sql/ppl/utils/ArgumentFactory.java | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) 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 9982d0f790e..ae9452c85f0 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 @@ -26,7 +26,6 @@ import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.IntegerLiteralContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PrefixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.RareCommandContext; -import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SuffixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.TopCommandContext; @@ -108,22 +107,6 @@ public static List getArgumentList(DedupCommandContext ctx) { : new Argument("consecutive", new Literal(false, DataType.BOOLEAN))); } - /** - * Get list of {@link Argument}. - * - * @param ctx SortFieldContext instance - * @return the list of arguments fetched from the sort field in sort command - */ - public static List getArgumentList(SortFieldContext ctx) { - if (ctx instanceof PrefixSortFieldContext) { - return getArgumentList((PrefixSortFieldContext) ctx); - } else if (ctx instanceof SuffixSortFieldContext) { - return getArgumentList((SuffixSortFieldContext) ctx); - } else { - return getArgumentList((DefaultSortFieldContext) ctx); - } - } - /** * Get list of {@link Argument} for prefix sort field (+/- syntax). * @@ -172,6 +155,7 @@ private static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionC if (ctx.STR() != null) return createTypeArgument("str"); return createTypeArgument(null); } + private static Argument createTypeArgument(String value) { DataType dataType = value != null ? DataType.STRING : DataType.NULL; return new Argument("type", new Literal(value, dataType)); From 18e09f042a4370f1a2d1561f13a6f099ca988f54 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Tue, 28 Oct 2025 14:10:33 -0700 Subject: [PATCH 9/9] fix formatting Signed-off-by: Ritvi Bhatt --- .../sql/ppl/utils/ArgumentFactory.java | 38 ++++++++++++++----- 1 file changed, 28 insertions(+), 10 deletions(-) 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 ae9452c85f0..8f58e41f5e3 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 @@ -26,6 +26,7 @@ import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.IntegerLiteralContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PrefixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.RareCommandContext; +import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SuffixSortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.TopCommandContext; @@ -107,6 +108,22 @@ public static List getArgumentList(DedupCommandContext ctx) { : new Argument("consecutive", new Literal(false, DataType.BOOLEAN))); } + /** + * Get list of {@link Argument}. + * + * @param ctx SortFieldContext instance + * @return the list of arguments fetched from the sort field in sort command + */ + public static List getArgumentList(SortFieldContext ctx) { + if (ctx instanceof PrefixSortFieldContext) { + return getArgumentList((PrefixSortFieldContext) ctx); + } else if (ctx instanceof SuffixSortFieldContext) { + return getArgumentList((SuffixSortFieldContext) ctx); + } else { + return getArgumentList((DefaultSortFieldContext) ctx); + } + } + /** * Get list of {@link Argument} for prefix sort field (+/- syntax). * @@ -149,16 +166,17 @@ public static List getArgumentList(DefaultSortFieldContext ctx) { /** Helper method to get type argument from sortFieldExpression. */ private static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) { - if (ctx.AUTO() != null) return createTypeArgument("auto"); - if (ctx.IP() != null) return createTypeArgument("ip"); - if (ctx.NUM() != null) return createTypeArgument("num"); - if (ctx.STR() != null) return createTypeArgument("str"); - return createTypeArgument(null); - } - - private static Argument createTypeArgument(String value) { - DataType dataType = value != null ? DataType.STRING : DataType.NULL; - return new Argument("type", new Literal(value, dataType)); + if (ctx.AUTO() != null) { + return new Argument("type", new Literal("auto", DataType.STRING)); + } else if (ctx.IP() != null) { + return new Argument("type", new Literal("ip", DataType.STRING)); + } else if (ctx.NUM() != null) { + return new Argument("type", new Literal("num", DataType.STRING)); + } else if (ctx.STR() != null) { + return new Argument("type", new Literal("str", DataType.STRING)); + } else { + return new Argument("type", new Literal(null, DataType.NULL)); + } } /**