Cleaned up

Author: Kubik42

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

@@ -249,10 +249,10 @@ private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOExcepti
             if (searchExecutionContext.isSourceSynthetic()) {
                 if (isWithinMultiField) {
                     // fetch the value from parent
-                    return parentFieldValueFetcher(searchExecutionContext);
+                    return parentFieldFetcher(searchExecutionContext);
                 } else if (textFieldType.syntheticSourceDelegate().isPresent()) {
                     // otherwise, if there is a delegate field, fetch the value from it
-                    return delegateFieldValueFetcher(searchExecutionContext, textFieldType.syntheticSourceDelegate().get());
+                    return delegateFieldFetcher(searchExecutionContext, textFieldType.syntheticSourceDelegate().get());
                 } else {
                     // otherwise, fetch the value from self
                     return storedFieldFetcher(name(), syntheticSourceFallbackFieldName());
@@ -284,9 +284,9 @@ private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOExcepti
         }
 
         /**
-         * Returns a function that will fetch value from a parent field.
+         * Returns a function that will fetch fields from the parent field.
          */
-        private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOException>> parentFieldValueFetcher(
+        private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOException>> parentFieldFetcher(
             final SearchExecutionContext searchExecutionContext
         ) {
             assert searchExecutionContext.isSourceSynthetic() : "Synthetic source should be enabled";
@@ -316,9 +316,9 @@ private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOExcepti
         }
 
         /**
-         * Returns a function that will fetch values from a synthetic source delegate.
+         * Returns a function that will fetch the fields from the delegate field (ex. keyword multi field).
          */
-        private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOException>> delegateFieldValueFetcher(
+        private IOFunction<LeafReaderContext, CheckedIntFunction<List<Object>, IOException>> delegateFieldFetcher(
             final SearchExecutionContext searchExecutionContext,
             final KeywordFieldMapper.KeywordFieldType keywordDelegate
         ) {
@@ -715,11 +715,11 @@ protected SyntheticSourceSupport syntheticSourceSupport() {
     }
 
     private SourceLoader.SyntheticFieldLoader syntheticFieldLoader(String fullFieldName, String leafFieldName) {
-        // match_only_text is not stored for space efficiency, except when synthetic source is enabled as synthetic source
-        // needs something to load to reconstruct source. In such cases, we *might* store this field or we *might* rely
-        // on a delegate field if one exists. Because of this uncertainty, we need multiple field loaders.
+        // we're using two field loaders here because we don't know who was responsible for storing this field to support synthetic source
+        // on one hand, if a delegate keyword field exists, then it might've stored the field. However, this is not true if said field was
+        // ignored during indexing (ex. it tripped ignore_above). Because of this uncertainty, we need multiple field loaders.
 
-        // first field loader, representing this field
+        // first field loader - to check whether the field's value was stored under this match_only_text field
         final String fieldName = fieldType().syntheticSourceFallbackFieldName();
         final var thisFieldLayer = new CompositeSyntheticFieldLoader.StoredFieldLayer(fieldName) {
             @Override
@@ -735,7 +735,7 @@ protected void writeValue(Object value, XContentBuilder b) throws IOException {
 
         final CompositeSyntheticFieldLoader fieldLoader = new CompositeSyntheticFieldLoader(leafFieldName, fullFieldName, thisFieldLayer);
 
-        // second loader, representing a delegate field, if one exists
+        // second loader - to check whether the field's value was stored by a keyword delegate field
         var kwd = TextFieldMapper.SyntheticSourceHelper.getKeywordFieldMapperForSyntheticSource(this);
         if (kwd != null) {
             // merge the two field loaders into one

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

@@ -625,10 +625,10 @@ protected void write(XContentBuilder b, Object value) throws IOException {
     }
 
     private SourceLoader.SyntheticFieldLoader syntheticFieldLoader(String fullFieldName, String leafFieldName) {
-        // since we don't know whether the delegate field loader can be used for synthetic source until parsing, we
-        // need to check both this field and the delegate
+        // since we don't know whether the delegate field loader can be used for synthetic source until parsing, we need to check both this
+        // field and the delegate
 
-        // first field loader, representing this field
+        // first field loader - to check whether the field's value was stored under this match_only_text field
         final String fieldName = fieldType().syntheticSourceFallbackFieldName();
         final var thisFieldLayer = new CompositeSyntheticFieldLoader.StoredFieldLayer(fieldName) {
             @Override
@@ -639,7 +639,7 @@ protected void writeValue(Object value, XContentBuilder b) throws IOException {
 
         final CompositeSyntheticFieldLoader fieldLoader = new CompositeSyntheticFieldLoader(leafFieldName, fullFieldName, thisFieldLayer);
 
-        // second loader, representing a delegate field, if one exists
+        // second loader - to check whether the field's value was stored by a keyword delegate field
         var kwd = TextFieldMapper.SyntheticSourceHelper.getKeywordFieldMapperForSyntheticSource(this);
         if (kwd != null) {
             // merge the two field loaders into one

server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java

@@ -341,7 +341,7 @@ public boolean newDynamicStringField(DocumentParserContext context, String name)
                         indexSettings.getIndexVersionCreated(),
                         indexSettings.getMode(),
                         context.indexAnalyzers(),
-                        mapperBuilderContext.isSourceSynthetic(),
+                        SourceFieldMapper.isSynthetic(indexSettings),
                         false
                     ).addMultiField(
                         new KeywordFieldMapper.Builder("keyword", context.indexSettings().getIndexVersionCreated(), true).ignoreAbove(256)

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

@@ -240,12 +240,12 @@ private static FielddataFrequencyFilter parseFrequencyFilter(String name, Mappin
     public static class Builder extends TextFamilyFieldMapper.Builder {
 
         private final IndexVersion indexCreatedVersion;
+        private final Parameter<Boolean> store;
         private final Parameter<Boolean> norms;
 
         private final IndexMode indexMode;
 
         private final Parameter<Boolean> index = Parameter.indexParam(m -> ((TextFieldMapper) m).index, true);
-        private Parameter<Boolean> store;
 
         final Parameter<SimilarityProvider> similarity = TextParams.similarity(m -> ((TextFieldMapper) m).similarity);
 
@@ -1606,10 +1606,10 @@ protected void write(XContentBuilder b, Object value) throws IOException {
     }
 
     private SourceLoader.SyntheticFieldLoader syntheticFieldLoader(String fullFieldName, String leafFieldName) {
-        // since we don't know whether the delegate field loader can be used for synthetic source until parsing, we
-        // need to check both this field and the delegate
+        // since we don't know whether the delegate field loader can be used for synthetic source until parsing, we need to check both this
+        // field and the delegate
 
-        // first field loader, representing this field
+        // first field loader - to check whether the field's value was stored under this match_only_text field
         final String fieldName = fieldType().syntheticSourceFallbackFieldName();
         final var thisFieldLayer = new CompositeSyntheticFieldLoader.StoredFieldLayer(fieldName) {
             @Override
@@ -1620,7 +1620,7 @@ protected void writeValue(Object value, XContentBuilder b) throws IOException {
 
         final CompositeSyntheticFieldLoader fieldLoader = new CompositeSyntheticFieldLoader(leafFieldName, fullFieldName, thisFieldLayer);
 
-        // second loader, representing a delegate field, if one exists
+        // second loader - to check whether the field's value was stored by a keyword delegate field
         var kwd = TextFieldMapper.SyntheticSourceHelper.getKeywordFieldMapperForSyntheticSource(this);
         if (kwd != null) {
             // merge the two field loaders into one

server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

@@ -978,7 +978,7 @@ public void test_isIgnoreAboveSet_returns_true_when_ignore_above_is_provided() t
         assertTrue(mapper.fieldType().isIgnoreAboveSet());
     }
 
-    public void test_isIgnoreAboveSet_returns_true_when_ignore_above_is_missing() throws IOException {
+    public void test_isIgnoreAboveSet_returns_false_when_ignore_above_is_missing() throws IOException {
         // given
         MapperService mapperService = createSytheticSourceMapperService(mapping(b -> {
             b.startObject("potato");
@@ -1058,7 +1058,7 @@ public void test_value_is_not_ignored_when_it_does_not_exceed_ignore_above_and_f
         assertFalse(mapper.fieldType().isIgnored("this value is too short to be ignored"));
     }
 
-    public void test_value_exceeds_ignore_above_and_field_is_not_a_multi_field() throws IOException {
+    public void test_value_is_stored_when_it_exceeds_ignore_above_and_field_is_not_a_multi_field() throws IOException {
         // given
         MapperService mapperService = createSytheticSourceMapperService(mapping(b -> {
             b.startObject("potato");
@@ -1077,7 +1077,7 @@ public void test_value_exceeds_ignore_above_and_field_is_not_a_multi_field() thr
         assertThat(doc.rootDoc().getField(mapper.fieldType().syntheticSourceFallbackFieldName()), Matchers.notNullValue());
     }
 
-    public void test_value_exceeds_ignore_above_and_field_is_a_multi_field() throws IOException {
+    public void test_value_is_not_stored_when_it_exceeds_ignore_above_and_field_is_a_multi_field() throws IOException {
         // given
         MapperService mapperService = createSytheticSourceMapperService(mapping(b -> {
             b.startObject("potato").field("type", "text");
@@ -1122,12 +1122,11 @@ public void test_value_does_not_exceed_ignore_above_and_field_is_a_multi_field()
 
         // when
         KeywordFieldMapper mapper = (KeywordFieldMapper) mapperService.documentMapper().mappers().getMapper("potato.tomato");
-        ParsedDocument doc = mapperService.documentMapper().parse(source(b -> { b.field("potato", "this value is too long"); }));
+        ParsedDocument doc = mapperService.documentMapper().parse(source(b -> { b.field("potato", "this value is short"); }));
 
         // then
 
-        // despite exceeding ignore_above, because the field is a multi field, we don't need to store anything extra in Lucene as _source
-        // can be reconstructed via the parent
+        // we don't expect to store anything extra when ignore_above isn't tripped as we can rely on doc_values for source
         assertThat(doc.rootDoc().getField(mapper.fieldType().syntheticSourceFallbackFieldName()), Matchers.nullValue());
     }
 
@@ -1146,6 +1145,8 @@ public void test_value_exceeds_ignore_above_when_synthetic_source_disabled() thr
 
         // then
 
+        // since synthetic source is disabled, the fallback field name shouldn't exist
+        assertThat(mapper.fieldType().syntheticSourceFallbackFieldName(), Matchers.nullValue());
         // despite exceeding ignore_above, because synthetic source is disabled, we don't expect to store anything
         assertThat(doc.rootDoc().getField(mapper.fieldType() + "_original"), Matchers.nullValue());
     }