Avoid double fields resolution in FetchFieldsPhase

We have moved fetching of metadata fields to FetchFieldsPhase with elastic#106325 . For backwards compatibility, metadata fields requested via `stored_fields` are returned even when they match a wildcard expression, which differs from how patterns are resolved via `fields`. Instead of calling getMatchingFieldNames and getFieldType twice per field, this distinction is now included in FieldFetcher#create via a predicate
javanna · Apr 15, 2024 · 012267a · 012267a
1 parent 4dfcb08
commit 012267a
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 21 deletions.
diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java
@@ -13,7 +13,6 @@
 import org.elasticsearch.index.mapper.IdFieldMapper;
 import org.elasticsearch.index.mapper.IgnoredFieldMapper;
 import org.elasticsearch.index.mapper.LegacyTypeFieldMapper;
-import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.RoutingFieldMapper;
 import org.elasticsearch.index.mapper.SourceFieldMapper;
 import org.elasticsearch.index.query.SearchExecutionContext;
@@ -57,31 +56,29 @@ public FetchSubPhaseProcessor getProcessor(FetchContext fetchContext) {
         final FieldFetcher fieldFetcher = fetchFieldsContext == null ? null
             : fetchFieldsContext.fields() == null ? null
             : fetchFieldsContext.fields().isEmpty() ? null
-            : FieldFetcher.create(searchExecutionContext, fetchFieldsContext.fields());
+            : FieldFetcher.create(searchExecutionContext, fetchFieldsContext.fields(), ft -> {
+                // we want to skip metadata fields if we have a wildcard pattern
+                return searchExecutionContext.isMetadataField(ft.name()) == false;
+            });
 
         final FieldFetcher metadataFieldFetcher;
         if (storedFieldsContext != null
             && storedFieldsContext.fieldNames() != null
             && storedFieldsContext.fieldNames().isEmpty() == false) {
             final Set<FieldAndFormat> metadataFields = new HashSet<>(DEFAULT_METADATA_FIELDS);
             for (final String storedField : storedFieldsContext.fieldNames()) {
-                final Set<String> matchingFieldNames = searchExecutionContext.getMatchingFieldNames(storedField);
-                for (final String matchingFieldName : matchingFieldNames) {
-                    if (SourceFieldMapper.NAME.equals(matchingFieldName) || IdFieldMapper.NAME.equals(matchingFieldName)) {
-                        continue;
-                    }
-                    final MappedFieldType fieldType = searchExecutionContext.getFieldType(matchingFieldName);
-                    // NOTE: checking if the field is stored is required for backward compatibility reasons and to make
-                    // sure we also handle here stored fields requested via `stored_fields`, which was previously a
-                    // responsibility of StoredFieldsPhase.
-                    if (searchExecutionContext.isMetadataField(matchingFieldName) && fieldType.isStored()) {
-                        metadataFields.add(new FieldAndFormat(matchingFieldName, null));
-                    }
-                }
+                metadataFields.add(new FieldAndFormat(storedField, null));
             }
-            metadataFieldFetcher = FieldFetcher.create(searchExecutionContext, metadataFields);
+            metadataFieldFetcher = FieldFetcher.create(
+                searchExecutionContext,
+                metadataFields,
+                ft -> ft.name().equals(SourceFieldMapper.NAME) == false
+                    && ft.name().equals(IdFieldMapper.NAME)
+                    && searchExecutionContext.isMetadataField(ft.name())
+                    && ft.isStored()
+            );
         } else {
-            metadataFieldFetcher = FieldFetcher.create(searchExecutionContext, DEFAULT_METADATA_FIELDS);
+            metadataFieldFetcher = FieldFetcher.create(searchExecutionContext, DEFAULT_METADATA_FIELDS, ft -> true);
         }
         return new FetchSubPhaseProcessor() {
             @Override

diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
@@ -28,6 +28,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Predicate;
 
 /**
  * A helper class to {@link FetchFieldsPhase} that's initialized with a list of field patterns to fetch.
@@ -40,7 +41,11 @@ private record ResolvedField(String field, String matchingPattern, MappedFieldTy
     /**
      * Build a FieldFetcher for a given search context and collection of fields and formats
      */
-    public static FieldFetcher create(SearchExecutionContext context, Collection<FieldAndFormat> fieldAndFormats) {
+    public static FieldFetcher create(
+        SearchExecutionContext context,
+        Collection<FieldAndFormat> fieldAndFormats,
+        Predicate<MappedFieldType> patternIncludePredicate
+    ) {
 
         List<String> unmappedFetchPattern = new ArrayList<>();
         List<ResolvedField> resolvedFields = new ArrayList<>();
@@ -55,10 +60,9 @@ public static FieldFetcher create(SearchExecutionContext context, Collection<Fie
             for (String field : context.getMatchingFieldNames(fieldPattern)) {
                 MappedFieldType ft = context.getFieldType(field);
                 // we want to skip metadata fields if we have a wildcard pattern
-                if (context.isMetadataField(field) && matchingPattern != null) {
-                    continue;
+                if (matchingPattern != null && patternIncludePredicate.test(ft)) {
+                    resolvedFields.add(new ResolvedField(field, matchingPattern, ft, fieldAndFormat.format));
                 }
-                resolvedFields.add(new ResolvedField(field, matchingPattern, ft, fieldAndFormat.format));
             }
         }