Skip to content

Commit 90449b6

Browse files
committed
fix unit tests
Signed-off-by: Lantao Jin <ltjin@amazon.com>
1 parent e7e4bb5 commit 90449b6

4 files changed

Lines changed: 43 additions & 11 deletions

File tree

docs/user/ppl/cmd/stats.rst

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ stats [bucket_nullable=bool] <aggregation>... [by-clause]
3939

4040
* aggregation: mandatory. A aggregation function. The argument of aggregation must be field.
4141

42-
* bucket_nullable: optional. Controls whether the stats command includes null buckets in group-by aggregations. When set to ``false``, the aggregation ignores records where the group-by field is null, resulting in faster performance by excluding null bucket. The default value of ``bucket_nullable`` is determined by ``plugins.ppl.syntax.legacy.preferred``:
42+
* bucket_nullable: optional (since 3.3.0). Controls whether the stats command includes null buckets in group-by aggregations. When set to ``false``, the aggregation ignores records where the group-by field is null, resulting in faster performance by excluding null bucket. The default value of ``bucket_nullable`` is determined by ``plugins.ppl.syntax.legacy.preferred``:
4343

4444
* When ``plugins.ppl.syntax.legacy.preferred=true``, ``bucket_nullable`` defaults to ``true``
4545
* When ``plugins.ppl.syntax.legacy.preferred=false``, ``bucket_nullable`` defaults to ``false``
@@ -692,9 +692,11 @@ PPL query::
692692
Example 15: Ignore null bucket
693693
==============================
694694

695+
Note: This argument requires version 3.3.0 or above.
696+
695697
PPL query::
696698

697-
os> source=accounts | stats bucket_nullable=false count() as cnt by email;
699+
PPL> source=accounts | stats bucket_nullable=false count() as cnt by email;
698700
fetched rows / total rows = 3/3
699701
+-----+-----------------------+
700702
| cnt | email |

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ public AggregationQueryBuilder(ExpressionSerializer serializer) {
8383
new NoBucketAggregationParser(metrics.getRight()));
8484
} else if (groupByList.size() == 1 && !bucketNullable) {
8585
// one bucket, use values source bucket builder for getting better performance
86+
// TODO for multiple buckets, use MultiTermsAggregationBuilder
8687
return Pair.of(
8788
Collections.singletonList(
8889
bucketBuilder.build(groupByList.getFirst()).subAggregations(metrics.getLeft())),

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,13 @@
1111
import static org.opensearch.sql.opensearch.storage.script.aggregation.AggregationQueryBuilder.AGGREGATION_BUCKET_SIZE;
1212

1313
import java.util.List;
14+
import java.util.stream.Collectors;
1415
import org.opensearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
1516
import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval;
1617
import org.opensearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
18+
import org.opensearch.search.aggregations.bucket.terms.MultiTermsAggregationBuilder;
1719
import org.opensearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
20+
import org.opensearch.search.aggregations.support.MultiTermsValuesSourceConfig;
1821
import org.opensearch.search.aggregations.support.ValueType;
1922
import org.opensearch.search.aggregations.support.ValuesSourceAggregationBuilder;
2023
import org.opensearch.sql.ast.expression.SpanUnit;
@@ -32,7 +35,7 @@ public BucketAggregationBuilder(ExpressionSerializer serializer) {
3235
this.helper = new AggregationBuilderHelper(serializer);
3336
}
3437

35-
/** Build the list of ValuesSourceAggregationBuilder. */
38+
/** Build the ValuesSourceAggregationBuilder. */
3639
public ValuesSourceAggregationBuilder<?> build(NamedExpression expr) {
3740
if (expr.getDelegated() instanceof SpanExpression) {
3841
SpanExpression spanExpr = (SpanExpression) expr.getDelegated();
@@ -55,6 +58,34 @@ public ValuesSourceAggregationBuilder<?> build(NamedExpression expr) {
5558
}
5659
}
5760

61+
/** Build the MultiTermsAggregationBuilder. */
62+
public MultiTermsAggregationBuilder buildMultipleTerms(List<NamedExpression> exprs) {
63+
MultiTermsAggregationBuilder sourceBuilder =
64+
new MultiTermsAggregationBuilder(
65+
exprs.stream().map(NamedExpression::getName).collect(Collectors.joining("_")));
66+
sourceBuilder.terms(
67+
exprs.stream()
68+
.map(
69+
expr -> {
70+
MultiTermsValuesSourceConfig.Builder config =
71+
new MultiTermsValuesSourceConfig.Builder();
72+
config.setFieldName(expr.getName());
73+
// Time types values are converted to LONG in ExpressionAggregationScript::execute
74+
if ((expr.getDelegated().type() instanceof OpenSearchDateType
75+
&& List.of(TIMESTAMP, TIME, DATE)
76+
.contains(
77+
((OpenSearchDateType) expr.getDelegated().type())
78+
.getExprCoreType()))
79+
|| List.of(TIMESTAMP, TIME, DATE).contains(expr.getDelegated().type())) {
80+
config.setUserValueTypeHint(ValueType.LONG);
81+
}
82+
return config.build();
83+
})
84+
.toList());
85+
sourceBuilder.size(AGGREGATION_BUCKET_SIZE);
86+
return sourceBuilder;
87+
}
88+
5889
public static ValuesSourceAggregationBuilder<?> buildHistogram(
5990
String name, String field, Double value, SpanUnit unit) {
6091
switch (unit) {

opensearch/src/test/java/org/opensearch/sql/opensearch/request/AggregateAnalyzerTest.java

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,7 @@ void analyze_aggCall_simple() throws ExpressionNotAnalyzableException {
138138
List.of(countCall, avgCall, sumCall, minCall, maxCall), ImmutableBitSet.of());
139139
Project project = createMockProject(List.of(0));
140140
Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> result =
141-
AggregateAnalyzer.analyze(
142-
aggregate, project, rowType, fieldTypes, outputFields, null, true);
141+
AggregateAnalyzer.analyze(aggregate, project, rowType, fieldTypes, outputFields, null);
143142
assertEquals(
144143
"[{\"cnt\":{\"value_count\":{\"field\":\"_index\"}}},"
145144
+ " {\"avg\":{\"avg\":{\"field\":\"a\"}}},"
@@ -220,8 +219,7 @@ void analyze_aggCall_extended() throws ExpressionNotAnalyzableException {
220219
List.of(varSampCall, varPopCall, stddevSampCall, stddevPopCall), ImmutableBitSet.of());
221220
Project project = createMockProject(List.of(0));
222221
Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> result =
223-
AggregateAnalyzer.analyze(
224-
aggregate, project, rowType, fieldTypes, outputFields, null, true);
222+
AggregateAnalyzer.analyze(aggregate, project, rowType, fieldTypes, outputFields, null);
225223
assertEquals(
226224
"[{\"var_samp\":{\"extended_stats\":{\"field\":\"a\",\"sigma\":2.0}}},"
227225
+ " {\"var_pop\":{\"extended_stats\":{\"field\":\"a\",\"sigma\":2.0}}},"
@@ -260,8 +258,7 @@ void analyze_groupBy() throws ExpressionNotAnalyzableException {
260258
Aggregate aggregate = createMockAggregate(List.of(aggCall), ImmutableBitSet.of(0, 1));
261259
Project project = createMockProject(List.of(0, 1));
262260
Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> result =
263-
AggregateAnalyzer.analyze(
264-
aggregate, project, rowType, fieldTypes, outputFields, null, true);
261+
AggregateAnalyzer.analyze(aggregate, project, rowType, fieldTypes, outputFields, null);
265262

266263
assertEquals(
267264
"[{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":["
@@ -304,7 +301,7 @@ void analyze_aggCall_TextWithoutKeyword() {
304301
ExpressionNotAnalyzableException.class,
305302
() ->
306303
AggregateAnalyzer.analyze(
307-
aggregate, project, rowType, fieldTypes, List.of("sum"), null, true));
304+
aggregate, project, rowType, fieldTypes, List.of("sum"), null));
308305
assertEquals("[field] must not be null: [sum]", exception.getCause().getMessage());
309306
}
310307

@@ -331,14 +328,15 @@ void analyze_groupBy_TextWithoutKeyword() {
331328
ExpressionNotAnalyzableException.class,
332329
() ->
333330
AggregateAnalyzer.analyze(
334-
aggregate, project, rowType, fieldTypes, outputFields, null, true));
331+
aggregate, project, rowType, fieldTypes, outputFields, null));
335332
assertEquals("[field] must not be null", exception.getCause().getMessage());
336333
}
337334

338335
private Aggregate createMockAggregate(List<AggregateCall> calls, ImmutableBitSet groups) {
339336
Aggregate agg = mock(Aggregate.class);
340337
when(agg.getGroupSet()).thenReturn(groups);
341338
when(agg.getAggCallList()).thenReturn(calls);
339+
when(agg.getHints()).thenReturn(ImmutableList.of());
342340
return agg;
343341
}
344342

0 commit comments

Comments
 (0)