From a606f60be97d86bf279d3cc4ef87319380ef44f4 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Tue, 10 Mar 2026 14:56:11 -0700 Subject: [PATCH 1/3] refactor ppl sort logic Signed-off-by: Ritvi Bhatt --- .../sql/ppl/parser/AstExpressionBuilder.java | 17 ++++++++++++----- .../sql/ppl/utils/ArgumentFactory.java | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) 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 471c0c2f1c9..d77eeb283ca 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 @@ -288,18 +288,20 @@ public UnresolvedExpression visitRenameFieldExpression(RenameFieldExpressionCont @Override public UnresolvedExpression visitPrefixSortField(OpenSearchPPLParser.PrefixSortFieldContext ctx) { - return buildSortField(ctx.sortFieldExpression(), ctx); + boolean ascending = ctx.MINUS() == null; + return buildSortField(ctx.sortFieldExpression(), ascending); } @Override public UnresolvedExpression visitSuffixSortField(OpenSearchPPLParser.SuffixSortFieldContext ctx) { - return buildSortField(ctx.sortFieldExpression(), ctx); + boolean ascending = (ctx.DESC() == null && ctx.D() == null); + return buildSortField(ctx.sortFieldExpression(), ascending); } @Override public UnresolvedExpression visitDefaultSortField( OpenSearchPPLParser.DefaultSortFieldContext ctx) { - return buildSortField(ctx.sortFieldExpression(), ctx); + return buildSortField(ctx.sortFieldExpression(), true); } @Override @@ -323,7 +325,7 @@ public UnresolvedExpression visitInvalidMixedSortField( private Field buildSortField( OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr, - OpenSearchPPLParser.SortFieldContext parentCtx) { + boolean ascending) { UnresolvedExpression fieldExpression = visit(sortFieldExpr.fieldExpression().qualifiedName()); if (sortFieldExpr.IP() != null) { @@ -334,7 +336,12 @@ private Field buildSortField( fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("string")); } // AUTO() case uses the field expression as-is - return new Field(fieldExpression, ArgumentFactory.getArgumentList(parentCtx)); + + List arguments = Arrays.asList( + new Argument("asc", new Literal(ascending, DataType.BOOLEAN)), + ArgumentFactory.getTypeArgument(sortFieldExpr) + ); + return new Field(fieldExpression, arguments); } @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 72090e2f069..c04358ef35d 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 @@ -211,7 +211,7 @@ public static List getArgumentList(DefaultSortFieldContext ctx) { } /** Helper method to get type argument from sortFieldExpression. */ - private static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) { + public static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) { if (ctx.AUTO() != null) { return new Argument("type", new Literal("auto", DataType.STRING)); } else if (ctx.IP() != null) { From cf133f49018fe3597d1f6c43c206b523da738c79 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Tue, 10 Mar 2026 16:07:01 -0700 Subject: [PATCH 2/3] apply spotless Signed-off-by: Ritvi Bhatt --- .../sql/ppl/parser/AstExpressionBuilder.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) 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 d77eeb283ca..e61ff2222d0 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 @@ -324,8 +324,7 @@ public UnresolvedExpression visitInvalidMixedSortField( } private Field buildSortField( - OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr, - boolean ascending) { + OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr, boolean ascending) { UnresolvedExpression fieldExpression = visit(sortFieldExpr.fieldExpression().qualifiedName()); if (sortFieldExpr.IP() != null) { @@ -337,10 +336,10 @@ private Field buildSortField( } // AUTO() case uses the field expression as-is - List arguments = Arrays.asList( - new Argument("asc", new Literal(ascending, DataType.BOOLEAN)), - ArgumentFactory.getTypeArgument(sortFieldExpr) - ); + List arguments = + Arrays.asList( + new Argument("asc", new Literal(ascending, DataType.BOOLEAN)), + ArgumentFactory.getTypeArgument(sortFieldExpr)); return new Field(fieldExpression, arguments); } From f600d3b9e7805c72bbc5ae584e6c927ca61ba373 Mon Sep 17 00:00:00 2001 From: Ritvi Bhatt Date: Tue, 10 Mar 2026 16:22:58 -0700 Subject: [PATCH 3/3] remove unused code Signed-off-by: Ritvi Bhatt --- .../sql/ppl/parser/AstExpressionBuilder.java | 2 +- .../sql/ppl/utils/ArgumentFactory.java | 60 ++----------------- 2 files changed, 6 insertions(+), 56 deletions(-) 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 e61ff2222d0..715627a1323 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 @@ -338,7 +338,7 @@ private Field buildSortField( List arguments = Arrays.asList( - new Argument("asc", new Literal(ascending, DataType.BOOLEAN)), + ArgumentFactory.createSortDirectionArgument(ascending), ArgumentFactory.getTypeArgument(sortFieldExpr)); return new Field(fieldExpression, arguments); } 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 c04358ef35d..2cdc702b785 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,14 +27,10 @@ import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.ChartCommandContext; 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.EventstatsCommandContext; 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.SortFieldContext; import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.StreamstatsCommandContext; -import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SuffixSortFieldContext; import org.opensearch.sql.ppl.parser.AstExpressionBuilder; /** Util class to get all arguments as a list from the PPL command. */ @@ -155,59 +151,13 @@ public static List getArgumentList(DedupCommandContext ctx) { } /** - * Get list of {@link Argument}. + * Creates an "asc" argument for sort field direction. * - * @param ctx SortFieldContext instance - * @return the list of arguments fetched from the sort field in sort command + * @param ascending true for ascending sort, false for descending + * @return Argument representing the sort direction */ - 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). - * - * @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)), - 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())); - } - - /** - * 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())); + public static Argument createSortDirectionArgument(boolean ascending) { + return new Argument("asc", new Literal(ascending, DataType.BOOLEAN)); } /** Helper method to get type argument from sortFieldExpression. */