Skip to content

Commit fe913e9

Browse files
karenyrxkarenx
authored and
Peter Alfonsi
committed
[GRPC][Compound queries] Implement Boolean query and inject registry for all built-in query converters (opensearch-project#19391)
* [GRPC] Implement Boolean query and inject registry for all internal converters Signed-off-by: karenx <[email protected]> * update CHANGELOG Signed-off-by: Karen X <[email protected]> * delete extra file Signed-off-by: Karen X <[email protected]> * remove dead code and update tests Signed-off-by: Karen X <[email protected]> * pakcage private Signed-off-by: Karen X <[email protected]> * spotless and compile Signed-off-by: Karen X <[email protected]> * pass registry instead of making it static Signed-off-by: Karen X <[email protected]> --------- Signed-off-by: karenx <[email protected]> Signed-off-by: Karen X <[email protected]> Co-authored-by: karenx <[email protected]>
1 parent be9d41c commit fe913e9

File tree

10 files changed

+638
-18
lines changed

10 files changed

+638
-18
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
3232
- Add skip_list param for date, scaled float and token count fields ([#19142](https://github.com/opensearch-project/OpenSearch/pull/19142))
3333
- Implement GRPC MatchPhrase, MultiMatch queries ([#19449](https://github.com/opensearch-project/OpenSearch/pull/19449))
3434
- Optimize gRPC transport thread management for improved throughput ([#19278](https://github.com/opensearch-project/OpenSearch/pull/19278))
35+
- Implement GRPC Boolean query and inject registry for all internal query converters ([#19391](https://github.com/opensearch-project/OpenSearch/pull/19391))
3536

3637
### Changed
3738
- 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
@@ -329,6 +329,10 @@ public Collection<Object> createComponents(
329329
queryRegistry.registerConverter(converter);
330330
}
331331
logger.info("Successfully injected registry and registered all {} external converters", queryConverters.size());
332+
333+
// Update the registry on all converters (including built-in ones) so they can access external converters
334+
queryRegistry.updateRegistryOnAllConverters();
335+
logger.info("Updated registry on all converters to include external converters");
332336
} else {
333337
logger.info("No external QueryBuilderProtoConverter(s) to register");
334338
}

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
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+
}
33+
34+
@Override
35+
public QueryContainer.QueryContainerCase getHandledQueryCase() {
36+
return QueryContainer.QueryContainerCase.BOOL;
37+
}
38+
39+
@Override
40+
public QueryBuilder fromProto(QueryContainer queryContainer) {
41+
if (queryContainer == null || queryContainer.getQueryContainerCase() != QueryContainer.QueryContainerCase.BOOL) {
42+
throw new IllegalArgumentException("QueryContainer does not contain a Bool query");
43+
}
44+
45+
return BoolQueryBuilderProtoUtils.fromProto(queryContainer.getBool(), registry);
46+
}
47+
}

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

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
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+
class BoolQueryBuilderProtoUtils {
26+
27+
private BoolQueryBuilderProtoUtils() {
28+
// Utility class, no instances
29+
}
30+
31+
/**
32+
* Converts a Protocol Buffer BoolQuery to an OpenSearch BoolQueryBuilder.
33+
* Similar to {@link BoolQueryBuilder#fromXContent(org.opensearch.core.xcontent.XContentParser)}, this method
34+
* parses the Protocol Buffer representation and creates a properly configured
35+
* BoolQueryBuilder with the appropriate must, must_not, should, filter clauses,
36+
* boost, query name, and minimum_should_match.
37+
*
38+
* @param boolQueryProto The Protocol Buffer BoolQuery object
39+
* @param registry The registry to use for converting nested queries
40+
* @return A configured BoolQueryBuilder instance
41+
*/
42+
static BoolQueryBuilder fromProto(BoolQuery boolQueryProto, QueryBuilderProtoConverterRegistry registry) {
43+
String queryName = null;
44+
float boost = AbstractQueryBuilder.DEFAULT_BOOST;
45+
String minimumShouldMatch = null;
46+
boolean adjustPureNegative = BoolQueryBuilder.ADJUST_PURE_NEGATIVE_DEFAULT;
47+
48+
// Create BoolQueryBuilder
49+
BoolQueryBuilder boolQuery = new BoolQueryBuilder();
50+
51+
// Process name
52+
if (boolQueryProto.hasXName()) {
53+
queryName = boolQueryProto.getXName();
54+
boolQuery.queryName(queryName);
55+
}
56+
57+
// Process boost
58+
if (boolQueryProto.hasBoost()) {
59+
boost = boolQueryProto.getBoost();
60+
boolQuery.boost(boost);
61+
}
62+
63+
// Process minimum_should_match
64+
if (boolQueryProto.hasMinimumShouldMatch()) {
65+
MinimumShouldMatch minimumShouldMatchProto = boolQueryProto.getMinimumShouldMatch();
66+
switch (minimumShouldMatchProto.getMinimumShouldMatchCase()) {
67+
case INT32:
68+
minimumShouldMatch = String.valueOf(minimumShouldMatchProto.getInt32());
69+
break;
70+
case STRING:
71+
minimumShouldMatch = minimumShouldMatchProto.getString();
72+
break;
73+
default:
74+
// No minimum_should_match specified
75+
break;
76+
}
77+
78+
if (minimumShouldMatch != null) {
79+
boolQuery.minimumShouldMatch(minimumShouldMatch);
80+
}
81+
}
82+
83+
// Process must clauses
84+
List<QueryContainer> mustClauses = boolQueryProto.getMustList();
85+
for (QueryContainer queryContainer : mustClauses) {
86+
QueryBuilder queryBuilder = registry.fromProto(queryContainer);
87+
if (queryBuilder != null) {
88+
boolQuery.must(queryBuilder);
89+
}
90+
}
91+
92+
// Process must_not clauses
93+
List<QueryContainer> mustNotClauses = boolQueryProto.getMustNotList();
94+
for (QueryContainer queryContainer : mustNotClauses) {
95+
QueryBuilder queryBuilder = registry.fromProto(queryContainer);
96+
if (queryBuilder != null) {
97+
boolQuery.mustNot(queryBuilder);
98+
}
99+
}
100+
101+
// Process should clauses
102+
List<QueryContainer> shouldClauses = boolQueryProto.getShouldList();
103+
for (QueryContainer queryContainer : shouldClauses) {
104+
QueryBuilder queryBuilder = registry.fromProto(queryContainer);
105+
if (queryBuilder != null) {
106+
boolQuery.should(queryBuilder);
107+
}
108+
}
109+
110+
// Process filter clauses
111+
List<QueryContainer> filterClauses = boolQueryProto.getFilterList();
112+
for (QueryContainer queryContainer : filterClauses) {
113+
QueryBuilder queryBuilder = registry.fromProto(queryContainer);
114+
if (queryBuilder != null) {
115+
boolQuery.filter(queryBuilder);
116+
}
117+
}
118+
119+
// Process adjust_pure_negative
120+
if (boolQueryProto.hasAdjustPureNegative()) {
121+
adjustPureNegative = boolQueryProto.getAdjustPureNegative();
122+
boolQuery.adjustPureNegative(adjustPureNegative);
123+
}
124+
125+
return boolQuery;
126+
}
127+
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,10 @@ protected void registerBuiltInConverters() {
5050
delegate.registerConverter(new TermsQueryBuilderProtoConverter());
5151
delegate.registerConverter(new MatchPhraseQueryBuilderProtoConverter());
5252
delegate.registerConverter(new MultiMatchQueryBuilderProtoConverter());
53+
delegate.registerConverter(new BoolQueryBuilderProtoConverter());
5354

55+
// Set the registry on all converters so they can access each other
56+
delegate.setRegistryOnAllConverters(this);
5457
logger.info("Registered {} built-in query converters", delegate.size());
5558
}
5659

@@ -73,4 +76,13 @@ public QueryBuilder fromProto(QueryContainer queryContainer) {
7376
public void registerConverter(QueryBuilderProtoConverter converter) {
7477
delegate.registerConverter(converter);
7578
}
79+
80+
/**
81+
* Updates the registry on all registered converters.
82+
* This should be called after all external converters have been registered
83+
* to ensure converters like BoolQueryBuilderProtoConverter can access the complete registry.
84+
*/
85+
public void updateRegistryOnAllConverters() {
86+
delegate.setRegistryOnAllConverters(this);
87+
}
7688
}

‎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
@@ -334,6 +334,9 @@ public void testCreateComponentsWithExternalConverters() {
334334
Mockito.verify(mockConverter, Mockito.times(3)).getHandledQueryCase();
335335

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

‎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)