Skip to content

Commit 91493b0

Browse files
Kubik42Kubik42
authored
[9.1] Fixed match only text block loader not working when a keyword multi field is present (#134582) (#135025)
* Fixed match only text block loader not working when a keyword multi field is present (#134582) * Fixed match only text block loader not working when a keyword multi field is present * Update docs/changelog/134582.yaml * Preemptively mute this test * [CI] Auto commit changes from spotless * Addressed feedback * Update modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldTypeTests.java Co-authored-by: Jordan Powers <[email protected]> * Preemptively mute this test * Fixed copyright * Gate tests on feature presence * [CI] Auto commit changes from spotless * Revert muted-tests to main --------- Co-authored-by: elasticsearchmachine <[email protected]> Co-authored-by: Jordan Powers <[email protected]> (cherry picked from commit 86227fb) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java * Removed non existent line in 9.1
1 parent faf031c commit 91493b0

File tree

8 files changed

+353
-106
lines changed

8 files changed

+353
-106
lines changed

‎docs/changelog/134582.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 134582
2+
summary: Fixed match only text block loader not working when a keyword multi field
3+
is present
4+
area: Mapping
5+
type: bug
6+
issues: []

‎modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java‎

Lines changed: 44 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -133,27 +133,28 @@ protected Parameter<?>[] getParameters() {
133133
return new Parameter<?>[] { meta };
134134
}
135135

136-
private MatchOnlyTextFieldType buildFieldType(MapperBuilderContext context) {
136+
private MatchOnlyTextFieldType buildFieldType(MapperBuilderContext context, MultiFields multiFields) {
137137
NamedAnalyzer searchAnalyzer = analyzers.getSearchAnalyzer();
138138
NamedAnalyzer searchQuoteAnalyzer = analyzers.getSearchQuoteAnalyzer();
139139
NamedAnalyzer indexAnalyzer = analyzers.getIndexAnalyzer();
140140
TextSearchInfo tsi = new TextSearchInfo(Defaults.FIELD_TYPE, null, searchAnalyzer, searchQuoteAnalyzer);
141-
MatchOnlyTextFieldType ft = new MatchOnlyTextFieldType(
141+
return new MatchOnlyTextFieldType(
142142
context.buildFullName(leafName()),
143143
tsi,
144144
indexAnalyzer,
145145
context.isSourceSynthetic(),
146146
meta.getValue(),
147147
withinMultiField,
148-
multiFieldsBuilder.hasSyntheticSourceCompatibleKeywordField(),
149-
storedFieldInBinaryFormat
148+
storedFieldInBinaryFormat,
149+
// match only text fields are not stored by definition
150+
TextFieldMapper.SyntheticSourceHelper.syntheticSourceDelegate(false, multiFields)
150151
);
151-
return ft;
152152
}
153153

154154
@Override
155155
public MatchOnlyTextFieldMapper build(MapperBuilderContext context) {
156-
MatchOnlyTextFieldType tft = buildFieldType(context);
156+
BuilderParams builderParams = builderParams(this, context);
157+
MatchOnlyTextFieldType tft = buildFieldType(context, builderParams.multiFields());
157158
final boolean storeSource;
158159
if (multiFieldsNotStoredByDefaultIndexVersionCheck(indexCreatedVersion)) {
159160
storeSource = context.isSourceSynthetic()
@@ -164,6 +165,7 @@ public MatchOnlyTextFieldMapper build(MapperBuilderContext context) {
164165
}
165166
return new MatchOnlyTextFieldMapper(leafName(), Defaults.FIELD_TYPE, tft, builderParams(this, context), storeSource, this);
166167
}
168+
167169
}
168170

169171
private static boolean isSyntheticSourceStoredFieldInBinaryFormat(IndexVersion indexCreatedVersion) {
@@ -191,7 +193,6 @@ public static class MatchOnlyTextFieldType extends StringFieldType {
191193
private final String originalName;
192194

193195
private final boolean withinMultiField;
194-
private final boolean hasCompatibleMultiFields;
195196
private final boolean storedFieldInBinaryFormat;
196197

197198
public MatchOnlyTextFieldType(
@@ -201,15 +202,14 @@ public MatchOnlyTextFieldType(
201202
boolean isSyntheticSource,
202203
Map<String, String> meta,
203204
boolean withinMultiField,
204-
boolean hasCompatibleMultiFields,
205-
boolean storedFieldInBinaryFormat
205+
boolean storedFieldInBinaryFormat,
206+
KeywordFieldMapper.KeywordFieldType syntheticSourceDelegate
206207
) {
207208
super(name, true, false, false, tsi, meta);
208209
this.indexAnalyzer = Objects.requireNonNull(indexAnalyzer);
209-
this.textFieldType = new TextFieldType(name, isSyntheticSource);
210+
this.textFieldType = new TextFieldType(name, isSyntheticSource, syntheticSourceDelegate);
210211
this.originalName = isSyntheticSource ? name + "._original" : null;
211212
this.withinMultiField = withinMultiField;
212-
this.hasCompatibleMultiFields = hasCompatibleMultiFields;
213213
this.storedFieldInBinaryFormat = storedFieldInBinaryFormat;
214214
}
215215

@@ -222,7 +222,7 @@ public MatchOnlyTextFieldType(String name) {
222222
Collections.emptyMap(),
223223
false,
224224
false,
225-
false
225+
null
226226
);
227227
}
228228

@@ -270,26 +270,23 @@ private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOExcepti
270270
} else {
271271
assert false : "parent field should either be stored or have doc values";
272272
}
273-
} else if (searchExecutionContext.isSourceSynthetic() && hasCompatibleMultiFields) {
274-
var mapper = (MatchOnlyTextFieldMapper) searchExecutionContext.getMappingLookup().getMapper(name());
275-
var kwd = TextFieldMapper.SyntheticSourceHelper.getKeywordFieldMapperForSyntheticSource(mapper);
273+
} else if (searchExecutionContext.isSourceSynthetic() && textFieldType.syntheticSourceDelegate() != null) {
274+
var kwd = textFieldType.syntheticSourceDelegate();
276275

277276
if (kwd != null) {
278-
var fieldType = kwd.fieldType();
279-
280-
if (fieldType.ignoreAbove().isSet()) {
281-
if (fieldType.isStored()) {
282-
return storedFieldFetcher(fieldType.name(), fieldType.originalName());
283-
} else if (fieldType.hasDocValues()) {
284-
var ifd = searchExecutionContext.getForField(fieldType, MappedFieldType.FielddataOperation.SEARCH);
285-
return combineFieldFetchers(docValuesFieldFetcher(ifd), storedFieldFetcher(fieldType.originalName()));
277+
if (kwd.ignoreAbove().isSet()) {
278+
if (kwd.isStored()) {
279+
return storedFieldFetcher(kwd.name(), kwd.originalName());
280+
} else if (kwd.hasDocValues()) {
281+
var ifd = searchExecutionContext.getForField(kwd, MappedFieldType.FielddataOperation.SEARCH);
282+
return combineFieldFetchers(docValuesFieldFetcher(ifd), storedFieldFetcher(kwd.originalName()));
286283
}
287284
}
288285

289-
if (fieldType.isStored()) {
290-
return storedFieldFetcher(fieldType.name());
291-
} else if (fieldType.hasDocValues()) {
292-
var ifd = searchExecutionContext.getForField(fieldType, MappedFieldType.FielddataOperation.SEARCH);
286+
if (kwd.isStored()) {
287+
return storedFieldFetcher(kwd.name());
288+
} else if (kwd.hasDocValues()) {
289+
var ifd = searchExecutionContext.getForField(kwd, MappedFieldType.FielddataOperation.SEARCH);
293290
return docValuesFieldFetcher(ifd);
294291
} else {
295292
assert false : "multi field should either be stored or have doc values";
@@ -512,7 +509,7 @@ public Query phrasePrefixQuery(TokenStream stream, int slop, int maxExpansions,
512509
return toQuery(query, queryShardContext);
513510
}
514511

515-
private static class BytesFromMixedStringsBytesRefBlockLoader extends BlockStoredFieldsReader.StoredFieldsBlockLoader {
512+
static class BytesFromMixedStringsBytesRefBlockLoader extends BlockStoredFieldsReader.StoredFieldsBlockLoader {
516513
BytesFromMixedStringsBytesRefBlockLoader(String field) {
517514
super(field);
518515
}
@@ -543,12 +540,27 @@ protected BytesRef toBytesRef(Object v) {
543540
@Override
544541
public BlockLoader blockLoader(BlockLoaderContext blContext) {
545542
if (textFieldType.isSyntheticSource()) {
546-
if (storedFieldInBinaryFormat) {
547-
return new BlockStoredFieldsReader.BytesFromBytesRefsBlockLoader(storedFieldNameForSyntheticSource());
548-
} else {
549-
return new BytesFromMixedStringsBytesRefBlockLoader(storedFieldNameForSyntheticSource());
543+
// if there is no synthetic source delegate, then this match only text field would've created StoredFields for us to use
544+
if (textFieldType.syntheticSourceDelegate() == null) {
545+
if (storedFieldInBinaryFormat) {
546+
return new BlockStoredFieldsReader.BytesFromBytesRefsBlockLoader(storedFieldNameForSyntheticSource());
547+
} else {
548+
return new BytesFromMixedStringsBytesRefBlockLoader(storedFieldNameForSyntheticSource());
549+
}
550+
}
551+
552+
// otherwise, delegate block loading to the synthetic source delegate if possible
553+
if (textFieldType.canUseSyntheticSourceDelegateForLoading()) {
554+
return new BlockLoader.Delegating(textFieldType.syntheticSourceDelegate().blockLoader(blContext)) {
555+
@Override
556+
protected String delegatingTo() {
557+
return textFieldType.syntheticSourceDelegate().name();
558+
}
559+
};
550560
}
551561
}
562+
563+
// fallback to _source (synthetic or not)
552564
SourceValueFetcher fetcher = SourceValueFetcher.toString(blContext.sourcePaths(name()));
553565
// MatchOnlyText never has norms, so we have to use the field names field
554566
BlockSourceReader.LeafIteratorLookup lookup = BlockSourceReader.lookupFromFieldNames(blContext.fieldNames(), name());

‎modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldTypeTests.java‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
package org.elasticsearch.index.mapper.extras;
1010

1111
import org.apache.lucene.analysis.TokenStream;
12+
import org.apache.lucene.document.FieldType;
1213
import org.apache.lucene.index.Term;
1314
import org.apache.lucene.queries.intervals.Intervals;
1415
import org.apache.lucene.queries.intervals.IntervalsSource;
@@ -27,20 +28,38 @@
2728
import org.apache.lucene.tests.analysis.Token;
2829
import org.apache.lucene.util.BytesRef;
2930
import org.elasticsearch.ElasticsearchException;
31+
import org.elasticsearch.cluster.metadata.IndexMetadata;
3032
import org.elasticsearch.common.lucene.BytesRefs;
33+
import org.elasticsearch.common.lucene.Lucene;
3134
import org.elasticsearch.common.lucene.search.AutomatonQueries;
3235
import org.elasticsearch.common.lucene.search.MultiPhrasePrefixQuery;
36+
import org.elasticsearch.common.settings.Settings;
3337
import org.elasticsearch.common.unit.Fuzziness;
38+
import org.elasticsearch.index.IndexMode;
39+
import org.elasticsearch.index.IndexSettings;
40+
import org.elasticsearch.index.IndexVersion;
41+
import org.elasticsearch.index.analysis.NamedAnalyzer;
42+
import org.elasticsearch.index.mapper.BlockLoader;
43+
import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
3444
import org.elasticsearch.index.mapper.FieldTypeTestCase;
45+
import org.elasticsearch.index.mapper.KeywordFieldMapper;
3546
import org.elasticsearch.index.mapper.MappedFieldType;
47+
import org.elasticsearch.index.mapper.MappingParserContext;
48+
import org.elasticsearch.index.mapper.TextFieldMapper;
49+
import org.elasticsearch.index.mapper.TextSearchInfo;
3650
import org.elasticsearch.index.mapper.extras.MatchOnlyTextFieldMapper.MatchOnlyTextFieldType;
51+
import org.elasticsearch.script.ScriptCompiler;
3752
import org.hamcrest.Matchers;
3853

3954
import java.io.IOException;
4055
import java.util.ArrayList;
4156
import java.util.Arrays;
57+
import java.util.Collections;
4258
import java.util.List;
4359

60+
import static org.mockito.Mockito.doReturn;
61+
import static org.mockito.Mockito.mock;
62+
4463
public class MatchOnlyTextFieldTypeTests extends FieldTypeTestCase {
4564

4665
public void testTermQuery() {
@@ -205,4 +224,149 @@ public void testRangeIntervals() {
205224
((SourceIntervalsSource) rangeIntervals).getIntervalsSource()
206225
);
207226
}
227+
228+
public void test_block_loader_uses_stored_fields_for_loading_when_synthetic_source_delegate_is_absent() {
229+
// given
230+
MatchOnlyTextFieldMapper.MatchOnlyTextFieldType ft = new MatchOnlyTextFieldMapper.MatchOnlyTextFieldType(
231+
"parent",
232+
new TextSearchInfo(TextFieldMapper.Defaults.FIELD_TYPE, null, Lucene.STANDARD_ANALYZER, Lucene.STANDARD_ANALYZER),
233+
mock(NamedAnalyzer.class),
234+
true,
235+
Collections.emptyMap(),
236+
false,
237+
false,
238+
null
239+
);
240+
241+
// when
242+
BlockLoader blockLoader = ft.blockLoader(mock(MappedFieldType.BlockLoaderContext.class));
243+
244+
// then
245+
// verify that we delegate block loading to the synthetic source delegate
246+
assertThat(blockLoader, Matchers.instanceOf(MatchOnlyTextFieldType.BytesFromMixedStringsBytesRefBlockLoader.class));
247+
}
248+
249+
public void test_block_loader_uses_synthetic_source_delegate_when_ignore_above_is_not_set() {
250+
// given
251+
KeywordFieldMapper.KeywordFieldType syntheticSourceDelegate = new KeywordFieldMapper.KeywordFieldType(
252+
"child",
253+
true,
254+
true,
255+
Collections.emptyMap()
256+
);
257+
258+
MatchOnlyTextFieldMapper.MatchOnlyTextFieldType ft = new MatchOnlyTextFieldMapper.MatchOnlyTextFieldType(
259+
"parent",
260+
new TextSearchInfo(TextFieldMapper.Defaults.FIELD_TYPE, null, Lucene.STANDARD_ANALYZER, Lucene.STANDARD_ANALYZER),
261+
mock(NamedAnalyzer.class),
262+
true,
263+
Collections.emptyMap(),
264+
false,
265+
false,
266+
syntheticSourceDelegate
267+
);
268+
269+
// when
270+
BlockLoader blockLoader = ft.blockLoader(mock(MappedFieldType.BlockLoaderContext.class));
271+
272+
// then
273+
// verify that we delegate block loading to the synthetic source delegate
274+
assertThat(blockLoader, Matchers.instanceOf(BlockLoader.Delegating.class));
275+
}
276+
277+
public void test_block_loader_does_not_use_synthetic_source_delegate_when_ignore_above_is_set() {
278+
// given
279+
Settings settings = Settings.builder()
280+
.put(IndexMetadata.SETTING_VERSION_CREATED, IndexVersion.current())
281+
.put(IndexSettings.MODE.getKey(), IndexMode.STANDARD)
282+
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
283+
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)
284+
.build();
285+
IndexSettings indexSettings = new IndexSettings(IndexMetadata.builder("index").settings(settings).build(), settings);
286+
MappingParserContext mappingParserContext = mock(MappingParserContext.class);
287+
doReturn(settings).when(mappingParserContext).getSettings();
288+
doReturn(indexSettings).when(mappingParserContext).getIndexSettings();
289+
doReturn(mock(ScriptCompiler.class)).when(mappingParserContext).scriptCompiler();
290+
291+
KeywordFieldMapper.Builder builder = new KeywordFieldMapper.Builder("child", mappingParserContext);
292+
builder.ignoreAbove(123);
293+
294+
KeywordFieldMapper.KeywordFieldType syntheticSourceDelegate = new KeywordFieldMapper.KeywordFieldType(
295+
"child",
296+
mock(FieldType.class),
297+
mock(NamedAnalyzer.class),
298+
mock(NamedAnalyzer.class),
299+
mock(NamedAnalyzer.class),
300+
builder,
301+
true
302+
);
303+
304+
MatchOnlyTextFieldMapper.MatchOnlyTextFieldType ft = new MatchOnlyTextFieldMapper.MatchOnlyTextFieldType(
305+
"parent",
306+
new TextSearchInfo(TextFieldMapper.Defaults.FIELD_TYPE, null, Lucene.STANDARD_ANALYZER, Lucene.STANDARD_ANALYZER),
307+
mock(NamedAnalyzer.class),
308+
true,
309+
Collections.emptyMap(),
310+
false,
311+
false,
312+
syntheticSourceDelegate
313+
);
314+
315+
// when
316+
MappedFieldType.BlockLoaderContext blContext = mock(MappedFieldType.BlockLoaderContext.class);
317+
doReturn(FieldNamesFieldMapper.FieldNamesFieldType.get(false)).when(blContext).fieldNames();
318+
BlockLoader blockLoader = ft.blockLoader(blContext);
319+
320+
// then
321+
// verify that we don't delegate anything
322+
assertThat(blockLoader, Matchers.not(Matchers.instanceOf(BlockLoader.Delegating.class)));
323+
}
324+
325+
public void test_block_loader_does_not_use_synthetic_source_delegate_when_ignore_above_is_set_at_index_level() {
326+
// given
327+
Settings settings = Settings.builder()
328+
.put(IndexMetadata.SETTING_VERSION_CREATED, IndexVersion.current())
329+
.put(IndexSettings.MODE.getKey(), IndexMode.STANDARD)
330+
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
331+
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)
332+
.put(IndexSettings.IGNORE_ABOVE_SETTING.getKey(), 123)
333+
.build();
334+
IndexSettings indexSettings = new IndexSettings(IndexMetadata.builder("index").settings(settings).build(), settings);
335+
MappingParserContext mappingParserContext = mock(MappingParserContext.class);
336+
doReturn(settings).when(mappingParserContext).getSettings();
337+
doReturn(indexSettings).when(mappingParserContext).getIndexSettings();
338+
doReturn(mock(ScriptCompiler.class)).when(mappingParserContext).scriptCompiler();
339+
340+
KeywordFieldMapper.Builder builder = new KeywordFieldMapper.Builder("child", mappingParserContext);
341+
342+
KeywordFieldMapper.KeywordFieldType syntheticSourceDelegate = new KeywordFieldMapper.KeywordFieldType(
343+
"child",
344+
mock(FieldType.class),
345+
mock(NamedAnalyzer.class),
346+
mock(NamedAnalyzer.class),
347+
mock(NamedAnalyzer.class),
348+
builder,
349+
true
350+
);
351+
352+
MatchOnlyTextFieldMapper.MatchOnlyTextFieldType ft = new MatchOnlyTextFieldMapper.MatchOnlyTextFieldType(
353+
"parent",
354+
new TextSearchInfo(TextFieldMapper.Defaults.FIELD_TYPE, null, Lucene.STANDARD_ANALYZER, Lucene.STANDARD_ANALYZER),
355+
mock(NamedAnalyzer.class),
356+
true,
357+
Collections.emptyMap(),
358+
false,
359+
false,
360+
syntheticSourceDelegate
361+
);
362+
363+
// when
364+
MappedFieldType.BlockLoaderContext blContext = mock(MappedFieldType.BlockLoaderContext.class);
365+
doReturn(FieldNamesFieldMapper.FieldNamesFieldType.get(false)).when(blContext).fieldNames();
366+
BlockLoader blockLoader = ft.blockLoader(blContext);
367+
368+
// then
369+
// verify that we don't delegate anything
370+
assertThat(blockLoader, Matchers.not(Matchers.instanceOf(BlockLoader.Delegating.class)));
371+
}
208372
}

‎plugins/mapper-annotated-text/src/main/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapper.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ private AnnotatedTextFieldType buildFieldType(FieldType fieldType, MapperBuilder
135135
store.getValue(),
136136
tsi,
137137
context.isSourceSynthetic(),
138-
TextFieldMapper.SyntheticSourceHelper.syntheticSourceDelegate(fieldType, multiFields),
138+
TextFieldMapper.SyntheticSourceHelper.syntheticSourceDelegate(fieldType.stored(), multiFields),
139139
meta.getValue()
140140
);
141141
}

‎server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ public class MapperFeatures implements FeatureSpecification {
3838
public static final NodeFeature SORT_FIELDS_CHECK_FOR_NESTED_OBJECT_FIX = new NodeFeature("mapper.nested.sorting_fields_check_fix");
3939
public static final NodeFeature DYNAMIC_HANDLING_IN_COPY_TO = new NodeFeature("mapper.copy_to.dynamic_handling");
4040
public static final NodeFeature DOC_VALUES_SKIPPER = new NodeFeature("mapper.doc_values_skipper");
41+
public static final NodeFeature MATCH_ONLY_TEXT_BLOCK_LOADER_FIX = new NodeFeature("mapper.match_only_text_block_loader_fix");
42+
4143
static final NodeFeature UKNOWN_FIELD_MAPPING_UPDATE_ERROR_MESSAGE = new NodeFeature(
4244
"mapper.unknown_field_mapping_update_error_message"
4345
);
@@ -80,7 +82,8 @@ public Set<NodeFeature> getTestFeatures() {
8082
SEARCH_LOAD_PER_SHARD,
8183
SPARSE_VECTOR_INDEX_OPTIONS_FEATURE,
8284
PATTERNED_TEXT,
83-
MULTI_FIELD_UNICODE_OPTIMISATION_FIX
85+
MULTI_FIELD_UNICODE_OPTIMISATION_FIX,
86+
MATCH_ONLY_TEXT_BLOCK_LOADER_FIX
8487
);
8588
}
8689
}

0 commit comments

Comments
 (0)