Skip to content

Commit 1085df8

Browse files
author
karenx
committed
[GRPC] Implement Boolean query and inject registry for all internal converters
1 parent 488149f commit 1085df8

File tree

11 files changed

+623
-1
lines changed

11 files changed

+623
-1
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2525
- Add new extensible method to DocRequest to specify type ([#19313](https://github.com/opensearch-project/OpenSearch/pull/19313))
2626
- [Rule based auto-tagging] Add Rule based auto-tagging IT ([#18550](https://github.com/opensearch-project/OpenSearch/pull/18550))
2727
- Add all-active ingestion as docrep equivalent in pull-based ingestion ([#19316](https://github.com/opensearch-project/OpenSearch/pull/19316))
28+
- Implement GRPC Boolean query and inject registry for all internal query converters ([#19388](https://github.com/opensearch-project/OpenSearch/pull/19388))
29+
2830

2931
### Changed
3032
- Refactor `if-else` chains to use `Java 17 pattern matching switch expressions`(([#18965](https://github.com/opensearch-project/OpenSearch/pull/18965))

‎modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcPlugin.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,10 @@ public Collection<Object> createComponents(
300300
queryRegistry.registerConverter(converter);
301301
}
302302
logger.info("Successfully injected registry and registered all {} external converters", queryConverters.size());
303+
304+
// Update the registry on all converters (including built-in ones) so they can access external converters
305+
queryRegistry.updateRegistryOnAllConverters();
306+
logger.info("Updated registry on all converters to include external converters");
303307
} else {
304308
logger.info("No external QueryBuilderProtoConverter(s) to register");
305309
}

‎modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/BoolQueryBuilderProtoConverter.java‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
package org.opensearch.transport.grpc.proto.request.search.query;
9+
10+
import org.opensearch.index.query.QueryBuilder;
11+
import org.opensearch.protobufs.QueryContainer;
12+
import org.opensearch.transport.grpc.spi.QueryBuilderProtoConverter;
13+
import org.opensearch.transport.grpc.spi.QueryBuilderProtoConverterRegistry;
14+
15+
/**
16+
* Converter for Bool queries.
17+
* This class implements the QueryBuilderProtoConverter interface to provide Bool query support
18+
* for the gRPC transport module.
19+
*/
20+
public class BoolQueryBuilderProtoConverter implements QueryBuilderProtoConverter {
21+
22+
/**
23+
* Default constructor for BoolQueryBuilderProtoConverter.
24+
*/
25+
public BoolQueryBuilderProtoConverter() {}
26+
27+
private QueryBuilderProtoConverterRegistry registry;
28+
29+
@Override
30+
public void setRegistry(QueryBuilderProtoConverterRegistry registry) {
31+
this.registry = registry;
32+
// Pass the registry to the utility class so it can convert nested queries
33+
BoolQueryBuilderProtoUtils.setRegistry(registry);
34+
}
35+
36+
@Override
37+
public QueryContainer.QueryContainerCase getHandledQueryCase() {
38+
return QueryContainer.QueryContainerCase.BOOL;
39+
}
40+
41+
@Override
42+
public QueryBuilder fromProto(QueryContainer queryContainer) {
43+
if (queryContainer == null || queryContainer.getQueryContainerCase() != QueryContainer.QueryContainerCase.BOOL) {
44+
throw new IllegalArgumentException("QueryContainer does not contain a Bool query");
45+
}
46+
47+
return BoolQueryBuilderProtoUtils.fromProto(queryContainer.getBool());
48+
}
49+
}

‎modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/BoolQueryBuilderProtoUtils.java‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
package org.opensearch.transport.grpc.proto.request.search.query;
9+
10+
import org.opensearch.index.query.AbstractQueryBuilder;
11+
import org.opensearch.index.query.BoolQueryBuilder;
12+
import org.opensearch.index.query.QueryBuilder;
13+
import org.opensearch.protobufs.BoolQuery;
14+
import org.opensearch.protobufs.MinimumShouldMatch;
15+
import org.opensearch.protobufs.QueryContainer;
16+
import org.opensearch.transport.grpc.spi.QueryBuilderProtoConverterRegistry;
17+
18+
import java.util.List;
19+
20+
/**
21+
* Utility class for converting BoolQuery Protocol Buffers to OpenSearch objects.
22+
* This class provides methods to transform Protocol Buffer representations of bool queries
23+
* into their corresponding OpenSearch BoolQueryBuilder implementations for search operations.
24+
*/
25+
public class BoolQueryBuilderProtoUtils {
26+
27+
// Registry for query conversion - injected by the gRPC plugin
28+
private static QueryBuilderProtoConverterRegistry REGISTRY;
29+
30+
private BoolQueryBuilderProtoUtils() {
31+
// Utility class, no instances
32+
}
33+
34+
/**
35+
* Sets the registry injected by the gRPC plugin.
36+
* This method is called when the Bool converter receives the populated registry.
37+
*
38+
* @param registry The registry to use
39+
*/
40+
public static void setRegistry(QueryBuilderProtoConverterRegistry registry) {
41+
REGISTRY = registry;
42+
}
43+
44+
/**
45+
* Gets the current registry.
46+
*
47+
* @return The current registry
48+
*/
49+
static QueryBuilderProtoConverterRegistry getRegistry() {
50+
return REGISTRY;
51+
}
52+
53+
/**
54+
* Converts a Protocol Buffer BoolQuery to an OpenSearch BoolQueryBuilder.
55+
* Similar to {@link BoolQueryBuilder#fromXContent(org.opensearch.core.xcontent.XContentParser)}, this method
56+
* parses the Protocol Buffer representation and creates a properly configured
57+
* BoolQueryBuilder with the appropriate must, must_not, should, filter clauses,
58+
* boost, query name, and minimum_should_match.
59+
*
60+
* @param boolQueryProto The Protocol Buffer BoolQuery object
61+
* @return A configured BoolQueryBuilder instance
62+
*/
63+
public static BoolQueryBuilder fromProto(BoolQuery boolQueryProto) {
64+
String queryName = null;
65+
float boost = AbstractQueryBuilder.DEFAULT_BOOST;
66+
String minimumShouldMatch = null;
67+
boolean adjustPureNegative = BoolQueryBuilder.ADJUST_PURE_NEGATIVE_DEFAULT;
68+
69+
// Create BoolQueryBuilder
70+
BoolQueryBuilder boolQuery = new BoolQueryBuilder();
71+
72+
// Process name
73+
if (boolQueryProto.hasXName()) {
74+
queryName = boolQueryProto.getXName();
75+
boolQuery.queryName(queryName);
76+
}
77+
78+
// Process boost
79+
if (boolQueryProto.hasBoost()) {
80+
boost = boolQueryProto.getBoost();
81+
boolQuery.boost(boost);
82+
}
83+
84+
// Process minimum_should_match
85+
if (boolQueryProto.hasMinimumShouldMatch()) {
86+
MinimumShouldMatch minimumShouldMatchProto = boolQueryProto.getMinimumShouldMatch();
87+
switch (minimumShouldMatchProto.getMinimumShouldMatchCase()) {
88+
case INT32_VALUE:
89+
minimumShouldMatch = String.valueOf(minimumShouldMatchProto.getInt32Value());
90+
break;
91+
case STRING_VALUE:
92+
minimumShouldMatch = minimumShouldMatchProto.getStringValue();
93+
break;
94+
default:
95+
// No minimum_should_match specified
96+
break;
97+
}
98+
99+
if (minimumShouldMatch != null) {
100+
boolQuery.minimumShouldMatch(minimumShouldMatch);
101+
}
102+
}
103+
104+
// Process must clauses
105+
List<QueryContainer> mustClauses = boolQueryProto.getMustList();
106+
for (QueryContainer queryContainer : mustClauses) {
107+
QueryBuilder queryBuilder = REGISTRY.fromProto(queryContainer);
108+
if (queryBuilder != null) {
109+
boolQuery.must(queryBuilder);
110+
}
111+
}
112+
113+
// Process must_not clauses
114+
List<QueryContainer> mustNotClauses = boolQueryProto.getMustNotList();
115+
for (QueryContainer queryContainer : mustNotClauses) {
116+
QueryBuilder queryBuilder = REGISTRY.fromProto(queryContainer);
117+
if (queryBuilder != null) {
118+
boolQuery.mustNot(queryBuilder);
119+
}
120+
}
121+
122+
// Process should clauses
123+
List<QueryContainer> shouldClauses = boolQueryProto.getShouldList();
124+
for (QueryContainer queryContainer : shouldClauses) {
125+
QueryBuilder queryBuilder = REGISTRY.fromProto(queryContainer);
126+
if (queryBuilder != null) {
127+
boolQuery.should(queryBuilder);
128+
}
129+
}
130+
131+
// Process filter clauses
132+
List<QueryContainer> filterClauses = boolQueryProto.getFilterList();
133+
for (QueryContainer queryContainer : filterClauses) {
134+
QueryBuilder queryBuilder = REGISTRY.fromProto(queryContainer);
135+
if (queryBuilder != null) {
136+
boolQuery.filter(queryBuilder);
137+
}
138+
}
139+
140+
// Process adjust_pure_negative
141+
if (boolQueryProto.hasAdjustPureNegative()) {
142+
adjustPureNegative = boolQueryProto.getAdjustPureNegative();
143+
boolQuery.adjustPureNegative(adjustPureNegative);
144+
}
145+
146+
return boolQuery;
147+
}
148+
}

‎modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/QueryBuilderProtoConverterRegistryImpl.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ protected void registerBuiltInConverters() {
4848
delegate.registerConverter(new MatchNoneQueryBuilderProtoConverter());
4949
delegate.registerConverter(new TermQueryBuilderProtoConverter());
5050
delegate.registerConverter(new TermsQueryBuilderProtoConverter());
51+
delegate.registerConverter(new BoolQueryBuilderProtoConverter());
52+
53+
// Set the registry on all converters so they can access each other
54+
delegate.setRegistryOnAllConverters(this);
5155

5256
logger.info("Registered {} built-in query converters", delegate.size());
5357
}
@@ -71,4 +75,13 @@ public QueryBuilder fromProto(QueryContainer queryContainer) {
7175
public void registerConverter(QueryBuilderProtoConverter converter) {
7276
delegate.registerConverter(converter);
7377
}
78+
79+
/**
80+
* Updates the registry on all registered converters.
81+
* This should be called after all external converters have been registered
82+
* to ensure converters like BoolQueryBuilderProtoConverter can access the complete registry.
83+
*/
84+
public void updateRegistryOnAllConverters() {
85+
delegate.setRegistryOnAllConverters(this);
86+
}
7487
}

‎modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/QueryBuilderProtoConverterSpiRegistry.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.opensearch.index.query.QueryBuilder;
1515
import org.opensearch.protobufs.QueryContainer;
1616
import org.opensearch.transport.grpc.spi.QueryBuilderProtoConverter;
17+
import org.opensearch.transport.grpc.spi.QueryBuilderProtoConverterRegistry;
1718

1819
import java.util.HashMap;
1920
import java.util.Map;
@@ -71,6 +72,19 @@ public int size() {
7172
return converterMap.size();
7273
}
7374

75+
/**
76+
* Sets the registry on all registered converters.
77+
* This is used to inject the complete registry into converters that need it (like BoolQueryBuilderProtoConverter).
78+
*
79+
* @param registry The registry to inject into all converters
80+
*/
81+
public void setRegistryOnAllConverters(QueryBuilderProtoConverterRegistry registry) {
82+
for (QueryBuilderProtoConverter converter : converterMap.values()) {
83+
converter.setRegistry(registry);
84+
}
85+
logger.info("Set registry on {} converters", converterMap.size());
86+
}
87+
7488
/**
7589
* Registers a new converter.
7690
*

‎modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/GrpcPluginTests.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,9 @@ public void testCreateComponentsWithExternalConverters() {
332332
Mockito.verify(mockConverter, Mockito.times(3)).getHandledQueryCase();
333333

334334
// Verify that setRegistry was called to inject the registry
335-
Mockito.verify(mockConverter, Mockito.times(1)).setRegistry(Mockito.any());
335+
// Note: setRegistry is called 2 times:
336+
// 1. In createComponents() when processing external converters
337+
// 2. In updateRegistryOnAllConverters() to ensure all converters have the complete registry
338+
Mockito.verify(mockConverter, Mockito.times(2)).setRegistry(Mockito.any());
336339
}
337340
}

‎modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/BoolQueryBuilderProtoConverterTests.java‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
package org.opensearch.transport.grpc.proto.request.search.query;
9+
10+
import org.opensearch.index.query.BoolQueryBuilder;
11+
import org.opensearch.index.query.QueryBuilder;
12+
import org.opensearch.protobufs.BoolQuery;
13+
import org.opensearch.protobufs.QueryContainer;
14+
import org.opensearch.test.OpenSearchTestCase;
15+
16+
public class BoolQueryBuilderProtoConverterTests extends OpenSearchTestCase {
17+
18+
private BoolQueryBuilderProtoConverter converter;
19+
20+
@Override
21+
public void setUp() throws Exception {
22+
super.setUp();
23+
converter = new BoolQueryBuilderProtoConverter();
24+
// Set up the registry for nested query conversion
25+
QueryBuilderProtoConverterRegistryImpl registry = new QueryBuilderProtoConverterRegistryImpl();
26+
converter.setRegistry(registry);
27+
}
28+
29+
public void testGetHandledQueryCase() {
30+
// Test that the converter returns the correct QueryContainerCase
31+
assertEquals("Converter should handle BOOL case", QueryContainer.QueryContainerCase.BOOL, converter.getHandledQueryCase());
32+
}
33+
34+
public void testFromProto() {
35+
// Create a QueryContainer with BoolQuery
36+
BoolQuery boolQuery = BoolQuery.newBuilder().setBoost(2.0f).setXName("test_bool_query").setAdjustPureNegative(false).build();
37+
QueryContainer queryContainer = QueryContainer.newBuilder().setBool(boolQuery).build();
38+
39+
// Convert the query
40+
QueryBuilder queryBuilder = converter.fromProto(queryContainer);
41+
42+
// Verify the result
43+
assertNotNull("QueryBuilder should not be null", queryBuilder);
44+
assertTrue("QueryBuilder should be a BoolQueryBuilder", queryBuilder instanceof BoolQueryBuilder);
45+
BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) queryBuilder;
46+
assertEquals("Boost should match", 2.0f, boolQueryBuilder.boost(), 0.0f);
47+
assertEquals("Query name should match", "test_bool_query", boolQueryBuilder.queryName());
48+
assertFalse("Adjust pure negative should match", boolQueryBuilder.adjustPureNegative());
49+
}
50+
51+
public void testFromProtoWithMinimalFields() {
52+
// Create a QueryContainer with minimal BoolQuery
53+
BoolQuery boolQuery = BoolQuery.newBuilder().build();
54+
QueryContainer queryContainer = QueryContainer.newBuilder().setBool(boolQuery).build();
55+
56+
// Convert the query
57+
QueryBuilder queryBuilder = converter.fromProto(queryContainer);
58+
59+
// Verify the result
60+
assertNotNull("QueryBuilder should not be null", queryBuilder);
61+
assertTrue("QueryBuilder should be a BoolQueryBuilder", queryBuilder instanceof BoolQueryBuilder);
62+
BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) queryBuilder;
63+
assertEquals("Default boost should be 1.0", 1.0f, boolQueryBuilder.boost(), 0.0f);
64+
assertNull("Query name should be null", boolQueryBuilder.queryName());
65+
assertTrue("Default adjust pure negative should be true", boolQueryBuilder.adjustPureNegative());
66+
}
67+
68+
public void testFromProtoWithInvalidContainer() {
69+
// Create a QueryContainer with a different query type
70+
QueryContainer emptyContainer = QueryContainer.newBuilder().build();
71+
72+
// Test that the converter throws an exception
73+
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> converter.fromProto(emptyContainer));
74+
75+
// Verify the exception message
76+
assertTrue(
77+
"Exception message should mention 'does not contain a Bool query'",
78+
exception.getMessage().contains("does not contain a Bool query")
79+
);
80+
}
81+
82+
public void testFromProtoWithNullContainer() {
83+
// Test that the converter throws an exception with null input
84+
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> converter.fromProto(null));
85+
86+
// Verify the exception message
87+
assertTrue(
88+
"Exception message should mention null",
89+
exception.getMessage().contains("null") || exception.getMessage().contains("does not contain a Bool query")
90+
);
91+
}
92+
}

0 commit comments

Comments
 (0)