Skip to content

Commit 7bef202

Browse files
yuancuyuancu
committed
Change grammart from limit=top 10 to limit=top10
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
1 parent 154cbc4 commit 7bef202

7 files changed

Lines changed: 29 additions & 21 deletions

File tree

‎core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,8 @@ private boolean isCountField(RexCall call) {
915915
private Pair<List<RexNode>, List<AggCall>> aggregateWithTrimming(
916916
List<UnresolvedExpression> groupExprList,
917917
List<UnresolvedExpression> aggExprList,
918-
CalcitePlanContext context) {
918+
CalcitePlanContext context,
919+
boolean hintBucketNonNull) {
919920
Pair<List<RexNode>, List<AggCall>> resolved =
920921
resolveAttributesForAggregation(groupExprList, aggExprList, context);
921922
List<RexNode> resolvedGroupByList = resolved.getLeft();
@@ -1019,6 +1020,7 @@ private Pair<List<RexNode>, List<AggCall>> aggregateWithTrimming(
10191020
List<String> intendedGroupKeyAliases = getGroupKeyNamesAfterAggregation(reResolved.getLeft());
10201021
context.relBuilder.aggregate(
10211022
context.relBuilder.groupKey(reResolved.getLeft()), reResolved.getRight());
1023+
if (hintBucketNonNull) addIgnoreNullBucketHintToAggregate(context);
10221024
// During aggregation, Calcite projects both input dependencies and output group-by fields.
10231025
// When names conflict, Calcite adds numeric suffixes (e.g., "value0").
10241026
// Apply explicit renaming to restore the intended aliases.
@@ -1141,10 +1143,7 @@ private void visitAggregation(Aggregation node, CalcitePlanContext context, bool
11411143
}
11421144

11431145
Pair<List<RexNode>, List<AggCall>> aggregationAttributes =
1144-
aggregateWithTrimming(groupExprList, aggExprList, context);
1145-
if (toAddHintsOnAggregate) {
1146-
addIgnoreNullBucketHintToAggregate(context);
1147-
}
1146+
aggregateWithTrimming(groupExprList, aggExprList, context, toAddHintsOnAggregate);
11481147

11491148
// schema reordering
11501149
List<RexNode> outputFields = context.relBuilder.fields();
@@ -1896,11 +1895,7 @@ public RelNode visitRareTopN(RareTopN node, CalcitePlanContext context) {
18961895
.map(context.relBuilder::isNotNull)
18971896
.toList());
18981897
}
1899-
aggregateWithTrimming(groupExprList, aggExprList, context);
1900-
1901-
if (toAddHintsOnAggregate) {
1902-
addIgnoreNullBucketHintToAggregate(context);
1903-
}
1898+
aggregateWithTrimming(groupExprList, aggExprList, context, toAddHintsOnAggregate);
19041899

19051900
// 2. add a window column
19061901
List<RexNode> partitionKeys = rexVisitor.analyze(node.getGroupExprList(), context);
@@ -2262,7 +2257,7 @@ public RelNode visitTimechart(
22622257
try {
22632258
// Step 1: Initial aggregation - IMPORTANT: order is [spanExpr, byField]
22642259
groupExprList = Arrays.asList(spanExpr, byField);
2265-
aggregateWithTrimming(groupExprList, List.of(node.getAggregateFunction()), context);
2260+
aggregateWithTrimming(groupExprList, List.of(node.getAggregateFunction()), context, false);
22662261

22672262
// First rename the timestamp field (2nd to last) to @timestamp
22682263
List<String> fieldNames = context.relBuilder.peek().getRowType().getFieldNames();

‎integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteChartCommandIT.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ public void testChartLimitTopWithUseOther() throws IOException {
211211
JSONObject result =
212212
executeQuery(
213213
String.format(
214-
"source=%s | chart limit=top 2 useother=true otherstr='max_among_other'"
214+
"source=%s | chart limit=top2 useother=true otherstr='max_among_other'"
215215
+ " max(severityNumber) over flags by severityText",
216216
TEST_INDEX_OTEL_LOGS));
217217
verifySchema(
@@ -232,7 +232,7 @@ public void testChartLimitBottomWithUseOther() throws IOException {
232232
JSONObject result =
233233
executeQuery(
234234
String.format(
235-
"source=%s | chart limit=bottom 2 useother=false otherstr='other_small_not_shown'"
235+
"source=%s | chart limit=bottom2 useother=false otherstr='other_small_not_shown'"
236236
+ " max(severityNumber) over flags by severityText",
237237
TEST_INDEX_OTEL_LOGS));
238238
verifySchema(
@@ -248,7 +248,7 @@ public void testChartLimitTopWithMinAgg() throws IOException {
248248
JSONObject result =
249249
executeQuery(
250250
String.format(
251-
"source=%s | chart limit=top 2 min(severityNumber) over flags by severityText",
251+
"source=%s | chart limit=top2 min(severityNumber) over flags by severityText",
252252
TEST_INDEX_OTEL_LOGS));
253253
verifySchema(
254254
result,

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ COST: 'COST';
9494
EXTENDED: 'EXTENDED';
9595
OVERRIDE: 'OVERRIDE';
9696
OVERWRITE: 'OVERWRITE';
97-
BOTTOM: 'BOTTOM';
97+
TOP_K: 'TOP'[0-9]+;
98+
BOTTOM_K: 'BOTTOM'[0-9]+;
9899

99100
// SORT FIELD KEYWORDS
100101
// TODO #3180: Fix broken sort functionality

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,8 @@ chartCommand
264264
;
265265

266266
chartOptions
267-
: LIMIT EQUAL (TOP | BOTTOM)? integerLiteral
267+
: LIMIT EQUAL integerLiteral
268+
| LIMIT EQUAL (TOP_K | BOTTOM_K)
268269
| USEOTHER EQUAL booleanLiteral
269270
| OTHERSTR EQUAL stringLiteral
270271
| USENULL EQUAL booleanLiteral

‎ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,20 @@ public static List<Argument> getArgumentList(ChartCommandContext ctx) {
185185
List<Argument> arguments = new ArrayList<>();
186186
for (var optionCtx : ctx.chartOptions()) {
187187
if (optionCtx.LIMIT() != null) {
188-
arguments.add(new Argument("limit", getArgumentValue(optionCtx.integerLiteral())));
188+
Literal limit;
189+
if (optionCtx.integerLiteral() != null) {
190+
limit = getArgumentValue(optionCtx.integerLiteral());
191+
} else {
192+
limit =
193+
AstDSL.intLiteral(
194+
Integer.parseInt(
195+
(optionCtx.TOP_K() != null ? optionCtx.TOP_K() : optionCtx.BOTTOM_K())
196+
.getText()
197+
.replaceAll("[^0-9-]", "")));
198+
}
199+
arguments.add(new Argument("limit", limit));
189200
// not specified | top presents -> true; bottom presents -> false
190-
arguments.add(new Argument("top", AstDSL.booleanLiteral(optionCtx.BOTTOM() == null)));
201+
arguments.add(new Argument("top", AstDSL.booleanLiteral(optionCtx.BOTTOM_K() == null)));
191202
} else if (optionCtx.USEOTHER() != null) {
192203
arguments.add(new Argument("useother", getArgumentValue(optionCtx.booleanLiteral())));
193204
} else if (optionCtx.OTHERSTR() != null) {

‎ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1535,7 +1535,7 @@ public void testChartCommandWithOptions() {
15351535
@Test
15361536
public void testChartCommandWithAllOptions() {
15371537
assertEqual(
1538-
"source=t | chart limit=5 useother=false otherstr='OTHER' usenull=true nullstr='NULL'"
1538+
"source=t | chart limit=top5 useother=false otherstr='OTHER' usenull=true nullstr='NULL'"
15391539
+ " avg(balance) by gender",
15401540
Chart.builder()
15411541
.child(relation("t"))
@@ -1555,7 +1555,7 @@ public void testChartCommandWithAllOptions() {
15551555
@Test
15561556
public void testChartCommandWithBottomLimit() {
15571557
assertEqual(
1558-
"source=t | chart limit=bottom 3 count() by category",
1558+
"source=t | chart limit=bottom3 count() by category",
15591559
Chart.builder()
15601560
.child(relation("t"))
15611561
.columnSplit(alias("category", field("category")))

‎ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ public void testChartCommandArguments() {
125125
@Test
126126
public void testChartCommandBottomArguments() {
127127
assertEqual(
128-
"source=t | chart limit=bottom 3 count() by status",
128+
"source=t | chart limit=bottom3 count() by status",
129129
Chart.builder()
130130
.child(relation("t"))
131131
.columnSplit(alias("status", field("status")))

0 commit comments

Comments
 (0)