Skip to content

Commit e646285

Browse files
committed
Simplify CalciteExplainIT and add UT for AggregateAnalyzer
Signed-off-by: Kai Huang <ahkcs@amazon.com> # Conflicts: # opensearch/src/test/java/org/opensearch/sql/opensearch/request/AggregateAnalyzerTest.java
1 parent 28b464d commit e646285

File tree

5 files changed

+134
-20
lines changed

5 files changed

+134
-20
lines changed

core/src/main/java/org/opensearch/sql/calcite/udf/udaf/FirstAggFunction.java

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public FirstAccumulator add(FirstAccumulator acc, Object... values) {
2828
Object candidateValue = values[0];
2929
// Only accept the first non-null value encountered
3030
// Skip null values to find the first actual value
31-
if (candidateValue != null && !acc.hasValue()) {
31+
if (candidateValue != null) {
3232
acc.setValue(candidateValue);
3333
}
3434
return acc;
@@ -50,10 +50,6 @@ public synchronized void setValue(Object value) {
5050
}
5151
}
5252

53-
public boolean hasValue() {
54-
return hasValue;
55-
}
56-
5753
@Override
5854
public Object value(Object... argList) {
5955
return first;

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

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -418,19 +418,6 @@ public void testExplainOnFirstLast() throws IOException {
418418
TEST_INDEX_BANK)));
419419
}
420420

421-
// Only for Calcite
422-
@Test
423-
public void testExplainOnFirstLastDifferentFields() throws IOException {
424-
String expected = loadExpectedPlan("explain_first_last_different_fields.json");
425-
assertJsonEqualsIgnoreId(
426-
expected,
427-
explainQueryToString(
428-
String.format(
429-
"source=%s | stats first(account_number) as first_account,"
430-
+ " last(balance) as last_balance, first(age) as first_age",
431-
TEST_INDEX_BANK)));
432-
}
433-
434421
@Test
435422
public void testListAggregationExplain() throws IOException {
436423
String expected = loadExpectedPlan("explain_list_aggregation.json");

integ-test/src/test/resources/expectedOutput/calcite/explain_first_last_different_fields.json

Lines changed: 0 additions & 1 deletion
This file was deleted.

integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_first_last_different_fields.json

Lines changed: 0 additions & 1 deletion
This file was deleted.

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

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.junit.jupiter.api.Test;
4343
import org.opensearch.search.aggregations.AggregationBuilder;
4444
import org.opensearch.sql.data.type.ExprType;
45+
import org.opensearch.sql.expression.function.PPLBuiltinOperators;
4546
import org.opensearch.sql.opensearch.data.type.OpenSearchDataType;
4647
import org.opensearch.sql.opensearch.data.type.OpenSearchDataType.MappingType;
4748
import org.opensearch.sql.opensearch.request.AggregateAnalyzer.ExpressionNotAnalyzableException;
@@ -52,6 +53,7 @@
5253
import org.opensearch.sql.opensearch.response.agg.OpenSearchAggregationResponseParser;
5354
import org.opensearch.sql.opensearch.response.agg.SingleValueParser;
5455
import org.opensearch.sql.opensearch.response.agg.StatsParser;
56+
import org.opensearch.sql.opensearch.response.agg.TopHitsParser;
5557

5658
class AggregateAnalyzerTest {
5759

@@ -345,6 +347,137 @@ void analyze_groupBy_TextWithoutKeyword() {
345347
assertEquals("[field] must not be null", exception.getCause().getMessage());
346348
}
347349

350+
@Test
351+
void analyze_firstAggregation() throws ExpressionNotAnalyzableException {
352+
AggregateCall firstCall =
353+
AggregateCall.create(
354+
PPLBuiltinOperators.FIRST,
355+
false,
356+
false,
357+
false,
358+
ImmutableList.of(),
359+
ImmutableList.of(0),
360+
-1,
361+
null,
362+
RelCollations.EMPTY,
363+
typeFactory.createSqlType(SqlTypeName.INTEGER),
364+
"first_a");
365+
List<String> outputFields = List.of("first_a");
366+
Aggregate aggregate = createMockAggregate(List.of(firstCall), ImmutableBitSet.of());
367+
Project project = null; // No project needed for simple aggregation
368+
Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> result =
369+
AggregateAnalyzer.analyze(aggregate, project, rowType, fieldTypes, outputFields, null);
370+
371+
// Verify the aggregation builder JSON output
372+
assertEquals(
373+
"[{\"first_a\":{\"top_hits\":{\"from\":0,\"size\":1,\"version\":false,\"seq_no_primary_term\":false,\"explain\":false}}}]",
374+
result.getLeft().toString());
375+
assertInstanceOf(NoBucketAggregationParser.class, result.getRight());
376+
MetricParserHelper metricsParser =
377+
((NoBucketAggregationParser) result.getRight()).getMetricsParser();
378+
assertEquals(1, metricsParser.getMetricParserMap().size());
379+
metricsParser
380+
.getMetricParserMap()
381+
.forEach(
382+
(k, v) -> {
383+
assertEquals("first_a", k);
384+
assertInstanceOf(TopHitsParser.class, v);
385+
});
386+
}
387+
388+
@Test
389+
void analyze_lastAggregation() throws ExpressionNotAnalyzableException {
390+
AggregateCall lastCall =
391+
AggregateCall.create(
392+
PPLBuiltinOperators.LAST,
393+
false,
394+
false,
395+
false,
396+
ImmutableList.of(),
397+
ImmutableList.of(1),
398+
-1,
399+
null,
400+
RelCollations.EMPTY,
401+
typeFactory.createSqlType(SqlTypeName.VARCHAR),
402+
"last_b");
403+
List<String> outputFields = List.of("last_b");
404+
Aggregate aggregate = createMockAggregate(List.of(lastCall), ImmutableBitSet.of());
405+
Project project = null; // No project needed for simple aggregation
406+
Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> result =
407+
AggregateAnalyzer.analyze(aggregate, project, rowType, fieldTypes, outputFields, null);
408+
409+
// Verify the aggregation builder JSON output
410+
assertEquals(
411+
"[{\"last_b\":{\"top_hits\":{\"from\":0,\"size\":1,\"version\":false,\"seq_no_primary_term\":false,\"explain\":false,\"sort\":[{\"_doc\":{\"order\":\"desc\"}}]}}}]",
412+
result.getLeft().toString());
413+
assertInstanceOf(NoBucketAggregationParser.class, result.getRight());
414+
MetricParserHelper metricsParser =
415+
((NoBucketAggregationParser) result.getRight()).getMetricsParser();
416+
assertEquals(1, metricsParser.getMetricParserMap().size());
417+
metricsParser
418+
.getMetricParserMap()
419+
.forEach(
420+
(k, v) -> {
421+
assertEquals("last_b", k);
422+
assertInstanceOf(TopHitsParser.class, v);
423+
});
424+
}
425+
426+
@Test
427+
void analyze_firstAndLastAggregations_withGroupBy() throws ExpressionNotAnalyzableException {
428+
AggregateCall firstCall =
429+
AggregateCall.create(
430+
PPLBuiltinOperators.FIRST,
431+
false,
432+
false,
433+
false,
434+
ImmutableList.of(),
435+
ImmutableList.of(0),
436+
-1,
437+
null,
438+
RelCollations.EMPTY,
439+
typeFactory.createSqlType(SqlTypeName.INTEGER),
440+
"first_a");
441+
AggregateCall lastCall =
442+
AggregateCall.create(
443+
PPLBuiltinOperators.LAST,
444+
false,
445+
false,
446+
false,
447+
ImmutableList.of(),
448+
ImmutableList.of(0),
449+
-1,
450+
null,
451+
RelCollations.EMPTY,
452+
typeFactory.createSqlType(SqlTypeName.INTEGER),
453+
"last_a");
454+
List<String> outputFields = List.of("b", "first_a", "last_a");
455+
Aggregate aggregate = createMockAggregate(List.of(firstCall, lastCall), ImmutableBitSet.of(1));
456+
Project project =
457+
createMockProject(List.of(0, 1)); // Need both fields for groupBy with aggregations
458+
Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> result =
459+
AggregateAnalyzer.analyze(aggregate, project, rowType, fieldTypes, outputFields, null);
460+
461+
assertEquals(
462+
"[{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":["
463+
+ "{\"b\":{\"terms\":{\"field\":\"b.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},"
464+
+ "\"aggregations\":{"
465+
+ "\"first_a\":{\"top_hits\":{\"from\":0,\"size\":1,\"version\":false,\"seq_no_primary_term\":false,\"explain\":false,\"_source\":{\"includes\":[\"a\"],\"excludes\":[]}}},"
466+
+ "\"last_a\":{\"top_hits\":{\"from\":0,\"size\":1,\"version\":false,\"seq_no_primary_term\":false,\"explain\":false,\"_source\":{\"includes\":[\"a\"],\"excludes\":[]},\"sort\":[{\"_doc\":{\"order\":\"desc\"}}]}}}}}]",
467+
result.getLeft().toString());
468+
assertInstanceOf(CompositeAggregationParser.class, result.getRight());
469+
MetricParserHelper metricsParser =
470+
((CompositeAggregationParser) result.getRight()).getMetricsParser();
471+
assertEquals(2, metricsParser.getMetricParserMap().size());
472+
metricsParser
473+
.getMetricParserMap()
474+
.forEach(
475+
(k, v) -> {
476+
assertTrue(k.equals("first_a") || k.equals("last_a"));
477+
assertInstanceOf(TopHitsParser.class, v);
478+
});
479+
}
480+
348481
@Test
349482
void analyze_aggCall_simpleFilter() throws ExpressionNotAnalyzableException {
350483
buildAggregation("filter_cnt")

0 commit comments

Comments
 (0)