Refactor PPL sort desc/asc logic

[ritvibhatt]

Description

This PR refactors the PPL sort functionality in AstExpressionBuilder.java to improve code organization and removes unused code from ArgumentFactory.java.

Related Issues

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f600d3b)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Visibility Change The getTypeArgument method was changed from private to public. This exposes an internal helper method as part of the public API of ArgumentFactory. Verify whether this is intentional and if it should remain public or be package-private. public static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) {

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f600d3b

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid unnecessarily widening method visibility The getTypeArgument method was previously private and is now public, which exposes an internal helper method as part of the public API. This increases coupling and may lead to misuse. Consider keeping it package-private (no modifier) or using a more controlled access level unless external access is explicitly required. ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java [164-168] -public static Argument getTypeArgument(OpenSearchPPLParser.SortFieldExpressionContext ctx) { +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)); Suggestion importance[1-10]: 4 __ Why: The suggestion is valid - getTypeArgument was previously private and is now public, which does widen the API surface unnecessarily. However, the method needs to be at least package-accessible since it's called from AstExpressionBuilder.java in the same package, so package-private would work. This is a minor API design concern with moderate impact.	Low

---

Previous suggestions

Suggestions up to commit cf133f4

Category	Suggestion	Impact
General	Return mutable list instead of fixed-size list Arrays.asList() returns a fixed-size list backed by an array, which may cause issues if the Field constructor or any downstream code attempts to modify the list (e.g., add or remove elements). Consider using new ArrayList<>(Arrays.asList(...)) to return a mutable list, consistent with how other getArgumentList methods in ArgumentFactory may be consumed. ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java [339-343] List<Argument> arguments = - Arrays.asList( + new ArrayList<>(Arrays.asList( new Argument("asc", new Literal(ascending, DataType.BOOLEAN)), - ArgumentFactory.getTypeArgument(sortFieldExpr)); + ArgumentFactory.getTypeArgument(sortFieldExpr))); return new Field(fieldExpression, arguments); Suggestion importance[1-10]: 3 __ Why: While using new ArrayList<>() over Arrays.asList() is a valid defensive practice, there's no evidence in the PR that the Field constructor or downstream code modifies the list. The other getArgumentList methods in ArgumentFactory also use Arrays.asList() directly, so this change would be inconsistent with the existing pattern.	Low

Suggestions up to commit a606f60

Category	Suggestion	Impact
Possible issue	Guard against null type argument in list ArgumentFactory.getTypeArgument(sortFieldExpr) may return null if none of the type conditions match (e.g., when AUTO(), IP(), NUM(), and STR() are all null), which would insert a null element into the list and likely cause a NullPointerException downstream. You should add a null-check before adding the type argument, or ensure getTypeArgument always returns a non-null default. ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java [340-343] -List<Argument> arguments = Arrays.asList( - new Argument("asc", new Literal(ascending, DataType.BOOLEAN)), - ArgumentFactory.getTypeArgument(sortFieldExpr) -); +Argument typeArgument = ArgumentFactory.getTypeArgument(sortFieldExpr); +List<Argument> arguments = typeArgument != null + ? Arrays.asList( + new Argument("asc", new Literal(ascending, DataType.BOOLEAN)), + typeArgument) + : Collections.singletonList( + new Argument("asc", new Literal(ascending, DataType.BOOLEAN))); Suggestion importance[1-10]: 5 __ Why: The concern about getTypeArgument returning null is valid if none of the type conditions match. However, looking at the ArgumentFactory.getTypeArgument method, it likely has a default/else case that returns a non-null value (e.g., for AUTO() or a default type). The suggestion is reasonable but may be addressing a case that's already handled, making it a minor defensive improvement rather than a critical bug fix.	Low

[github-actions]

Persistent review updated to latest commit cf133f4

[github-actions]

Persistent review updated to latest commit f600d3b

[opensearch-trigger-bot]

This PR is stalled because it has been open for 2 weeks with no activity.