Properly handle multi fields in block loaders with synthetic source enabled

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

@@ -378,7 +378,8 @@ public BlockLoader blockLoader(BlockLoaderContext blContext) {
             if (hasDocValues() && (blContext.fieldExtractPreference() != FieldExtractPreference.STORED || isSyntheticSource)) {
                 return new BlockDocValuesReader.DoublesBlockLoader(name(), l -> l / scalingFactor);
             }
-            if (isSyntheticSource) {
+            // Multi fields don't have fallback synthetic source.
+            if (isSyntheticSource && blContext.parentField(name()) == null) {
                 return new FallbackSyntheticSourceBlockLoader(fallbackSyntheticSourceBlockLoaderReader(), name()) {
                     @Override
                     public Builder builder(BlockFactory factory, int expectedCount) {

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

@@ -350,7 +350,8 @@ public BlockLoader blockLoader(BlockLoaderContext blContext) {
                 return new BlockDocValuesReader.BooleansBlockLoader(name());
             }
 
-            if (isSyntheticSource) {
+            // Multi fields don't have fallback synthetic source.
+            if (isSyntheticSource && blContext.parentField(name()) == null) {
                 return new FallbackSyntheticSourceBlockLoader(fallbackSyntheticSourceBlockLoaderReader(), name()) {
                     @Override
                     public Builder builder(BlockFactory factory, int expectedCount) {

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

@@ -948,7 +948,8 @@ public BlockLoader blockLoader(BlockLoaderContext blContext) {
                 return new BlockDocValuesReader.LongsBlockLoader(name());
             }
 
-            if (isSyntheticSource) {
+            // Multi fields don't have fallback synthetic source.
+            if (isSyntheticSource && blContext.parentField(name()) == null) {
                 return new FallbackSyntheticSourceBlockLoader(fallbackSyntheticSourceBlockLoaderReader(), name()) {
                     @Override
                     public Builder builder(BlockFactory factory, int expectedCount) {

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

@@ -316,7 +316,7 @@ protected void index(DocumentParserContext context, GeoPoint geometry) throws IO
 
     /**
      * Parser that pretends to be the main document parser, but exposes the provided geohash regardless of how the geopoint was provided
-     * in the incoming document. We rely on the fact that consumers are only ever call {@link XContentParser#textOrNull()} and never
+     * in the incoming document. We rely on the fact that consumers only ever read text from the parser and never
      * advance tokens, which is explicitly disallowed by this parser.
      */
     static class GeoHashMultiFieldParser extends FilterXContentParserWrapper {
@@ -332,6 +332,11 @@ public String textOrNull() throws IOException {
             return value;
         }
 
+        @Override
+        public String text() throws IOException {
+            return value;
+        }
+
         @Override
         public Token currentToken() {
             return Token.VALUE_STRING;
@@ -545,8 +550,9 @@ public BlockLoader blockLoader(BlockLoaderContext blContext) {
             // So we have two subcases:
             // - doc_values are enabled - _ignored_source field does not exist since we have doc_values. We will use
             // blockLoaderFromSource which reads "native" synthetic source.
-            // - doc_values are disabled - we know that _ignored_source field is present and use a special block loader.
-            if (isSyntheticSource && hasDocValues() == false) {
+            // - doc_values are disabled - we know that _ignored_source field is present and use a special block loader unless it's a multi
+            // field.
+            if (isSyntheticSource && hasDocValues() == false && blContext.parentField(name()) == null) {
                 return blockLoaderFromFallbackSyntheticSource(blContext);
             }
 

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

@@ -467,7 +467,8 @@ public BlockLoader blockLoader(BlockLoaderContext blContext) {
                 return new BlockStoredFieldsReader.BytesFromBytesRefsBlockLoader(name());
             }
 
-            if (isSyntheticSource) {
+            // Multi fields don't have fallback synthetic source.
+            if (isSyntheticSource && blContext.parentField(name()) == null) {
                 return blockLoaderFromFallbackSyntheticSource(blContext);
             }
 

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

@@ -755,7 +755,8 @@ public BlockLoader blockLoader(BlockLoaderContext blContext) {
                 return new BlockStoredFieldsReader.BytesFromBytesRefsBlockLoader(name());
             }
 
-            if (isSyntheticSource) {
+            // Multi fields don't have fallback synthetic source.
+            if (isSyntheticSource && blContext.parentField(name()) == null) {
                 return new FallbackSyntheticSourceBlockLoader(fallbackSyntheticSourceBlockLoaderReader(), name()) {
                     @Override
                     public Builder builder(BlockFactory factory, int expectedCount) {

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

@@ -1973,7 +1973,8 @@ public BlockLoader blockLoader(BlockLoaderContext blContext) {
                 return type.blockLoaderFromDocValues(name());
             }
 
-            if (isSyntheticSource) {
+            // Multi fields don't have fallback synthetic source.
+            if (isSyntheticSource && blContext.parentField(name()) == null) {
                 return type.blockLoaderFromFallbackSyntheticSource(name(), nullValue, coerce);
             }
 

server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java

@@ -35,28 +35,29 @@ protected Object expected(Map<String, Object> fieldMapping, Object value, TestCo
         var values = extractedFieldValues.values();
 
         var nullValue = switch (fieldMapping.get("null_value")) {
-            case String s -> convert(s, null);
+            case String s -> convert(s, null, false);
             case null -> null;
             default -> throw new IllegalStateException("Unexpected null_value format");
         };
 
         if (params.preference() == MappedFieldType.FieldExtractPreference.DOC_VALUES && hasDocValues(fieldMapping, true)) {
             if (values instanceof List<?> == false) {
-                var point = convert(values, nullValue);
+                var point = convert(values, nullValue, testContext.isMultifield());
                 return point != null ? point.getEncoded() : null;
             }
 
             var resultList = ((List<Object>) values).stream()
-                .map(v -> convert(v, nullValue))
+                .map(v -> convert(v, nullValue, testContext.isMultifield()))
                 .filter(Objects::nonNull)
                 .map(GeoPoint::getEncoded)
                 .sorted()
                 .toList();
             return maybeFoldList(resultList);
         }
 
+        // stored source is used
         if (params.syntheticSource() == false) {
-            return exactValuesFromSource(values, nullValue);
+            return exactValuesFromSource(values, nullValue, false);
         }
 
         // Usually implementation of block loader from source adjusts values read from source
@@ -67,25 +68,25 @@ protected Object expected(Map<String, Object> fieldMapping, Object value, TestCo
         // That is unless "synthetic_source_keep" forces fallback synthetic source again.
 
         if (testContext.forceFallbackSyntheticSource()) {
-            return exactValuesFromSource(values, nullValue);
+            return exactValuesFromSource(values, nullValue, false);
         }
 
         String syntheticSourceKeep = (String) fieldMapping.getOrDefault("synthetic_source_keep", "none");
         if (syntheticSourceKeep.equals("all")) {
-            return exactValuesFromSource(values, nullValue);
+            return exactValuesFromSource(values, nullValue, false);
         }
         if (syntheticSourceKeep.equals("arrays") && extractedFieldValues.documentHasObjectArrays()) {
-            return exactValuesFromSource(values, nullValue);
+            return exactValuesFromSource(values, nullValue, false);
         }
 
         // synthetic source and doc_values are present
         if (hasDocValues(fieldMapping, true)) {
             if (values instanceof List<?> == false) {
-                return toWKB(normalize(convert(values, nullValue)));
+                return toWKB(normalize(convert(values, nullValue, false)));
             }
 
             var resultList = ((List<Object>) values).stream()
-                .map(v -> convert(v, nullValue))
+                .map(v -> convert(v, nullValue, false))
                 .filter(Objects::nonNull)
                 .sorted(Comparator.comparingLong(GeoPoint::getEncoded))
                 .map(p -> toWKB(normalize(p)))
@@ -94,16 +95,20 @@ protected Object expected(Map<String, Object> fieldMapping, Object value, TestCo
         }
 
         // synthetic source but no doc_values so using fallback synthetic source
-        return exactValuesFromSource(values, nullValue);
+        return exactValuesFromSource(values, nullValue, false);
     }
 
     @SuppressWarnings("unchecked")
-    private Object exactValuesFromSource(Object value, GeoPoint nullValue) {
+    private Object exactValuesFromSource(Object value, GeoPoint nullValue, boolean needsMultifieldAdjustment) {
         if (value instanceof List<?> == false) {
-            return toWKB(convert(value, nullValue));
+            return toWKB(convert(value, nullValue, needsMultifieldAdjustment));
         }
 
-        var resultList = ((List<Object>) value).stream().map(v -> convert(v, nullValue)).filter(Objects::nonNull).map(this::toWKB).toList();
+        var resultList = ((List<Object>) value).stream()
+            .map(v -> convert(v, nullValue, needsMultifieldAdjustment))
+            .filter(Objects::nonNull)
+            .map(this::toWKB)
+            .toList();
         return maybeFoldList(resultList);
     }
 
@@ -163,14 +168,17 @@ private void processLeafLevel(Object value, ArrayList<Object> extracted) {
     }
 
     @SuppressWarnings("unchecked")
-    private GeoPoint convert(Object value, GeoPoint nullValue) {
+    private GeoPoint convert(Object value, GeoPoint nullValue, boolean needsMultifieldAdjustment) {
         if (value == null) {
-            return nullValue;
+            if (nullValue == null) {
+                return null;
+            }
+            return possiblyAdjustMultifieldValue(nullValue, needsMultifieldAdjustment);
         }
 
         if (value instanceof String s) {
             try {
-                return new GeoPoint(s);
+                return possiblyAdjustMultifieldValue(new GeoPoint(s), needsMultifieldAdjustment);
             } catch (Exception e) {
                 return null;
             }
@@ -180,16 +188,30 @@ private GeoPoint convert(Object value, GeoPoint nullValue) {
             if (m.get("type") != null) {
                 var coordinates = (List<Double>) m.get("coordinates");
                 // Order is GeoJSON is lon,lat
-                return new GeoPoint(coordinates.get(1), coordinates.get(0));
+                return possiblyAdjustMultifieldValue(new GeoPoint(coordinates.get(1), coordinates.get(0)), needsMultifieldAdjustment);
             } else {
-                return new GeoPoint((Double) m.get("lat"), (Double) m.get("lon"));
+                return possiblyAdjustMultifieldValue(new GeoPoint((Double) m.get("lat"), (Double) m.get("lon")), needsMultifieldAdjustment);
             }
         }
 
         // Malformed values are excluded
         return null;
     }
 
+    private GeoPoint possiblyAdjustMultifieldValue(GeoPoint point, boolean isMultifield) {
+        // geo_point multifields are parsed from a geohash representation of the original point (GeoPointFieldMapper#index)
+        // and it's not exact.
+        // So if this is a multifield we need another adjustment here.
+        // Note that this does not apply to block loader from source because in this case we parse raw original values.
+        // Same thing happens with synthetic source since it is generated from the parent field data that didn't go through multi field
+        // parsing logic.
+        if (isMultifield) {
+            return point.resetFromString(point.geohash());
+        }
+
+        return point;
+    }
+
     private GeoPoint normalize(GeoPoint point) {
         if (point == null) {
             return null;

server/src/test/java/org/elasticsearch/index/mapper/blockloader/TextFieldWithParentBlockLoaderTests.java

@@ -9,51 +9,108 @@
 
 package org.elasticsearch.index.mapper.blockloader;
 
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.elasticsearch.datageneration.DocumentGenerator;
 import org.elasticsearch.datageneration.FieldType;
+import org.elasticsearch.datageneration.MappingGenerator;
+import org.elasticsearch.datageneration.Template;
 import org.elasticsearch.datageneration.datasource.DataSourceHandler;
 import org.elasticsearch.datageneration.datasource.DataSourceRequest;
 import org.elasticsearch.datageneration.datasource.DataSourceResponse;
-import org.elasticsearch.datageneration.datasource.DefaultMappingParametersHandler;
 import org.elasticsearch.index.mapper.BlockLoaderTestCase;
+import org.elasticsearch.index.mapper.BlockLoaderTestRunner;
+import org.elasticsearch.index.mapper.MapperServiceTestCase;
+import org.elasticsearch.xcontent.XContentBuilder;
+import org.elasticsearch.xcontent.XContentType;
 
-import java.util.HashMap;
+import java.io.IOException;
 import java.util.List;
 import java.util.Map;
 
-public class TextFieldWithParentBlockLoaderTests extends BlockLoaderTestCase {
-    public TextFieldWithParentBlockLoaderTests(Params params) {
-        // keyword because we need a keyword parent field
-        super(FieldType.KEYWORD.toString(), List.of(new DataSourceHandler() {
+import static org.elasticsearch.index.mapper.BlockLoaderTestCase.buildSpecification;
+import static org.elasticsearch.index.mapper.BlockLoaderTestCase.hasDocValues;
+
+public class TextFieldWithParentBlockLoaderTests extends MapperServiceTestCase {
+    private final BlockLoaderTestCase.Params params;
+    private final BlockLoaderTestRunner runner;
+
+    @ParametersFactory(argumentFormatting = "preference=%s")
+    public static List<Object[]> args() {
+        return BlockLoaderTestCase.args();
+    }
+
+    public TextFieldWithParentBlockLoaderTests(BlockLoaderTestCase.Params params) {
+        this.params = params;
+        this.runner = new BlockLoaderTestRunner(params);
+    }
+
+    // This is similar to BlockLoaderTestCase#testBlockLoaderOfMultiField but has customizations required to properly test the case
+    // of text multi field in a keyword field.
+    public void testBlockLoaderOfParentField() throws IOException {
+        var template = new Template(Map.of("parent", new Template.Leaf("parent", FieldType.KEYWORD.toString())));
+        var specification = buildSpecification(List.of(new DataSourceHandler() {
             @Override
             public DataSourceResponse.LeafMappingParametersGenerator handle(DataSourceRequest.LeafMappingParametersGenerator request) {
-                assert request.fieldType().equals(FieldType.KEYWORD.toString());
+                // This is a bit tricky meta-logic.
+                // We want to customize mapping but to do this we need the mapping for the same field type
+                // so we use name to untangle this.
+                if (request.fieldName().equals("parent") == false) {
+                    return null;
+                }
 
-                // We need to force multi field generation
                 return new DataSourceResponse.LeafMappingParametersGenerator(() -> {
-                    var defaultSupplier = DefaultMappingParametersHandler.keywordMapping(
-                        request,
-                        DefaultMappingParametersHandler.commonMappingParameters()
-                    );
-                    var mapping = defaultSupplier.get();
+                    var dataSource = request.dataSource();
+
+                    var keywordParentMapping = dataSource.get(
+                        new DataSourceRequest.LeafMappingParametersGenerator(
+                            dataSource,
+                            "_field",
+                            FieldType.KEYWORD.toString(),
+                            request.eligibleCopyToFields(),
+                            request.dynamicMapping()
+                        )
+                    ).mappingGenerator().get();
+
+                    var textMultiFieldMapping = dataSource.get(
+                        new DataSourceRequest.LeafMappingParametersGenerator(
+                            dataSource,
+                            "_field",
+                            FieldType.TEXT.toString(),
+                            request.eligibleCopyToFields(),
+                            request.dynamicMapping()
+                        )
+                    ).mappingGenerator().get();
+
                     // we don't need this here
-                    mapping.remove("copy_to");
+                    keywordParentMapping.remove("copy_to");
 
-                    var textMultiFieldMappingSupplier = DefaultMappingParametersHandler.textMapping(request, new HashMap<>());
-                    var textMultiFieldMapping = textMultiFieldMappingSupplier.get();
                     textMultiFieldMapping.put("type", "text");
                     textMultiFieldMapping.remove("fields");
 
-                    mapping.put("fields", Map.of("txt", textMultiFieldMapping));
+                    keywordParentMapping.put("fields", Map.of("mf", textMultiFieldMapping));
 
-                    return mapping;
+                    return keywordParentMapping;
                 });
             }
-        }), params);
+        }));
+        var mapping = new MappingGenerator(specification).generate(template);
+        var fieldMapping = mapping.lookup().get("parent");
+
+        var document = new DocumentGenerator(specification).generate(template, mapping);
+        var fieldValue = document.get("parent");
+
+        Object expected = expected(fieldMapping, fieldValue, new BlockLoaderTestCase.TestContext(false, true));
+        var mappingXContent = XContentBuilder.builder(XContentType.JSON.xContent()).map(mapping.raw());
+        var mapperService = params.syntheticSource()
+            ? createSytheticSourceMapperService(mappingXContent)
+            : createMapperService(mappingXContent);
+
+        runner.runTest(mapperService, document, expected, "parent.mf");
     }
 
-    @Override
     @SuppressWarnings("unchecked")
-    protected Object expected(Map<String, Object> fieldMapping, Object value, TestContext testContext) {
+    private Object expected(Map<String, Object> fieldMapping, Object value, BlockLoaderTestCase.TestContext testContext) {
         assert fieldMapping.containsKey("fields");
 
         Object normalizer = fieldMapping.get("normalizer");
@@ -66,12 +123,7 @@ protected Object expected(Map<String, Object> fieldMapping, Object value, TestCo
         }
 
         // we are using block loader of the text field itself
-        var textFieldMapping = (Map<String, Object>) ((Map<String, Object>) fieldMapping.get("fields")).get("txt");
+        var textFieldMapping = (Map<String, Object>) ((Map<String, Object>) fieldMapping.get("fields")).get("mf");
         return TextFieldBlockLoaderTests.expectedValue(textFieldMapping, value, params, testContext);
     }
-
-    @Override
-    protected String blockLoaderFieldName(String originalName) {
-        return originalName + ".txt";
-    }
 }

test/framework/src/main/java/org/elasticsearch/datageneration/MappingGenerator.java

@@ -104,6 +104,7 @@ private void generateMapping(
                 var mappingParametersGenerator = specification.dataSource()
                     .get(
                         new DataSourceRequest.LeafMappingParametersGenerator(
+                            specification.dataSource(),
                             fieldName,
                             leaf.type(),
                             context.eligibleCopyToDestinations(),

test/framework/src/main/java/org/elasticsearch/datageneration/datasource/DataSourceRequest.java

@@ -199,6 +199,7 @@ public DataSourceResponse.ObjectArrayGenerator accept(DataSourceHandler handler)
     }
 
     record LeafMappingParametersGenerator(
+        DataSource dataSource,
         String fieldName,
         String fieldType,
         Set<String> eligibleCopyToFields,