Skip to content

Commit cea4893

Browse files
opensearch-trigger-bot[bot]github-actions[bot]
opensearch-trigger-bot[bot]
and
github-actions[bot]
authored
Fix asc/desc keyword behavior for sort command (opensearch-project#4651) (opensearch-project#4691)
(cherry picked from commit 448c42a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 631e895 commit cea4893

9 files changed

Lines changed: 333 additions & 76 deletions

File tree

‎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
@@ -220,7 +220,7 @@ public void testSortWithDescPushDownExplain() throws IOException {
220220
assertJsonEqualsIgnoreId(
221221
expected,
222222
explainQueryToString(
223-
"source=opensearch-sql_test_index_account | sort age, - firstname desc | fields age,"
223+
"source=opensearch-sql_test_index_account | sort age desc, firstname | fields age,"
224224
+ " firstname"));
225225
}
226226

‎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
@@ -196,9 +196,9 @@ public void testSortWithDescMultipleFields() throws IOException {
196196
JSONObject result =
197197
executeQuery(
198198
String.format(
199-
"source=%s | sort 4 age, - account_number desc | fields age, account_number",
199+
"source=%s | sort 4 age desc, account_number desc | fields age, account_number",
200200
TEST_INDEX_BANK));
201-
verifyOrder(result, rows(39, 25), rows(36, 6), rows(36, 20), rows(34, 32));
201+
verifyOrder(result, rows(39, 25), rows(36, 20), rows(36, 6), rows(34, 32));
202202
}
203203

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

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ dedupCommand
255255
;
256256

257257
sortCommand
258-
: SORT (count = integerLiteral)? sortbyClause (ASC | A | DESC | D)?
258+
: SORT (count = integerLiteral)? sortbyClause
259259
;
260260

261261
reverseCommand
@@ -827,7 +827,10 @@ fieldList
827827
;
828828

829829
sortField
830-
: (PLUS | MINUS)? sortFieldExpression
830+
: (PLUS | MINUS) sortFieldExpression (ASC | A | DESC | D) # invalidMixedSortField
831+
| (PLUS | MINUS) sortFieldExpression # prefixSortField
832+
| sortFieldExpression (ASC | A | DESC | D) # suffixSortField
833+
| sortFieldExpression # defaultSortField
831834
;
832835

833836
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;
@@ -587,29 +586,31 @@ public UnresolvedPlan visitBinCommand(BinCommandContext ctx) {
587586
@Override
588587
public UnresolvedPlan visitSortCommand(SortCommandContext ctx) {
589588
Integer count = ctx.count != null ? Math.max(0, Integer.parseInt(ctx.count.getText())) : 0;
590-
boolean desc = ctx.DESC() != null || ctx.D() != null;
589+
590+
List<OpenSearchPPLParser.SortFieldContext> sortFieldContexts = ctx.sortbyClause().sortField();
591+
validateSortDirectionSyntax(sortFieldContexts);
591592

592593
List<Field> sortFields =
593-
ctx.sortbyClause().sortField().stream()
594+
sortFieldContexts.stream()
594595
.map(sort -> (Field) internalVisitExpression(sort))
595-
.map(field -> desc ? reverseSortDirection(field) : field)
596596
.collect(Collectors.toList());
597597

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

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

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

615616
/** 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)