Skip to content

Commit c8ecfc3

Browse files
mch2dk2k
mch2
authored and
dk2k
committed
Fix DerivedFieldQuery to support concurrent search. (opensearch-project#15326)
* Fix DerivedFieldQuery to support concurrent search. This change updates DerivedFieldQuery to create a separate ValueFetcher instance per thread. The DerivedFieldValueFetcher is not thread safe in that it holds a ref to a compiled DerivedFieldScript that is created per thread. Each script also holds a SourceLookup object that is not thread safe. Signed-off-by: Marc Handalian <marc.handalian@gmail.com> * Fix broken cases relying on ObjectDerivedFieldValueFetcher. DerivedFieldQuery will accept a supplier for a valueFetcher rather than constructing it. This ensures that the DerivedFieldType creating the query (obj or regular) passes the correct supplier func. Signed-off-by: Marc Handalian <marc.handalian@gmail.com> * remove unused clone method Signed-off-by: Marc Handalian <marc.handalian@gmail.com> * Add changelog entry Signed-off-by: Marc Handalian <marc.handalian@gmail.com> * add an extra test for DerivedFieldType multiPhraseQuery Signed-off-by: Marc Handalian <marc.handalian@gmail.com> * more coverage Signed-off-by: Marc Handalian <marc.handalian@gmail.com> * add tests for normalizedWildcard and phrase prefix Signed-off-by: Marc Handalian <marc.handalian@gmail.com> --------- Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
1 parent c0fc003 commit c8ecfc3

File tree

7 files changed

+77
-48
lines changed

7 files changed

+77
-48
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2626
- Add allowlist setting for ingest-geoip and ingest-useragent ([#15325](https://github.com/opensearch-project/OpenSearch/pull/15325))
2727
- Adding access to noSubMatches and noOverlappingMatches in Hyphenation ([#13895](https://github.com/opensearch-project/OpenSearch/pull/13895))
2828
- Add support for index level max slice count setting for concurrent segment search ([#15336](https://github.com/opensearch-project/OpenSearch/pull/15336))
29+
- Add concurrent search support for Derived Fields ([#15326](https://github.com/opensearch-project/OpenSearch/pull/15326))
2930

3031
### Dependencies
3132
- Bump `netty` from 4.1.111.Final to 4.1.112.Final ([#15081](https://github.com/opensearch-project/OpenSearch/pull/15081))

‎modules/lang-painless/src/internalClusterTest/java/org/opensearch/painless/SimplePainlessIT.java

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -188,10 +188,6 @@ public void testTermsValuesSource() throws Exception {
188188
}
189189

190190
public void testSimpleDerivedFieldsQuery() {
191-
assumeFalse(
192-
"Derived fields do not support concurrent search https://github.com/opensearch-project/OpenSearch/issues/15007",
193-
internalCluster().clusterService().getClusterSettings().get(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING)
194-
);
195191
SearchRequest searchRequest = new SearchRequest("test-df").source(
196192
SearchSourceBuilder.searchSource()
197193
.derivedField("result", "keyword", new Script("emit(params._source[\"field\"])"))
@@ -204,10 +200,6 @@ public void testSimpleDerivedFieldsQuery() {
204200
}
205201

206202
public void testSimpleDerivedFieldsAgg() {
207-
assumeFalse(
208-
"Derived fields do not support concurrent search https://github.com/opensearch-project/OpenSearch/issues/15007",
209-
internalCluster().clusterService().getClusterSettings().get(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING)
210-
);
211203
SearchRequest searchRequest = new SearchRequest("test-df").source(
212204
SearchSourceBuilder.searchSource()
213205
.derivedField("result", "keyword", new Script("emit(params._source[\"field\"])"))

‎server/src/main/java/org/opensearch/index/mapper/DerivedFieldType.java

Lines changed: 15 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,9 @@ public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName, S
159159
@Override
160160
public Query termQuery(Object value, QueryShardContext context) {
161161
Query query = typeFieldMapper.mappedFieldType.termQuery(value, context);
162-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
163162
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
164163
query,
165-
valueFetcher,
164+
() -> valueFetcher(context, context.lookup(), null),
166165
context.lookup(),
167166
getIndexAnalyzer(),
168167
indexableFieldGenerator,
@@ -176,10 +175,9 @@ public Query termQuery(Object value, QueryShardContext context) {
176175
@Override
177176
public Query termQueryCaseInsensitive(Object value, @Nullable QueryShardContext context) {
178177
Query query = typeFieldMapper.mappedFieldType.termQueryCaseInsensitive(value, context);
179-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
180178
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
181179
query,
182-
valueFetcher,
180+
() -> valueFetcher(context, context.lookup(), null),
183181
context.lookup(),
184182
getIndexAnalyzer(),
185183
indexableFieldGenerator,
@@ -195,10 +193,9 @@ public Query termQueryCaseInsensitive(Object value, @Nullable QueryShardContext
195193
@Override
196194
public Query termsQuery(List<?> values, @Nullable QueryShardContext context) {
197195
Query query = typeFieldMapper.mappedFieldType.termsQuery(values, context);
198-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
199196
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
200197
query,
201-
valueFetcher,
198+
() -> valueFetcher(context, context.lookup(), null),
202199
context.lookup(),
203200
getIndexAnalyzer(),
204201
indexableFieldGenerator,
@@ -230,10 +227,9 @@ public Query rangeQuery(
230227
parser,
231228
context
232229
);
233-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
234230
return new DerivedFieldQuery(
235231
query,
236-
valueFetcher,
232+
() -> valueFetcher(context, context.lookup(), null),
237233
context.lookup(),
238234
getIndexAnalyzer(),
239235
indexableFieldGenerator,
@@ -251,10 +247,9 @@ public Query fuzzyQuery(
251247
QueryShardContext context
252248
) {
253249
Query query = typeFieldMapper.mappedFieldType.fuzzyQuery(value, fuzziness, prefixLength, maxExpansions, transpositions, context);
254-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
255250
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
256251
query,
257-
valueFetcher,
252+
() -> valueFetcher(context, context.lookup(), null),
258253
context.lookup(),
259254
getIndexAnalyzer(),
260255
indexableFieldGenerator,
@@ -289,10 +284,9 @@ public Query fuzzyQuery(
289284
method,
290285
context
291286
);
292-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
293287
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
294288
query,
295-
valueFetcher,
289+
() -> valueFetcher(context, context.lookup(), null),
296290
context.lookup(),
297291
getIndexAnalyzer(),
298292
indexableFieldGenerator,
@@ -316,10 +310,9 @@ public Query prefixQuery(
316310
QueryShardContext context
317311
) {
318312
Query query = typeFieldMapper.mappedFieldType.prefixQuery(value, method, caseInsensitive, context);
319-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
320313
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
321314
query,
322-
valueFetcher,
315+
() -> valueFetcher(context, context.lookup(), null),
323316
context.lookup(),
324317
getIndexAnalyzer(),
325318
indexableFieldGenerator,
@@ -343,10 +336,9 @@ public Query wildcardQuery(
343336
QueryShardContext context
344337
) {
345338
Query query = typeFieldMapper.mappedFieldType.wildcardQuery(value, method, caseInsensitive, context);
346-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
347339
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
348340
query,
349-
valueFetcher,
341+
() -> valueFetcher(context, context.lookup(), null),
350342
context.lookup(),
351343
getIndexAnalyzer(),
352344
indexableFieldGenerator,
@@ -365,10 +357,9 @@ public Query wildcardQuery(
365357
@Override
366358
public Query normalizedWildcardQuery(String value, @Nullable MultiTermQuery.RewriteMethod method, QueryShardContext context) {
367359
Query query = typeFieldMapper.mappedFieldType.normalizedWildcardQuery(value, method, context);
368-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
369360
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
370361
query,
371-
valueFetcher,
362+
() -> valueFetcher(context, context.lookup(), null),
372363
context.lookup(),
373364
getIndexAnalyzer(),
374365
indexableFieldGenerator,
@@ -394,10 +385,9 @@ public Query regexpQuery(
394385
QueryShardContext context
395386
) {
396387
Query query = typeFieldMapper.mappedFieldType.regexpQuery(value, syntaxFlags, matchFlags, maxDeterminizedStates, method, context);
397-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
398388
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
399389
query,
400-
valueFetcher,
390+
() -> valueFetcher(context, context.lookup(), null),
401391
context.lookup(),
402392
getIndexAnalyzer(),
403393
indexableFieldGenerator,
@@ -416,10 +406,9 @@ public Query regexpQuery(
416406
@Override
417407
public Query phraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements, QueryShardContext context) throws IOException {
418408
Query query = typeFieldMapper.mappedFieldType.phraseQuery(stream, slop, enablePositionIncrements, context);
419-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
420409
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
421410
query,
422-
valueFetcher,
411+
() -> valueFetcher(context, context.lookup(), null),
423412
context.lookup(),
424413
getIndexAnalyzer(),
425414
indexableFieldGenerator,
@@ -441,10 +430,9 @@ public Query phraseQuery(TokenStream stream, int slop, boolean enablePositionInc
441430
public Query multiPhraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements, QueryShardContext context)
442431
throws IOException {
443432
Query query = typeFieldMapper.mappedFieldType.multiPhraseQuery(stream, slop, enablePositionIncrements, context);
444-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
445433
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
446434
query,
447-
valueFetcher,
435+
() -> valueFetcher(context, context.lookup(), null),
448436
context.lookup(),
449437
getIndexAnalyzer(),
450438
indexableFieldGenerator,
@@ -465,10 +453,9 @@ public Query multiPhraseQuery(TokenStream stream, int slop, boolean enablePositi
465453
@Override
466454
public Query phrasePrefixQuery(TokenStream stream, int slop, int maxExpansions, QueryShardContext context) throws IOException {
467455
Query query = typeFieldMapper.mappedFieldType.phrasePrefixQuery(stream, slop, maxExpansions, context);
468-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
469456
DerivedFieldQuery derivedFieldQuery = new DerivedFieldQuery(
470457
query,
471-
valueFetcher,
458+
() -> valueFetcher(context, context.lookup(), null),
472459
context.lookup(),
473460
getIndexAnalyzer(),
474461
indexableFieldGenerator,
@@ -493,10 +480,9 @@ public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRew
493480
@Override
494481
public Query distanceFeatureQuery(Object origin, String pivot, float boost, QueryShardContext context) {
495482
Query query = typeFieldMapper.mappedFieldType.distanceFeatureQuery(origin, pivot, boost, context);
496-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
497483
return new DerivedFieldQuery(
498484
query,
499-
valueFetcher,
485+
() -> valueFetcher(context, context.lookup(), null),
500486
context.lookup(),
501487
getIndexAnalyzer(),
502488
indexableFieldGenerator,
@@ -507,10 +493,9 @@ public Query distanceFeatureQuery(Object origin, String pivot, float boost, Quer
507493
@Override
508494
public Query geoShapeQuery(Geometry shape, String fieldName, ShapeRelation relation, QueryShardContext context) {
509495
Query query = ((GeoShapeQueryable) (typeFieldMapper.mappedFieldType)).geoShapeQuery(shape, fieldName, relation, context);
510-
DerivedFieldValueFetcher valueFetcher = valueFetcher(context, context.lookup(), null);
511496
return new DerivedFieldQuery(
512497
query,
513-
valueFetcher,
498+
() -> valueFetcher(context, context.lookup(), null),
514499
context.lookup(),
515500
getIndexAnalyzer(),
516501
indexableFieldGenerator,

‎server/src/main/java/org/opensearch/index/query/DerivedFieldQuery.java

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,15 @@
3030
import java.util.List;
3131
import java.util.Objects;
3232
import java.util.function.Function;
33+
import java.util.function.Supplier;
3334

3435
/**
3536
* DerivedFieldQuery used for querying derived fields. It contains the logic to execute an input lucene query against
3637
* DerivedField. It also accepts DerivedFieldValueFetcher and SearchLookup as an input.
3738
*/
3839
public final class DerivedFieldQuery extends Query {
3940
private final Query query;
40-
private final DerivedFieldValueFetcher valueFetcher;
41+
private final Supplier<DerivedFieldValueFetcher> valueFetcherSupplier;
4142
private final SearchLookup searchLookup;
4243
private final Analyzer indexAnalyzer;
4344
private final boolean ignoreMalformed;
@@ -46,20 +47,19 @@ public final class DerivedFieldQuery extends Query {
4647

4748
/**
4849
* @param query lucene query to be executed against the derived field
49-
* @param valueFetcher DerivedFieldValueFetcher ValueFetcher to fetch the value of a derived field from _source
50-
* using LeafSearchLookup
50+
* @param valueFetcherSupplier Supplier of a DerivedFieldValueFetcher that will be reconstructed per leaf
5151
* @param searchLookup SearchLookup to get the LeafSearchLookup look used by valueFetcher to fetch the _source
5252
*/
5353
public DerivedFieldQuery(
5454
Query query,
55-
DerivedFieldValueFetcher valueFetcher,
55+
Supplier<DerivedFieldValueFetcher> valueFetcherSupplier,
5656
SearchLookup searchLookup,
5757
Analyzer indexAnalyzer,
5858
Function<Object, IndexableField> indexableFieldGenerator,
5959
boolean ignoreMalformed
6060
) {
6161
this.query = query;
62-
this.valueFetcher = valueFetcher;
62+
this.valueFetcherSupplier = valueFetcherSupplier;
6363
this.searchLookup = searchLookup;
6464
this.indexAnalyzer = indexAnalyzer;
6565
this.indexableFieldGenerator = indexableFieldGenerator;
@@ -77,7 +77,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
7777
if (rewritten == query) {
7878
return this;
7979
}
80-
return new DerivedFieldQuery(rewritten, valueFetcher, searchLookup, indexAnalyzer, indexableFieldGenerator, ignoreMalformed);
80+
;
81+
return new DerivedFieldQuery(
82+
rewritten,
83+
valueFetcherSupplier,
84+
searchLookup,
85+
indexAnalyzer,
86+
indexableFieldGenerator,
87+
ignoreMalformed
88+
);
8189
}
8290

8391
@Override
@@ -88,6 +96,11 @@ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float bo
8896
public Scorer scorer(LeafReaderContext context) {
8997
DocIdSetIterator approximation;
9098
approximation = DocIdSetIterator.all(context.reader().maxDoc());
99+
100+
// Create a new ValueFetcher per thread.
101+
// ValueFetcher.setNextReader creates a DerivedFieldScript and internally SourceLookup and these objects are not
102+
// thread safe.
103+
final DerivedFieldValueFetcher valueFetcher = valueFetcherSupplier.get();
91104
valueFetcher.setNextReader(context);
92105
LeafSearchLookup leafSearchLookup = searchLookup.getLeafSearchLookup(context);
93106
TwoPhaseIterator twoPhase = new TwoPhaseIterator(approximation) {

‎server/src/test/java/org/opensearch/index/mapper/DerivedFieldMapperQueryTests.java

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.apache.lucene.index.IndexReader;
1616
import org.apache.lucene.index.IndexWriter;
1717
import org.apache.lucene.index.IndexWriterConfig;
18+
import org.apache.lucene.queryparser.classic.ParseException;
1819
import org.apache.lucene.search.IndexSearcher;
1920
import org.apache.lucene.search.Query;
2021
import org.apache.lucene.search.TopDocs;
@@ -24,9 +25,12 @@
2425
import org.opensearch.common.lucene.Lucene;
2526
import org.opensearch.core.index.Index;
2627
import org.opensearch.geometry.Rectangle;
28+
import org.opensearch.index.query.MatchPhrasePrefixQueryBuilder;
29+
import org.opensearch.index.query.MultiMatchQueryBuilder;
2730
import org.opensearch.index.query.QueryBuilders;
2831
import org.opensearch.index.query.QueryShardContext;
2932
import org.opensearch.index.query.TermQueryBuilder;
33+
import org.opensearch.index.search.QueryStringQueryParser;
3034
import org.opensearch.script.DerivedFieldScript;
3135

3236
import java.io.IOException;
@@ -435,7 +439,7 @@ public void execute() {
435439
}
436440
}
437441

438-
public void testObjectDerivedFields() throws IOException {
442+
public void testObjectDerivedFields() throws IOException, ParseException {
439443
MapperService mapperService = createMapperService(topMapping(b -> {
440444
b.startObject("properties");
441445
{
@@ -545,6 +549,17 @@ public void execute() {
545549
topDocs = searcher.search(query, 10);
546550
assertEquals(0, topDocs.totalHits.value);
547551

552+
query = new MatchPhrasePrefixQueryBuilder("object_field.text_field", "document number").toQuery(queryShardContext);
553+
topDocs = searcher.search(query, 10);
554+
assertEquals(0, topDocs.totalHits.value);
555+
556+
// Multi Phrase Query
557+
query = QueryBuilders.multiMatchQuery("GET", "object_field.nested_field.sub_field_1", "object_field.keyword_field")
558+
.type(MultiMatchQueryBuilder.Type.PHRASE)
559+
.toQuery(queryShardContext);
560+
topDocs = searcher.search(query, 10);
561+
assertEquals(7, topDocs.totalHits.value);
562+
548563
// Range queries of types - date, long and double
549564
query = QueryBuilders.rangeQuery("object_field.date_field").from("2024-03-20T14:20:50").toQuery(queryShardContext);
550565
topDocs = searcher.search(query, 10);
@@ -567,6 +582,11 @@ public void execute() {
567582
topDocs = searcher.search(query, 10);
568583
assertEquals(7, topDocs.totalHits.value);
569584

585+
QueryStringQueryParser queryParser = new QueryStringQueryParser(queryShardContext, "object_field.keyword_field");
586+
queryParser.parse("GE?");
587+
topDocs = searcher.search(query, 10);
588+
assertEquals(7, topDocs.totalHits.value);
589+
570590
// Regexp Query
571591
query = QueryBuilders.regexpQuery("object_field.keyword_field", ".*let.*").toQuery(queryShardContext);
572592
topDocs = searcher.search(query, 10);

‎server/src/test/java/org/opensearch/index/mapper/DerivedFieldTypeTests.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.apache.lucene.document.LongPoint;
1818
import org.apache.lucene.index.LeafReaderContext;
1919
import org.apache.lucene.index.memory.MemoryIndex;
20+
import org.apache.lucene.queries.spans.SpanMultiTermQueryWrapper;
2021
import org.apache.lucene.util.BytesRef;
2122
import org.opensearch.OpenSearchException;
2223
import org.opensearch.common.collect.Tuple;
@@ -59,6 +60,7 @@ public void testBooleanType() {
5960
assertTrue(dft.getFieldMapper() instanceof BooleanFieldMapper);
6061
assertTrue(dft.getIndexableFieldGenerator().apply(true) instanceof Field);
6162
assertTrue(dft.getIndexableFieldGenerator().apply(false) instanceof Field);
63+
assertEquals("derived", dft.typeName());
6264
}
6365

6466
public void testDateType() {
@@ -159,6 +161,22 @@ public void testGetAggregationScript_ip() throws IOException {
159161
assertEquals(new BytesRef(InetAddressPoint.encode(InetAddresses.forString((String) expected.get(0)))), result.get(0));
160162
}
161163

164+
public void testDerivedFieldValueFetcherDoesNotSupportCustomFormats() {
165+
DerivedFieldType dft = createDerivedFieldType("boolean");
166+
expectThrows(
167+
IllegalArgumentException.class,
168+
() -> dft.valueFetcher(mock(QueryShardContext.class), mock(SearchLookup.class), "yyyy-MM-dd")
169+
);
170+
}
171+
172+
public void testSpanPrefixQueryNotSupported() {
173+
DerivedFieldType dft = createDerivedFieldType("boolean");
174+
expectThrows(
175+
IllegalArgumentException.class,
176+
() -> dft.spanPrefixQuery("value", mock(SpanMultiTermQueryWrapper.SpanRewriteMethod.class), mock(QueryShardContext.class))
177+
);
178+
}
179+
162180
private static LeafSearchLookup mockValueFetcherForAggs(QueryShardContext mockContext, DerivedFieldType dft, List<Object> expected) {
163181
SearchLookup searchLookup = mock(SearchLookup.class);
164182
LeafSearchLookup leafLookup = mock(LeafSearchLookup.class);

0 commit comments

Comments
 (0)