Search: Simply SingleFieldsVisitor (#34052)

`SingleFieldsVisitor` is meant to load a single stored field but it
manages to be quite complex to reason about because it inherits from our
"basic" `FieldsVisitor` which is designed to load many fields. This
breaks that inheritance and adds logic to `SingleFieldsVisitor` so it can
be properly stand alone. While this amounts to more lines of code they
ought to be significantly easier to reason about.

server/src/main/java/org/elasticsearch/index/fieldvisitor/SingleFieldsVisitor.java

@@ -19,42 +19,79 @@
 package org.elasticsearch.index.fieldvisitor;
 
 import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.StoredFieldVisitor;
 import org.elasticsearch.index.mapper.IdFieldMapper;
-import org.elasticsearch.index.mapper.MapperService;
-import org.elasticsearch.index.mapper.TypeFieldMapper;
+import org.elasticsearch.index.mapper.MappedFieldType;
+import org.elasticsearch.index.mapper.Uid;
+import org.apache.lucene.util.BytesRef;
 
-import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
 
-public class SingleFieldsVisitor extends FieldsVisitor {
-
-    private String field;
+/**
+ * {@linkplain StoredFieldVisitor} that loads a single field value.
+ */
+public final class SingleFieldsVisitor extends StoredFieldVisitor {
+    private final MappedFieldType field;
+    private final List<Object> destination;
 
-    public SingleFieldsVisitor(String field) {
-        super(false);
+    /**
+     * Build the field visitor;
+     * @param field the name of the field to load
+     * @param destination where to put the field's values
+     */
+    public SingleFieldsVisitor(MappedFieldType field, List<Object> destination) {
         this.field = field;
+        this.destination = destination;
     }
 
     @Override
-    public Status needsField(FieldInfo fieldInfo) throws IOException {
-        if (fieldInfo.name.equals(field)) {
+    public Status needsField(FieldInfo fieldInfo) {
+        if (fieldInfo.name.equals(field.name())) {
             return Status.YES;
         }
+        /*
+         * We can't return Status.STOP here because we could be loading
+         * multi-valued fields.
+         */
         return Status.NO;
     }
 
-    public void reset(String field) {
-        this.field = field;
-        super.reset();
+    private void addValue(Object value) {
+        destination.add(field.valueForDisplay(value));
     }
 
     @Override
-    public void postProcess(MapperService mapperService) {
-        super.postProcess(mapperService);
-        if (id != null) {
-            addValue(IdFieldMapper.NAME, id);
-        }
-        if (type != null) {
-            addValue(TypeFieldMapper.NAME, type);
+    public void binaryField(FieldInfo fieldInfo, byte[] value) {
+        if (IdFieldMapper.NAME.equals(fieldInfo.name)) {
+            addValue(Uid.decodeId(value));
+        } else {
+            addValue(new BytesRef(value));
         }
     }
+
+    @Override
+    public void stringField(FieldInfo fieldInfo, byte[] bytes) {
+        addValue(new String(bytes, StandardCharsets.UTF_8));
+    }
+
+    @Override
+    public void intField(FieldInfo fieldInfo, int value) {
+        addValue(value);
+    }
+
+    @Override
+    public void longField(FieldInfo fieldInfo, long value) {
+        addValue(value);
+    }
+
+    @Override
+    public void floatField(FieldInfo fieldInfo, float value) {
+        addValue(value);
+    }
+
+    @Override
+    public void doubleField(FieldInfo fieldInfo, double value) {
+        addValue(value);
+    }
 }

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

@@ -22,10 +22,13 @@ import org.apache.lucene.index.LeafReader;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.index.fieldvisitor.SingleFieldsVisitor;
+import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.MapperService;
+import org.elasticsearch.index.mapper.TypeFieldMapper;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
@@ -48,13 +51,10 @@ public class LeafFieldsLookup implements Map {
 
     private final Map<String, FieldLookup> cachedFieldData = new HashMap<>();
 
-    private final SingleFieldsVisitor fieldVisitor;
-
     LeafFieldsLookup(MapperService mapperService, @Nullable String[] types, LeafReader reader) {
         this.mapperService = mapperService;
         this.types = types;
         this.reader = reader;
-        this.fieldVisitor = new SingleFieldsVisitor(null);
     }
 
     public void setDocument(int docId) {
@@ -142,16 +142,23 @@ public class LeafFieldsLookup implements Map {
             cachedFieldData.put(name, data);
         }
         if (data.fields() == null) {
-            String fieldName = data.fieldType().name();
-            fieldVisitor.reset(fieldName);
-            try {
-                reader.document(docId, fieldVisitor);
-                fieldVisitor.postProcess(mapperService);
-                List<Object> storedFields = fieldVisitor.fields().get(data.fieldType().name());
-                data.fields(singletonMap(fieldName, storedFields));
-            } catch (IOException e) {
-                throw new ElasticsearchParseException("failed to load field [{}]", e, name);
+            List<Object> values;
+            if (TypeFieldMapper.NAME.equals(data.fieldType().name())) {
+                values = new ArrayList<>(1);
+                final DocumentMapper mapper = mapperService.documentMapper();
+                if (mapper != null) {
+                    values.add(mapper.type());
+                }
+            } else {
+                values = new ArrayList<Object>(2);
+                SingleFieldsVisitor visitor = new SingleFieldsVisitor(data.fieldType(), values);
+                try {
+                    reader.document(docId, visitor);
+                } catch (IOException e) {
+                    throw new ElasticsearchParseException("failed to load field [{}]", e, name);
+                }
             }
+            data.fields(singletonMap(data.fieldType().name(), values));
         }
         return data;
     }

server/src/test/java/org/elasticsearch/index/shard/RefreshListenersTests.java

@@ -44,7 +44,6 @@ import org.elasticsearch.index.engine.Engine;
 import org.elasticsearch.index.engine.EngineConfig;
 import org.elasticsearch.index.engine.EngineTestCase;
 import org.elasticsearch.index.engine.InternalEngine;
-import org.elasticsearch.index.fieldvisitor.SingleFieldsVisitor;
 import org.elasticsearch.index.mapper.IdFieldMapper;
 import org.elasticsearch.index.mapper.ParseContext.Document;
 import org.elasticsearch.index.mapper.ParsedDocument;
@@ -324,9 +323,9 @@ public class RefreshListenersTests extends ESTestCase {
                         try (Engine.GetResult getResult = engine.get(get, engine::acquireSearcher)) {
                             assertTrue("document not found", getResult.exists());
                             assertEquals(iteration, getResult.version());
-                            SingleFieldsVisitor visitor = new SingleFieldsVisitor("test");
-                            getResult.docIdAndVersion().reader.document(getResult.docIdAndVersion().docId, visitor);
-                            assertEquals(Arrays.asList(testFieldValue), visitor.fields().get("test"));
+                            org.apache.lucene.document.Document document =
+                                    getResult.docIdAndVersion().reader.document(getResult.docIdAndVersion().docId);
+                            assertEquals(new String[] {testFieldValue}, document.getValues("test"));
                         }
                     } catch (Exception t) {
                         throw new RuntimeException("failure on the [" + iteration + "] iteration of thread [" + threadId + "]", t);

server/src/test/java/org/elasticsearch/search/lookup/LeafFieldsLookupTests.java

@@ -31,7 +31,6 @@ import org.junit.Before;
 import java.util.Collections;
 import java.util.List;
 
-import static org.mockito.AdditionalAnswers.returnsFirstArg;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyObject;
@@ -48,7 +47,9 @@ public class LeafFieldsLookupTests extends ESTestCase {
 
         MappedFieldType fieldType = mock(MappedFieldType.class);
         when(fieldType.name()).thenReturn("field");
-        when(fieldType.valueForDisplay(anyObject())).then(returnsFirstArg());
+        // Add 10 when valueForDisplay is called so it is easy to be sure it *was* called
+        when(fieldType.valueForDisplay(anyObject())).then(invocation ->
+                (Double) invocation.getArguments()[0] + 10);
 
         MapperService mapperService = mock(MapperService.class);
         when(mapperService.fullName("field")).thenReturn(fieldType);
@@ -77,7 +78,7 @@ public class LeafFieldsLookupTests extends ESTestCase {
         List<Object> values = fieldLookup.getValues();
         assertNotNull(values);
         assertEquals(1, values.size());
-        assertEquals(2.718, values.get(0));
+        assertEquals(12.718, values.get(0));
     }
 
     public void testLookupWithFieldAlias() {
@@ -87,6 +88,6 @@ public class LeafFieldsLookupTests extends ESTestCase {
         List<Object> values = fieldLookup.getValues();
         assertNotNull(values);
         assertEquals(1, values.size());
-        assertEquals(2.718, values.get(0));
+        assertEquals(12.718, values.get(0));
     }
 }