Handle ignored fields directly in SourceValueFetcher (#68738)

Currently, the value fetcher framework handles ignored fields by reading
the stored values of the _ignored metadata field, and passing these through
on calls to fetchValues(). However, this means that if a document has multiple
values indexed for a field, and one malformed value, then the fields API will
ignore everything, including the valid values, and return an empty list for this
document.

If a document source contains a malformed value, then it must have been
ignored at index time. Therefore, we can safely assume that if we get an
exception parsing values from source at fetch time, they were also ignored
at index time and they can be skipped. This commit moves this exception
handling directly into SourceValueFetcher and ArraySourceValueFetcher,
removing the need to inspect the _ignored metadata and fixing the case
of mixed valid and invalid values.

Author: romseygeek

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

@@ -83,7 +83,7 @@ static class TokenCountFieldType extends NumberFieldMapper.NumberFieldType {
         @Override
         public ValueFetcher valueFetcher(SearchExecutionContext context, String format) {
             if (hasDocValues() == false) {
-                return (lookup, ignoredFields) -> List.of();
+                return lookup -> List.of();
             }
             return new DocValueFetcher(docValueFormat(format, null), context.getForField(this));
         }

rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml

@@ -335,7 +335,7 @@ Test unmapped field:
               - f1
               - { "field" : "f4", "include_unmapped" : true }
   -  match:
-        hits.hits.0.fields.f1: 
+        hits.hits.0.fields.f1:
         - some text
   -  match:
         hits.hits.0.fields.f4:
@@ -347,7 +347,7 @@ Test unmapped field:
               fields:
               - { "field" : "f*", "include_unmapped" : true }
   -  match:
-        hits.hits.0.fields.f1: 
+        hits.hits.0.fields.f1:
         - some text
   -  match:
         hits.hits.0.fields.f2\.a:
@@ -381,7 +381,7 @@ Test unmapped fields inside disabled objects:
            id: 1
            refresh: true
            body:
-              f1: 
+              f1:
                  - some text
                  - a: b
                  -
@@ -809,9 +809,44 @@ Test nested field with sibling field resolving to DocValueFetcher:
         hits.hits.0.fields.owner:
           - "Anna Ott"
   - match:
-        hits.hits.0.fields.owner\.length: 
+        hits.hits.0.fields.owner\.length:
         - 2
   - match:
         hits.hits.0.fields.products.0: { "manufacturer" : ["Supersoft"]}
   - match:
         hits.hits.0.fields.products.1: { "manufacturer" : ["HyperSmart"]}
+
+---
+"Test ignores malformed values while returning valid ones":
+  - skip:
+      version: ' - 8.0.0'
+      reason:  'Added in 8.0 - change on backport'
+  - do:
+      indices.create:
+        index: test
+        body:
+          mappings:
+            properties:
+              number:
+                type: long
+                ignore_malformed: true
+  - do:
+      index:
+        index: test
+        id: 1
+        refresh: true
+        body:
+          number: [ 1, 2, "3", "four", 5, 6 ]
+
+  - do:
+      search:
+        index: test
+        body:
+          fields: [ "*" ]
+
+  - length: { hits.hits.0.fields.number : 5 }
+  - match: { hits.hits.0.fields.number.0 : 1 }
+  - match: { hits.hits.0.fields.number.1 : 2 }
+  - match: { hits.hits.0.fields.number.2 : 3 }
+  - match: { hits.hits.0.fields.number.3 : 5 }
+  - match: { hits.hits.0.fields.number.4 : 6 }

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

@@ -27,7 +27,6 @@
 public abstract class ArraySourceValueFetcher implements ValueFetcher {
     private final Set<String> sourcePaths;
     private final @Nullable Object nullValue;
-    private final String fieldName;
 
     public ArraySourceValueFetcher(String fieldName, SearchExecutionContext context) {
         this(fieldName, context, null);
@@ -41,21 +40,23 @@ public ArraySourceValueFetcher(String fieldName, SearchExecutionContext context)
     public ArraySourceValueFetcher(String fieldName, SearchExecutionContext context, Object nullValue) {
         this.sourcePaths = context.sourcePath(fieldName);
         this.nullValue = nullValue;
-        this.fieldName = fieldName;
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields) {
+    public List<Object> fetchValues(SourceLookup lookup) {
         List<Object> values = new ArrayList<>();
-        if (ignoredFields.contains(fieldName)) {
-            return values;
-        }
         for (String path : sourcePaths) {
             Object sourceValue = lookup.extractValue(path, nullValue);
             if (sourceValue == null) {
                 return List.of();
             }
-            values.addAll((List<?>) parseSourceValue(sourceValue));
+            try {
+                values.addAll((List<?>) parseSourceValue(sourceValue));
+            } catch (Exception e) {
+                // if parsing fails here then it would have failed at index time
+                // as well, meaning that we must be ignoring malformed values.
+                // So ignore it here too.
+            }
         }
         return values;
     }

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

@@ -17,7 +17,6 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Set;
 
 import static java.util.Collections.emptyList;
 
@@ -40,7 +39,7 @@ public void setNextReader(LeafReaderContext context) {
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields) throws IOException {
+    public List<Object> fetchValues(SourceLookup lookup) throws IOException {
         if (false == formattedDocValues.advanceExact(lookup.docId())) {
             return emptyList();
         }

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

@@ -19,7 +19,6 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 public class NestedValueFetcher implements ValueFetcher {
 
@@ -39,7 +38,7 @@ public NestedValueFetcher(String nestedField, FieldFetcher nestedFieldFetcher) {
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoreFields) throws IOException {
+    public List<Object> fetchValues(SourceLookup lookup) throws IOException {
         List<Object> nestedEntriesToReturn = new ArrayList<>();
         Map<String, Object> filteredSource = new HashMap<>();
         Map<String, Object> stub = createSourceMapStub(filteredSource);
@@ -53,7 +52,7 @@ public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoreFields) t
             SourceLookup nestedSourceLookup = new SourceLookup();
             nestedSourceLookup.setSource(filteredSource);
 
-            Map<String, DocumentField> fetchResult = nestedFieldFetcher.fetch(nestedSourceLookup, ignoreFields);
+            Map<String, DocumentField> fetchResult = nestedFieldFetcher.fetch(nestedSourceLookup);
 
             Map<String, Object> nestedEntry = new HashMap<>();
             for (DocumentField field : fetchResult.values()) {

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

@@ -28,29 +28,23 @@
 public abstract class SourceValueFetcher implements ValueFetcher {
     private final Set<String> sourcePaths;
     private final @Nullable Object nullValue;
-    private final String fieldName;
 
     public SourceValueFetcher(String fieldName, SearchExecutionContext context) {
         this(fieldName, context, null);
     }
 
     /**
-     * @param fieldName The name of the field.
      * @param context The query shard context
      * @param nullValue A optional substitute value if the _source value is 'null'.
      */
     public SourceValueFetcher(String fieldName, SearchExecutionContext context, Object nullValue) {
         this.sourcePaths = context.sourcePath(fieldName);
         this.nullValue = nullValue;
-        this.fieldName = fieldName;
     }
 
     @Override
-    public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields) {
+    public List<Object> fetchValues(SourceLookup lookup) {
         List<Object> values = new ArrayList<>();
-        if (ignoredFields.contains(fieldName)) {
-            return values;
-        }
         for (String path : sourcePaths) {
             Object sourceValue = lookup.extractValue(path, nullValue);
             if (sourceValue == null) {
@@ -66,9 +60,16 @@ public List<Object> fetchValues(SourceLookup lookup, Set<String> ignoredFields)
                 if (value instanceof List) {
                     queue.addAll((List<?>) value);
                 } else {
-                    Object parsedValue = parseSourceValue(value);
-                    if (parsedValue != null) {
-                        values.add(parsedValue);
+                    try {
+                        Object parsedValue = parseSourceValue(value);
+                        if (parsedValue != null) {
+                            values.add(parsedValue);
+                        }
+                    } catch (Exception e) {
+                        // if we get a parsing exception here, that means that the
+                        // value in _source would have also caused a parsing
+                        // exception at index time and the value ignored.
+                        // so ignore it here as well
                     }
                 }
             }

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

@@ -9,13 +9,11 @@
 package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.index.LeafReaderContext;
-import org.elasticsearch.common.Nullable;
 import org.elasticsearch.search.fetch.subphase.FetchFieldsPhase;
 import org.elasticsearch.search.lookup.SourceLookup;
 
 import java.io.IOException;
 import java.util.List;
-import java.util.Set;
 
 /**
  * A helper class for fetching field values during the {@link FetchFieldsPhase}. Each {@link MappedFieldType}
@@ -33,10 +31,9 @@ public interface ValueFetcher {
     * should not be relied on.
     *
     * @param lookup a lookup structure over the document's source.
-    * @param ignoredFields the fields in _ignored that have been ignored for this document because they were malformed
     * @return a list a standardized field values.
     */
-    List<Object> fetchValues(SourceLookup lookup, @Nullable Set<String> ignoredFields) throws IOException;
+    List<Object> fetchValues(SourceLookup lookup) throws IOException;
 
     /**
      * Update the leaf reader used to fetch values.

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

@@ -18,7 +18,6 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 
 /**
@@ -71,7 +70,7 @@ public void process(HitContext hit) throws IOException {
                         // docValues fields will still be document fields, and put under "fields" section of a hit.
                         hit.hit().setDocumentField(f.field, hitField);
                     }
-                    hitField.getValues().addAll(f.fetcher.fetchValues(hit.sourceLookup(), Collections.emptySet()));
+                    hitField.getValues().addAll(f.fetcher.fetchValues(hit.sourceLookup()));
                 }
             }
         };

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

@@ -10,17 +10,14 @@
 
 import org.apache.lucene.index.LeafReaderContext;
 import org.elasticsearch.common.document.DocumentField;
-import org.elasticsearch.index.mapper.IgnoredFieldMapper;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.fetch.FetchContext;
 import org.elasticsearch.search.fetch.FetchSubPhase;
 import org.elasticsearch.search.fetch.FetchSubPhaseProcessor;
 import org.elasticsearch.search.lookup.SourceLookup;
 
 import java.io.IOException;
-import java.util.HashSet;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * A fetch sub-phase for high-level field retrieval. Given a list of fields, it
@@ -53,25 +50,11 @@ public void process(HitContext hitContext) throws IOException {
                 SearchHit hit = hitContext.hit();
                 SourceLookup sourceLookup = hitContext.sourceLookup();
 
-                Set<String> ignoredFields = getIgnoredFields(hit);
-                Map<String, DocumentField> documentFields = fieldFetcher.fetch(sourceLookup, ignoredFields);
+                Map<String, DocumentField> documentFields = fieldFetcher.fetch(sourceLookup);
                 for (Map.Entry<String, DocumentField> entry : documentFields.entrySet()) {
                     hit.setDocumentField(entry.getKey(), entry.getValue());
                 }
             }
         };
     }
-
-    private Set<String> getIgnoredFields(SearchHit hit) {
-        DocumentField field = hit.field(IgnoredFieldMapper.NAME);
-        if (field == null) {
-            return Set.of();
-        }
-
-        Set<String> ignoredFields = new HashSet<>();
-        for (Object value : field.getValues()) {
-            ignoredFields.add((String) value);
-        }
-        return ignoredFields;
-    }
 }

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

@@ -134,14 +134,13 @@ private FieldFetcher(
         this.unmappedFieldsFetchAutomaton = unmappedFieldsFetchAutomaton;
     }
 
-    public Map<String, DocumentField> fetch(SourceLookup sourceLookup, Set<String> ignoredFields) throws IOException {
-        assert ignoredFields != null;
+    public Map<String, DocumentField> fetch(SourceLookup sourceLookup) throws IOException {
         Map<String, DocumentField> documentFields = new HashMap<>();
         for (FieldContext context : fieldContexts.values()) {
             String field = context.fieldName;
 
             ValueFetcher valueFetcher = context.valueFetcher;
-            List<Object> parsedValues = valueFetcher.fetchValues(sourceLookup, ignoredFields);
+            List<Object> parsedValues = valueFetcher.fetchValues(sourceLookup);
             if (parsedValues.isEmpty() == false) {
                 documentFields.put(field, new DocumentField(field, parsedValues));
             }

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java

@@ -48,7 +48,7 @@ public static List<Object> loadFieldValues(MappedFieldType fieldType,
         }
         ValueFetcher fetcher = fieldType.valueFetcher(searchContext, null);
         fetcher.setNextReader(hitContext.readerContext());
-        return fetcher.fetchValues(hitContext.sourceLookup(), Collections.emptySet());
+        return fetcher.fetchValues(hitContext.sourceLookup());
     }
 
     public static class Encoders {

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

@@ -41,5 +41,9 @@ public void testFetchSourceValue() throws IOException {
         sourceValue = "POINT (42.0 27.1)";
         assertEquals(List.of(jsonPoint), fetchSourceValue(mapper, sourceValue, null));
         assertEquals(List.of(wktPoint), fetchSourceValue(mapper, sourceValue, "wkt"));
+
+        // Test a malformed value
+        sourceValue = "malformed";
+        assertEquals(List.of(), fetchSourceValue(mapper, sourceValue, null));
     }
 }

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

@@ -639,6 +639,7 @@ public void testFetchSourceValue() throws IOException {
             .fieldType();
         assertEquals(List.of(3), fetchSourceValue(mapper, 3.14));
         assertEquals(List.of(42), fetchSourceValue(mapper, "42.9"));
+        assertEquals(List.of(3, 42), fetchSourceValues(mapper, 3.14, "foo", "42.9"));
 
         MappedFieldType nullValueMapper = new NumberFieldMapper.Builder("field", NumberType.FLOAT, false, true)
             .nullValue(2.71f)