Skip to content

Commit 32770fc

Browse files
yiyuabcclaude
andcommitted
Add integration tests for min/max aggregations and fix javadocs
- Add testMinAggregation() and testMaxAggregation() to SearchServiceIT - Verify min/max aggregations work end-to-end with live cluster via gRPC - Add missing javadocs to AggregationBuilderProtoConverter* classes - Add missing javadocs to AggregateProtoConverter* classes Integration tests validate: - Min aggregation returns correct minimum value (5.2 from [10.5, 25.0, 5.2]) - Max aggregation returns correct maximum value (25.0 from [10.5, 25.0, 5.2]) - gRPC aggregation results match expected values All integration tests passing (8 tests total). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yiyu Pan <yypan14@gmail.com>
1 parent d0c814d commit 32770fc

File tree

7 files changed

+146
-0
lines changed

7 files changed

+146
-0
lines changed

modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/SearchServiceIT.java

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@
88

99
package org.opensearch.transport.grpc;
1010

11+
import org.opensearch.protobufs.Aggregate;
12+
import org.opensearch.protobufs.AggregationContainer;
13+
import org.opensearch.protobufs.MaxAggregation;
14+
import org.opensearch.protobufs.MinAggregation;
1115
import org.opensearch.protobufs.SearchRequest;
1216
import org.opensearch.protobufs.SearchRequestBody;
1317
import org.opensearch.protobufs.SearchResponse;
@@ -57,4 +61,86 @@ public void testSearchServiceSearch() throws Exception {
5761
assertEquals("Hit should have correct ID", "1", searchResponse.getHits().getHits(0).getXId());
5862
}
5963
}
64+
65+
/**
66+
* Tests min aggregation via gRPC.
67+
*/
68+
public void testMinAggregation() throws Exception {
69+
// Create a test index
70+
String indexName = "test-min-agg";
71+
createTestIndex(indexName);
72+
73+
// Index documents with different price values
74+
indexTestDocument(indexName, "1", "{\"price\": 10.5}");
75+
indexTestDocument(indexName, "2", "{\"price\": 25.0}");
76+
indexTestDocument(indexName, "3", "{\"price\": 5.2}");
77+
78+
// Create a gRPC client
79+
try (NettyGrpcClient client = createGrpcClient()) {
80+
ManagedChannel channel = client.getChannel();
81+
SearchServiceGrpc.SearchServiceBlockingStub searchStub = SearchServiceGrpc.newBlockingStub(channel);
82+
83+
// Build min aggregation request
84+
MinAggregation minAgg = MinAggregation.newBuilder().setField("price").build();
85+
86+
SearchRequestBody requestBody = SearchRequestBody.newBuilder()
87+
.setSize(0)
88+
.putAggregations("min_price", AggregationContainer.newBuilder().setMin(minAgg).build())
89+
.build();
90+
91+
SearchRequest searchRequest = SearchRequest.newBuilder().addIndex(indexName).setSearchRequestBody(requestBody).build();
92+
93+
// Execute search
94+
SearchResponse response = searchStub.search(searchRequest);
95+
96+
// Verify min aggregation result
97+
assertNotNull("Search response should not be null", response);
98+
assertTrue("Should have aggregations", response.getAggregationsCount() > 0);
99+
Aggregate minResult = response.getAggregationsMap().get("min_price");
100+
assertNotNull("Min aggregation should exist", minResult);
101+
assertTrue("Should have value", minResult.hasValue());
102+
assertEquals("Min value should be 5.2", 5.2, minResult.getValue().getDouble(), 0.001);
103+
}
104+
}
105+
106+
/**
107+
* Tests max aggregation via gRPC.
108+
*/
109+
public void testMaxAggregation() throws Exception {
110+
// Create a test index
111+
String indexName = "test-max-agg";
112+
createTestIndex(indexName);
113+
114+
// Index documents with different price values
115+
indexTestDocument(indexName, "1", "{\"price\": 10.5}");
116+
indexTestDocument(indexName, "2", "{\"price\": 25.0}");
117+
indexTestDocument(indexName, "3", "{\"price\": 5.2}");
118+
119+
// Create a gRPC client
120+
try (NettyGrpcClient client = createGrpcClient()) {
121+
ManagedChannel channel = client.getChannel();
122+
SearchServiceGrpc.SearchServiceBlockingStub searchStub = SearchServiceGrpc.newBlockingStub(channel);
123+
124+
// Build max aggregation request
125+
MaxAggregation maxAgg = MaxAggregation.newBuilder().setField("price").build();
126+
127+
SearchRequestBody requestBody = SearchRequestBody.newBuilder()
128+
.setSize(0)
129+
.putAggregations("max_price", AggregationContainer.newBuilder().setMax(maxAgg).build())
130+
.build();
131+
132+
SearchRequest searchRequest = SearchRequest.newBuilder().addIndex(indexName).setSearchRequestBody(requestBody).build();
133+
134+
// Execute search
135+
SearchResponse response = searchStub.search(searchRequest);
136+
137+
// Verify max aggregation result
138+
assertNotNull("Search response should not be null", response);
139+
assertTrue("Should have aggregations", response.getAggregationsCount() > 0);
140+
Aggregate maxResult = response.getAggregationsMap().get("max_price");
141+
assertNotNull("Max aggregation should exist", maxResult);
142+
assertTrue("Should have value", maxResult.hasValue());
143+
assertEquals("Max value should be 25.0", 25.0, maxResult.getValue().getDouble(), 0.001);
144+
}
145+
}
60146
}

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/AggregationBuilderProtoConverterRegistryImpl.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,19 @@ public class AggregationBuilderProtoConverterRegistryImpl implements Aggregation
2727
private static final Logger logger = LogManager.getLogger(AggregationBuilderProtoConverterRegistryImpl.class);
2828
private final AggregationBuilderProtoConverterSpiRegistry delegate;
2929

30+
/**
31+
* Creates a new AggregationBuilderProtoConverterRegistryImpl and registers built-in converters.
32+
*/
3033
@Inject
3134
public AggregationBuilderProtoConverterRegistryImpl() {
3235
this.delegate = new AggregationBuilderProtoConverterSpiRegistry();
3336

3437
registerBuiltInConverters();
3538
}
3639

40+
/**
41+
* Registers all built-in aggregation converters.
42+
*/
3743
protected void registerBuiltInConverters() {
3844
// Register metric aggregation converters
3945
delegate.registerConverter(new MinAggregationBuilderProtoConverter());
@@ -51,16 +57,28 @@ protected void registerBuiltInConverters() {
5157
/**
5258
* Converts protobuf to AggregationBuilder.
5359
* Mirrors {@link org.opensearch.search.aggregations.AggregatorFactories#parseAggregators}.
60+
*
61+
* @param name The aggregation name
62+
* @param container The protobuf container
63+
* @return The OpenSearch AggregationBuilder
5464
*/
5565
@Override
5666
public AggregationBuilder fromProto(String name, AggregationContainer container) {
5767
return delegate.fromProto(name, container);
5868
}
5969

70+
/**
71+
* Registers an external aggregation converter.
72+
*
73+
* @param converter The converter to register
74+
*/
6075
public void registerConverter(AggregationBuilderProtoConverter converter) {
6176
delegate.registerConverter(converter);
6277
}
6378

79+
/**
80+
* Updates the registry reference on all registered converters.
81+
*/
6482
public void updateRegistryOnAllConverters() {
6583
delegate.setRegistryOnAllConverters(this);
6684
}

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/AggregationBuilderProtoConverterSpiRegistry.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ public class AggregationBuilderProtoConverterSpiRegistry implements AggregationB
3232
private static final Logger logger = LogManager.getLogger(AggregationBuilderProtoConverterSpiRegistry.class);
3333
private final Map<AggregationContainer.AggregationContainerCase, AggregationBuilderProtoConverter> converterMap = new HashMap<>();
3434

35+
/**
36+
* Creates a new AggregationBuilderProtoConverterSpiRegistry.
37+
* External converters are loaded via OpenSearch's ExtensiblePlugin mechanism
38+
* and registered manually via registerConverter() calls.
39+
*/
3540
@Inject
3641
public AggregationBuilderProtoConverterSpiRegistry() {
3742
// External converters are loaded via OpenSearch's ExtensiblePlugin mechanism
@@ -41,6 +46,10 @@ public AggregationBuilderProtoConverterSpiRegistry() {
4146
/**
4247
* Converts protobuf to AggregationBuilder with metadata and subaggregations.
4348
* Mirrors {@link org.opensearch.search.aggregations.AggregatorFactories#parseAggregators}.
49+
*
50+
* @param name The aggregation name
51+
* @param container The protobuf container
52+
* @return The OpenSearch AggregationBuilder
4453
*/
4554
@Override
4655
public AggregationBuilder fromProto(String name, AggregationContainer container) {
@@ -98,17 +107,32 @@ public AggregationBuilder fromProto(String name, AggregationContainer container)
98107
return builder;
99108
}
100109

110+
/**
111+
* Returns the number of registered converters.
112+
*
113+
* @return The converter count
114+
*/
101115
public int size() {
102116
return converterMap.size();
103117
}
104118

119+
/**
120+
* Sets the registry on all registered converters.
121+
*
122+
* @param registry The registry to set
123+
*/
105124
public void setRegistryOnAllConverters(AggregationBuilderProtoConverterRegistry registry) {
106125
for (AggregationBuilderProtoConverter converter : converterMap.values()) {
107126
converter.setRegistry(registry);
108127
}
109128
logger.info("Set registry on {} aggregation converter(s)", converterMap.size());
110129
}
111130

131+
/**
132+
* Registers a converter for a specific aggregation type.
133+
*
134+
* @param converter The converter to register
135+
*/
112136
public void registerConverter(AggregationBuilderProtoConverter converter) {
113137
if (converter == null) {
114138
throw new IllegalArgumentException("Converter cannot be null");

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoConverterRegistryImpl.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ public class AggregateProtoConverterRegistryImpl implements AggregateProtoConver
2323

2424
private final AggregateProtoConverterSpiRegistry spiRegistry;
2525

26+
/**
27+
* Creates a new AggregateProtoConverterRegistryImpl and registers built-in converters.
28+
*/
2629
public AggregateProtoConverterRegistryImpl() {
2730
this.spiRegistry = new AggregateProtoConverterSpiRegistry();
2831
registerBuiltInConverters();

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoConverterSpiRegistry.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ public class AggregateProtoConverterSpiRegistry implements AggregateProtoConvert
3030
private static final Logger logger = LogManager.getLogger(AggregateProtoConverterSpiRegistry.class);
3131
private final Map<Class<? extends InternalAggregation>, AggregateProtoConverter> converters = new HashMap<>();
3232

33+
/**
34+
* Creates a new AggregateProtoConverterSpiRegistry.
35+
* External converters can be loaded via OpenSearch's ExtensiblePlugin mechanism
36+
* and registered manually via registerConverter() calls.
37+
*/
3338
public AggregateProtoConverterSpiRegistry() {
3439
// External converters can be loaded via OpenSearch's ExtensiblePlugin mechanism
3540
// and registered manually via registerConverter() calls

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MaxAggregateProtoConverter.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@
2020
*/
2121
public class MaxAggregateProtoConverter implements AggregateProtoConverter {
2222

23+
/**
24+
* Creates a new MaxAggregateProtoConverter.
25+
*/
26+
public MaxAggregateProtoConverter() {}
27+
2328
@Override
2429
public Class<? extends InternalAggregation> getHandledAggregationType() {
2530
return InternalMax.class;

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MinAggregateProtoConverter.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@
2020
*/
2121
public class MinAggregateProtoConverter implements AggregateProtoConverter {
2222

23+
/**
24+
* Creates a new MinAggregateProtoConverter.
25+
*/
26+
public MinAggregateProtoConverter() {}
27+
2328
@Override
2429
public Class<? extends InternalAggregation> getHandledAggregationType() {
2530
return InternalMin.class;

0 commit comments

Comments
 (0)