Feedback from MVG on fields with both keyword and text subfields

Author: not-napoleon

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

@@ -1008,17 +1008,21 @@ protected String delegatingTo() {
                     }
                 };
             }
+            if (isStored()) {
+                return new BlockStoredFieldsReader.BytesFromStringsBlockLoader(name());
+            }
             /*
              * If this is a sub-text field try and return the parent's loader. Text
              * fields will always be slow to load and if the parent is exact then we
              * should use that instead.
              */
+            // TODO: should this be removed? I think SyntheticSourceHelper already does this:
             String parentField = blContext.parentField(name());
             if (parentField != null) {
                 MappedFieldType parent = blContext.lookup().fieldType(parentField);
                 if (parent.typeName().equals(KeywordFieldMapper.CONTENT_TYPE)) {
                     KeywordFieldMapper.KeywordFieldType kwd = (KeywordFieldMapper.KeywordFieldType) parent;
-                    if (kwd.hasNormalizer() == false && (kwd.hasDocValues() || kwd.isStored())) {
+                    if (kwd.hasDocValues() || kwd.isStored()) {
                         return new BlockLoader.Delegating(kwd.blockLoader(blContext)) {
                             @Override
                             protected String delegatingTo() {
@@ -1028,9 +1032,6 @@ protected String delegatingTo() {
                     }
                 }
             }
-            if (isStored()) {
-                return new BlockStoredFieldsReader.BytesFromStringsBlockLoader(name());
-            }
 
             // _ignored_source field will contain entries for this field if it is not stored
             // and there is no syntheticSourceDelegate.
@@ -1579,7 +1580,7 @@ public static KeywordFieldMapper getKeywordFieldMapperForSyntheticSource(Iterabl
             for (Mapper sub : multiFields) {
                 if (sub.typeName().equals(KeywordFieldMapper.CONTENT_TYPE)) {
                     KeywordFieldMapper kwd = (KeywordFieldMapper) sub;
-                    if (kwd.hasNormalizer() == false && (kwd.fieldType().hasDocValues() || kwd.fieldType().isStored())) {
+                    if (kwd.fieldType().hasDocValues() || kwd.fieldType().isStored()) {
                         return kwd;
                     }
                 }

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

@@ -39,14 +39,13 @@ public static Object expectedValue(Map<String, Object> fieldMapping, Object valu
         var fields = (Map<String, Object>) fieldMapping.get("fields");
         if (fields != null) {
             var keywordMultiFieldMapping = (Map<String, Object>) fields.get("kwd");
-            Object normalizer = fields.get("normalizer");
             boolean docValues = hasDocValues(keywordMultiFieldMapping, true);
             boolean store = keywordMultiFieldMapping.getOrDefault("store", false).equals(true);
             Object ignoreAbove = keywordMultiFieldMapping.get("ignore_above");
 
             // See TextFieldMapper.SyntheticSourceHelper#getKeywordFieldMapperForSyntheticSource
             // and TextFieldMapper#canUseSyntheticSourceDelegateForLoading().
-            boolean usingSyntheticSourceDelegate = normalizer == null && (docValues || store);
+            boolean usingSyntheticSourceDelegate = docValues || store;
             boolean canUseSyntheticSourceDelegateForLoading = usingSyntheticSourceDelegate && ignoreAbove == null;
             if (canUseSyntheticSourceDelegateForLoading) {
                 return KeywordFieldBlockLoaderTests.expectedValue(keywordMultiFieldMapping, value, params, testContext);

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

@@ -118,13 +118,18 @@ private Object expected(Map<String, Object> fieldMapping, Object value, BlockLoa
         boolean docValues = hasDocValues(fieldMapping, true);
         boolean store = fieldMapping.getOrDefault("store", false).equals(true);
 
-        if (normalizer == null && (docValues || store)) {
+        // if text sub field is stored, then always use that:
+        var textFieldMapping = (Map<String, Object>) ((Map<String, Object>) fieldMapping.get("fields")).get("mf");
+        if (textFieldMapping.getOrDefault("store", false).equals(true)) {
+            return TextFieldBlockLoaderTests.expectedValue(textFieldMapping, value, params, testContext);
+        }
+
+        if (docValues || store) {
             // we are using block loader of the parent field
             return KeywordFieldBlockLoaderTests.expectedValue(fieldMapping, value, params, testContext);
         }
 
         // we are using block loader of the text field itself
-        var textFieldMapping = (Map<String, Object>) ((Map<String, Object>) fieldMapping.get("fields")).get("mf");
         return TextFieldBlockLoaderTests.expectedValue(textFieldMapping, value, params, testContext);
     }
 }