Avoid attempting to load the same empty field twice in fetch phase

docs/changelog/107551.yaml

@@ -0,0 +1,5 @@
+pr: 107551
+summary: Avoid attempting to load the same empty field twice in fetch phase
+area: Search
+type: bug
+issues: []

docs/reference/search/profile.asciidoc

@@ -194,7 +194,7 @@ The API returns the following result:
             "load_source_count": 5
           },
           "debug": {
-            "stored_fields": ["_id", "_routing", "_source"]
+            "stored_fields": ["_id", "_ignored", "_routing", "_source"]
           },
           "children": [
             {
@@ -1051,7 +1051,7 @@ And here is the fetch profile:
             "load_source_count": 5
           },
           "debug": {
-            "stored_fields": ["_id", "_routing", "_source"]
+            "stored_fields": ["_id", "_ignored", "_routing", "_source"]
           },
           "children": [
             {

modules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/30_inner_hits.yml

@@ -140,7 +140,7 @@ profile fetch:
   - gt: { profile.shards.0.fetch.breakdown.next_reader: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields_count: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields: 0 }
-  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _routing, _source] }
+  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _ignored, _routing, _source] }
   - length: { profile.shards.0.fetch.children: 4 }
   - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase }
   - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 }

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.yml

@@ -41,7 +41,7 @@ fetch fields:
   - gt: { profile.shards.0.fetch.breakdown.next_reader: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields_count: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields: 0 }
-  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _routing, _source] }
+  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _ignored, _routing, _source] }
   - length: { profile.shards.0.fetch.children: 2 }
   - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase }
   - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 }
@@ -74,7 +74,7 @@ fetch source:
   - gt: { profile.shards.0.fetch.breakdown.next_reader: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields_count: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields: 0 }
-  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _routing, _source] }
+  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _ignored, _routing, _source] }
   - length: { profile.shards.0.fetch.children: 3 }
   - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase }
   - match: { profile.shards.0.fetch.children.1.type: FetchSourcePhase }
@@ -139,7 +139,7 @@ fetch nested source:
   - gt: { profile.shards.0.fetch.breakdown.next_reader: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields_count: 0 }
   - gt: { profile.shards.0.fetch.breakdown.load_stored_fields: 0 }
-  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _routing, _source] }
+  - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _ignored, _routing, _source] }
   - length: { profile.shards.0.fetch.children: 4 }
   - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase }
   - match: { profile.shards.0.fetch.children.1.type: FetchSourcePhase }

server/src/internalClusterTest/java/org/elasticsearch/search/source/MetadataFetchingIT.java

@@ -168,4 +168,18 @@ public void testInvalid() {
             assertThat(exc.getMessage(), equalTo("cannot combine _none_ with other fields"));
         }
     }
+
+    public void testFetchId() {
+        assertAcked(prepareCreate("test"));
+        ensureGreen();
+
+        prepareIndex("test").setId("1").setSource("field", "value").get();
+        refresh();
+
+        assertResponse(prepareSearch("test").addFetchField("_id"), response -> {
+            assertEquals(1, response.getHits().getHits().length);
+            assertEquals("1", response.getHits().getAt(0).getId());
+            assertEquals("1", response.getHits().getAt(0).field("_id").getValue());
+        });
+    }
 }

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

@@ -112,9 +112,13 @@ private SearchHits buildSearchHits(SearchContext context, int[] docIdsToLoad, Pr
         context.getSearchExecutionContext().setLookupProviders(sourceProvider, ctx -> fieldLookupProvider);
 
         List<FetchSubPhaseProcessor> processors = getProcessors(context.shardTarget(), fetchContext, profiler);
-
         StoredFieldsSpec storedFieldsSpec = StoredFieldsSpec.build(processors, FetchSubPhaseProcessor::storedFieldsSpec);
         storedFieldsSpec = storedFieldsSpec.merge(new StoredFieldsSpec(false, false, sourceLoader.requiredStoredFields()));
+        // Ideally the required stored fields would be provided as constructor argument a few lines above, but that requires moving
+        // the getProcessors call to before the setLookupProviders call, which causes weird issues in InnerHitsPhase.
+        // setLookupProviders resets the SearchLookup used throughout the rest of the fetch phase, which StoredValueFetchers rely on
+        // to retrieve stored fields, and InnerHitsPhase is the last sub-fetch phase and re-runs the entire fetch phase.
+        fieldLookupProvider.setPreloadedStoredFieldNames(storedFieldsSpec.requiredStoredFields());
 
         StoredFieldLoader storedFieldLoader = profiler.storedFields(StoredFieldLoader.fromSpec(storedFieldsSpec));
         IdLoader idLoader = context.newIdLoader();
@@ -164,7 +168,7 @@ protected SearchHit nextDoc(int doc) throws IOException {
                     leafIdLoader
                 );
                 sourceProvider.source = hit.source();
-                fieldLookupProvider.storedFields = hit.loadedFields();
+                fieldLookupProvider.setPreloadedStoredFieldValues(hit.hit().getId(), hit.loadedFields());
                 for (FetchSubPhaseProcessor processor : processors) {
                     processor.process(hit);
                 }

server/src/main/java/org/elasticsearch/search/fetch/PreloadedFieldLookupProvider.java

@@ -9,12 +9,16 @@
 package org.elasticsearch.search.fetch;
 
 import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.util.SetOnce;
+import org.elasticsearch.index.mapper.IdFieldMapper;
 import org.elasticsearch.search.lookup.FieldLookup;
 import org.elasticsearch.search.lookup.LeafFieldLookupProvider;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Supplier;
 
 /**
@@ -26,15 +30,22 @@
  */
 class PreloadedFieldLookupProvider implements LeafFieldLookupProvider {
 
-    Map<String, List<Object>> storedFields;
-    LeafFieldLookupProvider backUpLoader;
-    Supplier<LeafFieldLookupProvider> loaderSupplier;
+    private final SetOnce<Set<String>> preloadedStoredFieldNames = new SetOnce<>();
+    private Map<String, List<Object>> preloadedStoredFieldValues;
+    private String id;
+    private LeafFieldLookupProvider backUpLoader;
+    private Supplier<LeafFieldLookupProvider> loaderSupplier;
 
     @Override
     public void populateFieldLookup(FieldLookup fieldLookup, int doc) throws IOException {
         String field = fieldLookup.fieldType().name();
-        if (storedFields.containsKey(field)) {
-            fieldLookup.setValues(storedFields.get(field));
+
+        if (field.equals(IdFieldMapper.NAME)) {
+            fieldLookup.setValues(Collections.singletonList(id));
+            return;
+        }
+        if (preloadedStoredFieldNames.get().contains(field)) {
+            fieldLookup.setValues(preloadedStoredFieldValues.get(field));
             return;
         }
         // stored field not preloaded, go and get it directly
@@ -44,8 +55,26 @@ public void populateFieldLookup(FieldLookup fieldLookup, int doc) throws IOExcep
         backUpLoader.populateFieldLookup(fieldLookup, doc);
     }
 
+    void setPreloadedStoredFieldNames(Set<String> preloadedStoredFieldNames) {
+        this.preloadedStoredFieldNames.set(preloadedStoredFieldNames);
+    }
+
+    void setPreloadedStoredFieldValues(String id, Map<String, List<Object>> preloadedStoredFieldValues) {
+        assert preloadedStoredFieldNames.get().containsAll(preloadedStoredFieldValues.keySet())
+            : "Provided stored field that was not expected to be preloaded? "
+                + preloadedStoredFieldValues.keySet()
+                + " - "
+                + preloadedStoredFieldNames;
+        this.preloadedStoredFieldValues = preloadedStoredFieldValues;
+        this.id = id;
+    }
+
     void setNextReader(LeafReaderContext ctx) {
         backUpLoader = null;
         loaderSupplier = () -> LeafFieldLookupProvider.fromStoredFields().apply(ctx);
     }
+
+    LeafFieldLookupProvider getBackUpLoader() {
+        return backUpLoader;
+    }
 }

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java

@@ -40,6 +40,7 @@ public final class FetchFieldsPhase implements FetchSubPhase {
     private static final List<FieldAndFormat> DEFAULT_METADATA_FIELDS = List.of(
         new FieldAndFormat(IgnoredFieldMapper.NAME, null),
         new FieldAndFormat(RoutingFieldMapper.NAME, null),
+        // will only be fetched when mapped (older archived indices)
         new FieldAndFormat(LegacyTypeFieldMapper.NAME, null)
     );
 
@@ -95,9 +96,9 @@ public void setNextReader(LeafReaderContext readerContext) {
             @Override
             public StoredFieldsSpec storedFieldsSpec() {
                 if (fieldFetcher != null) {
-                    return fieldFetcher.storedFieldsSpec();
+                    return metadataFieldFetcher.storedFieldsSpec().merge(fieldFetcher.storedFieldsSpec());
                 }
-                return StoredFieldsSpec.NO_REQUIREMENTS;
+                return metadataFieldFetcher.storedFieldsSpec();
             }
 
             @Override

server/src/main/java/org/elasticsearch/search/lookup/FieldLookup.java

@@ -31,7 +31,9 @@ public MappedFieldType fieldType() {
      */
     public void setValues(List<Object> values) {
         assert valuesLoaded == false : "Call clear() before calling setValues()";
-        values.stream().map(fieldType::valueForDisplay).forEach(this.values::add);
+        if (values != null) {
+            values.stream().map(fieldType::valueForDisplay).forEach(this.values::add);
+        }
         this.valuesLoaded = true;
     }
 

server/src/test/java/org/elasticsearch/search/fetch/PreloadedFieldLookupProviderTests.java

@@ -13,11 +13,13 @@
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.index.memory.MemoryIndex;
 import org.apache.lucene.util.BytesRef;
+import org.elasticsearch.index.mapper.IdFieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.search.lookup.FieldLookup;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -30,7 +32,16 @@ public class PreloadedFieldLookupProviderTests extends ESTestCase {
 
     public void testFallback() throws IOException {
         PreloadedFieldLookupProvider lookup = new PreloadedFieldLookupProvider();
-        lookup.storedFields = Map.of("foo", List.of("bar"));
+        lookup.setPreloadedStoredFieldNames(Collections.singleton("foo"));
+        lookup.setPreloadedStoredFieldValues("id", Map.of("foo", List.of("bar")));
+
+        MappedFieldType idFieldType = mock(MappedFieldType.class);
+        when(idFieldType.name()).thenReturn(IdFieldMapper.NAME);
+        when(idFieldType.valueForDisplay(any())).then(invocation -> (invocation.getArguments()[0]));
+        FieldLookup idFieldLookup = new FieldLookup(idFieldType);
+        lookup.populateFieldLookup(idFieldLookup, 0);
+        assertEquals("id", idFieldLookup.getValue());
+        assertNull(lookup.getBackUpLoader());    // fallback didn't get used because 'foo' is in the list
 
         MappedFieldType fieldType = mock(MappedFieldType.class);
         when(fieldType.name()).thenReturn("foo");
@@ -39,7 +50,7 @@ public void testFallback() throws IOException {
 
         lookup.populateFieldLookup(fieldLookup, 0);
         assertEquals("BAR", fieldLookup.getValue());
-        assertNull(lookup.backUpLoader);    // fallback didn't get used because 'foo' is in the list
+        assertNull(lookup.getBackUpLoader());    // fallback didn't get used because 'foo' is in the list
 
         MappedFieldType unloadedFieldType = mock(MappedFieldType.class);
         when(unloadedFieldType.name()).thenReturn("unloaded");
@@ -56,5 +67,4 @@ public void testFallback() throws IOException {
         lookup.populateFieldLookup(unloadedFieldLookup, 0);
         assertEquals("VALUE", unloadedFieldLookup.getValue());
     }
-
 }

server/src/test/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhaseTests.java

@@ -19,13 +19,17 @@
 import org.elasticsearch.index.mapper.DocValueFetcher;
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.NestedLookup;
+import org.elasticsearch.index.mapper.StoredValueFetcher;
 import org.elasticsearch.index.query.SearchExecutionContext;
 import org.elasticsearch.search.DocValueFormat;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
 import org.elasticsearch.search.fetch.FetchContext;
 import org.elasticsearch.search.fetch.FetchSubPhase;
 import org.elasticsearch.search.fetch.FetchSubPhaseProcessor;
+import org.elasticsearch.search.fetch.StoredFieldsContext;
+import org.elasticsearch.search.fetch.StoredFieldsSpec;
+import org.elasticsearch.search.lookup.SearchLookup;
 import org.elasticsearch.search.lookup.Source;
 import org.elasticsearch.test.ESTestCase;
 
@@ -35,6 +39,7 @@
 import java.util.Set;
 
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -90,6 +95,58 @@ public void testDocValueFetcher() throws IOException {
 
         reader.close();
         dir.close();
+    }
+
+    public void testStoredFieldsSpec() {
+        StoredFieldsContext storedFieldsContext = StoredFieldsContext.fromList(List.of("stored", "_metadata"));
+        FetchFieldsContext ffc = new FetchFieldsContext(List.of(new FieldAndFormat("field", null)));
+
+        SearchLookup searchLookup = mock(SearchLookup.class);
+
+        SearchExecutionContext sec = mock(SearchExecutionContext.class);
+        when(sec.isMetadataField(any())).then(invocation -> invocation.getArguments()[0].toString().startsWith("_"));
+
+        MappedFieldType routingFt = mock(MappedFieldType.class);
+        when(routingFt.valueFetcher(any(), any())).thenReturn(new StoredValueFetcher(searchLookup, "_routing"));
+        when(sec.getFieldType(eq("_routing"))).thenReturn(routingFt);
+
+        // this would normally not be mapped -> getMatchingFieldsNames would not resolve it (unless for older archive indices)
+        MappedFieldType typeFt = mock(MappedFieldType.class);
+        when(typeFt.valueFetcher(any(), any())).thenReturn(new StoredValueFetcher(searchLookup, "_type"));
+        when(sec.getFieldType(eq("_type"))).thenReturn(typeFt);
+
+        MappedFieldType ignoredFt = mock(MappedFieldType.class);
+        when(ignoredFt.valueFetcher(any(), any())).thenReturn(new StoredValueFetcher(searchLookup, "_ignored"));
+        when(sec.getFieldType(eq("_ignored"))).thenReturn(ignoredFt);
+
+        // Ideally we would test that explicitly requested stored fields are included in stored fields spec, but isStored is final hence it
+        // can't be mocked. In reality, _metadata would be included but stored would not.
+        MappedFieldType storedFt = mock(MappedFieldType.class);
+        when(sec.getFieldType(eq("stored"))).thenReturn(storedFt);
+        MappedFieldType metadataFt = mock(MappedFieldType.class);
+        when(sec.getFieldType(eq("_metadata"))).thenReturn(metadataFt);
+
+        MappedFieldType fieldType = mock(MappedFieldType.class);
+        when(fieldType.valueFetcher(any(), any())).thenReturn(
+            new DocValueFetcher(
+                DocValueFormat.RAW,
+                new SortedNumericIndexFieldData("field", IndexNumericFieldData.NumericType.LONG, CoreValuesSourceType.NUMERIC, null, false)
+            )
+        );
+        when(sec.getFieldType(eq("field"))).thenReturn(fieldType);
 
+        when(sec.getMatchingFieldNames(any())).then(invocation -> Set.of(invocation.getArguments()[0]));
+        when(sec.nestedLookup()).thenReturn(NestedLookup.EMPTY);
+        FetchContext fetchContext = mock(FetchContext.class);
+        when(fetchContext.fetchFieldsContext()).thenReturn(ffc);
+        when(fetchContext.storedFieldsContext()).thenReturn(storedFieldsContext);
+        when(fetchContext.getSearchExecutionContext()).thenReturn(sec);
+        FetchFieldsPhase fetchFieldsPhase = new FetchFieldsPhase();
+        FetchSubPhaseProcessor processor = fetchFieldsPhase.getProcessor(fetchContext);
+        StoredFieldsSpec storedFieldsSpec = processor.storedFieldsSpec();
+        assertEquals(3, storedFieldsSpec.requiredStoredFields().size());
+        assertTrue(storedFieldsSpec.requiredStoredFields().contains("_routing"));
+        assertTrue(storedFieldsSpec.requiredStoredFields().contains("_ignored"));
+        assertTrue(storedFieldsSpec.requiredStoredFields().contains("_type"));
     }
 }