Skip to content

Commit 448c42a

Browse files
ritvibhattritvibhatt
authored
Fix asc/desc keyword behavior for sort command (#4651)
1 parent 0dd5949 commit 448c42a

File tree

9 files changed

+333
-76
lines changed

9 files changed

+333
-76
lines changed

‎docs/user/ppl/cmd/sort.rst‎

Lines changed: 52 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,16 @@ Description
1616

1717
Syntax
1818
============
19-
sort [count] <[+|-] sort-field>... [asc|a|desc|d]
19+
sort [count] <[+|-] sort-field | sort-field [asc|a|desc|d]>...
2020

2121

2222
* 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.
2323
* [+|-]: 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.
24+
* [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.
2425
* 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.
25-
* [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.
26+
27+
.. note::
28+
You cannot mix +/- and asc/desc in the same sort command. Choose one approach for all fields in a single sort command.
2629

2730

2831
Example 1: Sort by one field
@@ -63,10 +66,10 @@ PPL query::
6366
+----------------+-----+
6467

6568

66-
Example 3: Sort by one field in descending order
67-
================================================
69+
Example 3: Sort by one field in descending order (using -)
70+
==========================================================
6871

69-
The example show sort all the document with age field in descending order.
72+
The example show sort all the document with age field in descending order using the - operator.
7073

7174
PPL query::
7275

@@ -81,10 +84,28 @@ PPL query::
8184
| 13 | 28 |
8285
+----------------+-----+
8386

84-
Example 4: Sort by multiple field
85-
=============================
87+
Example 4: Sort by one field in descending order (using desc)
88+
==============================================================
8689

87-
The example show sort all the document with gender field in ascending order and age field in descending.
90+
The example show sort all the document with age field in descending order using the desc keyword.
91+
92+
PPL query::
93+
94+
os> source=accounts | sort age desc | fields account_number, age;
95+
fetched rows / total rows = 4/4
96+
+----------------+-----+
97+
| account_number | age |
98+
|----------------+-----|
99+
| 6 | 36 |
100+
| 18 | 33 |
101+
| 1 | 32 |
102+
| 13 | 28 |
103+
+----------------+-----+
104+
105+
Example 5: Sort by multiple fields (using +/-)
106+
==============================================
107+
108+
The example show sort all the document with gender field in ascending order and age field in descending using +/- operators.
88109

89110
PPL query::
90111

@@ -99,10 +120,28 @@ PPL query::
99120
| 1 | M | 32 |
100121
+----------------+--------+-----+
101122

102-
Example 4: Sort by field include null value
123+
Example 6: Sort by multiple fields (using asc/desc)
124+
====================================================
125+
126+
The example show sort all the document with gender field in ascending order and age field in descending using asc/desc keywords.
127+
128+
PPL query::
129+
130+
os> source=accounts | sort gender asc, age desc | fields account_number, gender, age;
131+
fetched rows / total rows = 4/4
132+
+----------------+--------+-----+
133+
| account_number | gender | age |
134+
|----------------+--------+-----|
135+
| 13 | F | 28 |
136+
| 6 | M | 36 |
137+
| 18 | M | 33 |
138+
| 1 | M | 32 |
139+
+----------------+--------+-----+
140+
141+
Example 7: Sort by field include null value
103142
===========================================
104143

105-
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.
144+
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.
106145

107146
PPL query::
108147

@@ -117,7 +156,7 @@ PPL query::
117156
| Quility |
118157
+----------+
119158

120-
Example 5: Specify the number of sorted documents to return
159+
Example 8: Specify the number of sorted documents to return
121160
============================================================
122161

123162
The example shows sorting all the document and returning 2 documents.
@@ -133,7 +172,7 @@ PPL query::
133172
| 1 | 32 |
134173
+----------------+-----+
135174

136-
Example 6: Sort with desc modifier
175+
Example 9: Sort with desc modifier
137176
===================================
138177

139178
The example shows sorting with the desc modifier to reverse sort order.
@@ -151,26 +190,7 @@ PPL query::
151190
| 13 | 28 |
152191
+----------------+-----+
153192

154-
Example 7: Sort by multiple fields with desc modifier
155-
======================================================
156-
157-
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.
158-
159-
PPL query::
160-
161-
os> source=accounts | sort gender, -age desc | fields account_number, gender, age;
162-
fetched rows / total rows = 4/4
163-
+----------------+--------+-----+
164-
| account_number | gender | age |
165-
|----------------+--------+-----|
166-
| 1 | M | 32 |
167-
| 18 | M | 33 |
168-
| 6 | M | 36 |
169-
| 13 | F | 28 |
170-
+----------------+--------+-----+
171-
172-
173-
Example 8: Sort with specifying field type
193+
Example 10: Sort with specifying field type
174194
==================================
175195

176196
The example shows sorting with str() to sort numeric values lexicographically.

‎integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ public void testSortWithDescPushDownExplain() throws IOException {
183183
assertJsonEqualsIgnoreId(
184184
expected,
185185
explainQueryToString(
186-
"source=opensearch-sql_test_index_account | sort age, - firstname desc | fields age,"
186+
"source=opensearch-sql_test_index_account | sort age desc, firstname | fields age,"
187187
+ " firstname"));
188188
}
189189

‎integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java‎

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -195,9 +195,9 @@ public void testSortWithDescMultipleFields() throws IOException {
195195
JSONObject result =
196196
executeQuery(
197197
String.format(
198-
"source=%s | sort 4 age, - account_number desc | fields age, account_number",
198+
"source=%s | sort 4 age desc, account_number desc | fields age, account_number",
199199
TEST_INDEX_BANK));
200-
verifyOrder(result, rows(39, 25), rows(36, 6), rows(36, 20), rows(34, 32));
200+
verifyOrder(result, rows(39, 25), rows(36, 20), rows(36, 6), rows(34, 32));
201201
}
202202

203203
@Test
@@ -241,7 +241,63 @@ public void testSortWithAscMultipleFields() throws IOException {
241241
JSONObject result =
242242
executeQuery(
243243
String.format(
244-
"source=%s | sort age, account_number asc | fields age, account_number",
244+
"source=%s | sort age asc, account_number asc | fields age, account_number",
245+
TEST_INDEX_BANK));
246+
verifyOrder(
247+
result,
248+
rows(28, 13),
249+
rows(32, 1),
250+
rows(33, 18),
251+
rows(34, 32),
252+
rows(36, 6),
253+
rows(36, 20),
254+
rows(39, 25));
255+
}
256+
257+
@Test
258+
public void testSortMixingPrefixWithDefault() throws IOException {
259+
JSONObject result =
260+
executeQuery(
261+
String.format(
262+
"source=%s | sort +age, account_number, -balance | fields age, account_number,"
263+
+ " balance",
264+
TEST_INDEX_BANK));
265+
verifyOrder(
266+
result,
267+
rows(28, 13, 32838),
268+
rows(32, 1, 39225),
269+
rows(33, 18, 4180),
270+
rows(34, 32, 48086),
271+
rows(36, 6, 5686),
272+
rows(36, 20, 16418),
273+
rows(39, 25, 40540));
274+
}
275+
276+
@Test
277+
public void testSortMixingSuffixWithDefault() throws IOException {
278+
JSONObject result =
279+
executeQuery(
280+
String.format(
281+
"source=%s | sort age, account_number desc, balance | fields age,"
282+
+ " account_number, balance",
283+
TEST_INDEX_BANK));
284+
verifyOrder(
285+
result,
286+
rows(28, 13, 32838),
287+
rows(32, 1, 39225),
288+
rows(33, 18, 4180),
289+
rows(34, 32, 48086),
290+
rows(36, 20, 16418),
291+
rows(36, 6, 5686),
292+
rows(39, 25, 40540));
293+
}
294+
295+
@Test
296+
public void testSortAllDefaultFields() throws IOException {
297+
JSONObject result =
298+
executeQuery(
299+
String.format(
300+
"source=%s | sort age, account_number | fields age, account_number",
245301
TEST_INDEX_BANK));
246302
verifyOrder(
247303
result,

‎ppl/src/main/antlr/OpenSearchPPLParser.g4‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ dedupCommand
251251
;
252252

253253
sortCommand
254-
: SORT (count = integerLiteral)? sortbyClause (ASC | A | DESC | D)?
254+
: SORT (count = integerLiteral)? sortbyClause
255255
;
256256

257257
reverseCommand
@@ -811,7 +811,10 @@ fieldList
811811
;
812812

813813
sortField
814-
: (PLUS | MINUS)? sortFieldExpression
814+
: (PLUS | MINUS) sortFieldExpression (ASC | A | DESC | D) # invalidMixedSortField
815+
| (PLUS | MINUS) sortFieldExpression # prefixSortField
816+
| sortFieldExpression (ASC | A | DESC | D) # suffixSortField
817+
| sortFieldExpression # defaultSortField
815818
;
816819

817820
sortFieldExpression

‎ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
import static java.util.Collections.emptyList;
99
import static java.util.Collections.emptyMap;
10-
import static org.opensearch.sql.ast.dsl.AstDSL.booleanLiteral;
1110
import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName;
1211
import static org.opensearch.sql.calcite.utils.CalciteUtils.getOnlyForCalciteException;
1312
import static org.opensearch.sql.lang.PPLLangSpec.PPL_SPEC;
@@ -586,29 +585,31 @@ public UnresolvedPlan visitBinCommand(BinCommandContext ctx) {
586585
@Override
587586
public UnresolvedPlan visitSortCommand(SortCommandContext ctx) {
588587
Integer count = ctx.count != null ? Math.max(0, Integer.parseInt(ctx.count.getText())) : 0;
589-
boolean desc = ctx.DESC() != null || ctx.D() != null;
588+
589+
List<OpenSearchPPLParser.SortFieldContext> sortFieldContexts = ctx.sortbyClause().sortField();
590+
validateSortDirectionSyntax(sortFieldContexts);
590591

591592
List<Field> sortFields =
592-
ctx.sortbyClause().sortField().stream()
593+
sortFieldContexts.stream()
593594
.map(sort -> (Field) internalVisitExpression(sort))
594-
.map(field -> desc ? reverseSortDirection(field) : field)
595595
.collect(Collectors.toList());
596596

597597
return new Sort(count, sortFields);
598598
}
599599

600-
private Field reverseSortDirection(Field field) {
601-
List<Argument> updatedArgs =
602-
field.getFieldArgs().stream()
603-
.map(
604-
arg ->
605-
"asc".equals(arg.getArgName())
606-
? new Argument(
607-
"asc", booleanLiteral(!((Boolean) arg.getValue().getValue())))
608-
: arg)
609-
.collect(Collectors.toList());
600+
private void validateSortDirectionSyntax(List<OpenSearchPPLParser.SortFieldContext> sortFields) {
601+
boolean hasPrefix =
602+
sortFields.stream()
603+
.anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.PrefixSortFieldContext);
604+
boolean hasSuffix =
605+
sortFields.stream()
606+
.anyMatch(sortField -> sortField instanceof OpenSearchPPLParser.SuffixSortFieldContext);
610607

611-
return new Field(field.getField(), updatedArgs);
608+
if (hasPrefix && hasSuffix) {
609+
throw new SemanticCheckException(
610+
"Cannot mix prefix (+/-) and suffix (asc/desc) sort direction syntax in the same"
611+
+ " command.");
612+
}
612613
}
613614

614615
/** Reverse command. */

‎ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.opensearch.sql.calcite.plan.OpenSearchConstants;
3232
import org.opensearch.sql.common.antlr.SyntaxCheckException;
3333
import org.opensearch.sql.common.utils.StringUtils;
34+
import org.opensearch.sql.exception.SemanticCheckException;
3435
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser;
3536
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.BinaryArithmeticContext;
3637
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.BooleanLiteralContext;
@@ -64,7 +65,6 @@
6465
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PerFunctionCallContext;
6566
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.RenameFieldExpressionContext;
6667
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SingleFieldRelevanceFunctionContext;
67-
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext;
6868
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SpanClauseContext;
6969
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.StatsFunctionCallContext;
7070
import org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.StringLiteralContext;
@@ -226,20 +226,54 @@ public UnresolvedExpression visitRenameFieldExpression(RenameFieldExpressionCont
226226
}
227227

228228
@Override
229-
public UnresolvedExpression visitSortField(SortFieldContext ctx) {
229+
public UnresolvedExpression visitPrefixSortField(OpenSearchPPLParser.PrefixSortFieldContext ctx) {
230+
return buildSortField(ctx.sortFieldExpression(), ctx);
231+
}
232+
233+
@Override
234+
public UnresolvedExpression visitSuffixSortField(OpenSearchPPLParser.SuffixSortFieldContext ctx) {
235+
return buildSortField(ctx.sortFieldExpression(), ctx);
236+
}
237+
238+
@Override
239+
public UnresolvedExpression visitDefaultSortField(
240+
OpenSearchPPLParser.DefaultSortFieldContext ctx) {
241+
return buildSortField(ctx.sortFieldExpression(), ctx);
242+
}
243+
244+
@Override
245+
public UnresolvedExpression visitInvalidMixedSortField(
246+
OpenSearchPPLParser.InvalidMixedSortFieldContext ctx) {
247+
String prefixOperator = ctx.PLUS() != null ? "+" : "-";
248+
String suffixKeyword =
249+
ctx.ASC() != null ? "asc" : ctx.A() != null ? "a" : ctx.DESC() != null ? "desc" : "d";
250+
251+
throw new SemanticCheckException(
252+
String.format(
253+
"Cannot use both prefix (%s) and suffix (%s) sort direction syntax on the same field. "
254+
+ "Use either '%s%s' or '%s %s', not both.",
255+
prefixOperator,
256+
suffixKeyword,
257+
prefixOperator,
258+
ctx.sortFieldExpression().getText(),
259+
ctx.sortFieldExpression().getText(),
260+
suffixKeyword));
261+
}
230262

231-
UnresolvedExpression fieldExpression =
232-
visit(ctx.sortFieldExpression().fieldExpression().qualifiedName());
263+
private Field buildSortField(
264+
OpenSearchPPLParser.SortFieldExpressionContext sortFieldExpr,
265+
OpenSearchPPLParser.SortFieldContext parentCtx) {
266+
UnresolvedExpression fieldExpression = visit(sortFieldExpr.fieldExpression().qualifiedName());
233267

234-
if (ctx.sortFieldExpression().IP() != null) {
268+
if (sortFieldExpr.IP() != null) {
235269
fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("ip"));
236-
} else if (ctx.sortFieldExpression().NUM() != null) {
270+
} else if (sortFieldExpr.NUM() != null) {
237271
fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("double"));
238-
} else if (ctx.sortFieldExpression().STR() != null) {
272+
} else if (sortFieldExpr.STR() != null) {
239273
fieldExpression = new Cast(fieldExpression, AstDSL.stringLiteral("string"));
240274
}
241275
// AUTO() case uses the field expression as-is
242-
return new Field(fieldExpression, ArgumentFactory.getArgumentList(ctx));
276+
return new Field(fieldExpression, ArgumentFactory.getArgumentList(parentCtx));
243277
}
244278

245279
@Override

0 commit comments

Comments
 (0)