Revert "Revert "Save one utf8 conversion in KeywordFieldMapper. #19867""

This reverts commit d805266d94.

core/src/main/java/org/elasticsearch/index/get/ShardGetService.java

@@ -239,10 +239,7 @@ public final class ShardGetService extends AbstractIndexShardComponent {
                     }
 
                     List<Object> values = searchLookup.source().extractRawValues(field);
-                    if (!values.isEmpty()) {
+                    if (values.isEmpty() == false) {
-                        for (int i = 0; i < values.size(); i++) {
-                            values.set(i, fieldMapper.fieldType().valueForSearch(values.get(i)));
-                        }
                         value = values;
                     }
                 }

core/src/main/java/org/elasticsearch/index/mapper/BaseGeoPointFieldMapper.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.document.Field;
+import org.apache.lucene.index.IndexOptions;
 import org.apache.lucene.search.Query;
 import org.elasticsearch.common.geo.GeoHashUtils;
 import org.apache.lucene.util.LegacyNumericUtils;
@@ -139,7 +140,7 @@ public abstract class BaseGeoPointFieldMapper extends FieldMapper implements Arr
 
         public abstract Y build(BuilderContext context, String simpleName, MappedFieldType fieldType, MappedFieldType defaultFieldType,
                                 Settings indexSettings, FieldMapper latMapper, FieldMapper lonMapper,
-                                KeywordFieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed, CopyTo copyTo);
+                                FieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed, CopyTo copyTo);
 
         public Y build(Mapper.BuilderContext context) {
             GeoPointFieldType geoPointFieldType = (GeoPointFieldType)fieldType;
@@ -166,10 +167,17 @@ public abstract class BaseGeoPointFieldMapper extends FieldMapper implements Arr
                 }
                 geoPointFieldType.setLatLonEnabled(latMapper.fieldType(), lonMapper.fieldType());
             }
-            KeywordFieldMapper geoHashMapper = null;
+            FieldMapper geoHashMapper = null;
             if (enableGeoHash || enableGeoHashPrefix) {
                 // TODO: possible also implicitly enable geohash if geohash precision is set
-                geoHashMapper = new KeywordFieldMapper.Builder(Names.GEOHASH).index(true).includeInAll(false).store(fieldType.stored()).build(context);
+                if (context.indexCreatedVersion().onOrAfter(Version.V_5_0_0_alpha1)) {
+                    geoHashMapper = new KeywordFieldMapper.Builder(Names.GEOHASH)
+                            .index(true).includeInAll(false).store(fieldType.stored()).build(context);
+                } else {
+                    geoHashMapper = new StringFieldMapper.Builder(Names.GEOHASH)
+                            .tokenized(false).index(true).omitNorms(true).indexOptions(IndexOptions.DOCS)
+                            .includeInAll(false).store(fieldType.stored()).build(context);
+                }
                 geoPointFieldType.setGeoHashEnabled(geoHashMapper.fieldType(), geoHashPrecision, enableGeoHashPrefix);
             }
             context.path().remove();
@@ -376,12 +384,12 @@ public abstract class BaseGeoPointFieldMapper extends FieldMapper implements Arr
 
     protected FieldMapper lonMapper;
 
-    protected KeywordFieldMapper geoHashMapper;
+    protected FieldMapper geoHashMapper;
 
     protected Explicit<Boolean> ignoreMalformed;
 
     protected BaseGeoPointFieldMapper(String simpleName, MappedFieldType fieldType, MappedFieldType defaultFieldType, Settings indexSettings,
-                                      FieldMapper latMapper, FieldMapper lonMapper, KeywordFieldMapper geoHashMapper,
+                                      FieldMapper latMapper, FieldMapper lonMapper, FieldMapper geoHashMapper,
                                       MultiFields multiFields, Explicit<Boolean> ignoreMalformed, CopyTo copyTo) {
         super(simpleName, fieldType, defaultFieldType, indexSettings, multiFields, copyTo);
         this.latMapper = latMapper;
@@ -552,7 +560,7 @@ public abstract class BaseGeoPointFieldMapper extends FieldMapper implements Arr
     @Override
     public FieldMapper updateFieldType(Map<String, MappedFieldType> fullNameToFieldType) {
         BaseGeoPointFieldMapper updated = (BaseGeoPointFieldMapper) super.updateFieldType(fullNameToFieldType);
-        KeywordFieldMapper geoUpdated = geoHashMapper == null ? null : (KeywordFieldMapper) geoHashMapper.updateFieldType(fullNameToFieldType);
+        FieldMapper geoUpdated = geoHashMapper == null ? null : geoHashMapper.updateFieldType(fullNameToFieldType);
         FieldMapper latUpdated = latMapper == null ? null : latMapper.updateFieldType(fullNameToFieldType);
         FieldMapper lonUpdated = lonMapper == null ? null : lonMapper.updateFieldType(fullNameToFieldType);
         if (updated == this

core/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java

@@ -73,7 +73,7 @@ public class GeoPointFieldMapper extends BaseGeoPointFieldMapper  {
         @Override
         public GeoPointFieldMapper build(BuilderContext context, String simpleName, MappedFieldType fieldType,
                                          MappedFieldType defaultFieldType, Settings indexSettings, FieldMapper latMapper,
-                                         FieldMapper lonMapper, KeywordFieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed,
+                                         FieldMapper lonMapper, FieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed,
                                          CopyTo copyTo) {
             fieldType.setTokenized(false);
             if (context.indexCreatedVersion().before(Version.V_2_3_0)) {
@@ -104,7 +104,7 @@ public class GeoPointFieldMapper extends BaseGeoPointFieldMapper  {
 
     public GeoPointFieldMapper(String simpleName, MappedFieldType fieldType, MappedFieldType defaultFieldType, Settings indexSettings,
                                FieldMapper latMapper, FieldMapper lonMapper,
-                               KeywordFieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed, CopyTo copyTo) {
+                               FieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed, CopyTo copyTo) {
         super(simpleName, fieldType, defaultFieldType, indexSettings, latMapper, lonMapper, geoHashMapper, multiFields,
                 ignoreMalformed, copyTo);
     }

core/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java

@@ -160,6 +160,16 @@ public final class KeywordFieldMapper extends FieldMapper {
             failIfNoDocValues();
             return new DocValuesIndexFieldData.Builder();
         }
+
+        @Override
+        public Object valueForSearch(Object value) {
+            if (value == null) {
+                return null;
+            }
+            // keywords are internally stored as utf8 bytes
+            BytesRef binaryValue = (BytesRef) value;
+            return binaryValue.utf8ToString();
+        }
     }
 
     private Boolean includeInAll;
@@ -212,12 +222,14 @@ public final class KeywordFieldMapper extends FieldMapper {
             context.allEntries().addText(fieldType().name(), value, fieldType().boost());
         }
 
+        // convert to utf8 only once before feeding postings/dv/stored fields
+        final BytesRef binaryValue = new BytesRef(value);
         if (fieldType().indexOptions() != IndexOptions.NONE || fieldType().stored()) {
-            Field field = new Field(fieldType().name(), value, fieldType());
+            Field field = new Field(fieldType().name(), binaryValue, fieldType());
             fields.add(field);
         }
         if (fieldType().hasDocValues()) {
-            fields.add(new SortedSetDocValuesField(fieldType().name(), new BytesRef(value)));
+            fields.add(new SortedSetDocValuesField(fieldType().name(), binaryValue));
         }
     }
 

core/src/main/java/org/elasticsearch/index/mapper/LegacyGeoPointFieldMapper.java

@@ -58,7 +58,7 @@ public class LegacyGeoPointFieldMapper extends BaseGeoPointFieldMapper implement
     }
 
     public static class Defaults extends BaseGeoPointFieldMapper.Defaults{
-        public static final Explicit<Boolean> COERCE = new Explicit(false, false);
+        public static final Explicit<Boolean> COERCE = new Explicit<>(false, false);
 
         public static final GeoPointFieldType FIELD_TYPE = new GeoPointFieldType();
 
@@ -100,7 +100,7 @@ public class LegacyGeoPointFieldMapper extends BaseGeoPointFieldMapper implement
         @Override
         public LegacyGeoPointFieldMapper build(BuilderContext context, String simpleName, MappedFieldType fieldType,
                 MappedFieldType defaultFieldType, Settings indexSettings, FieldMapper latMapper, FieldMapper lonMapper,
-                KeywordFieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed,
+                FieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed,
                 CopyTo copyTo) {
             fieldType.setTokenized(false);
             setupFieldType(context);
@@ -261,7 +261,7 @@ public class LegacyGeoPointFieldMapper extends BaseGeoPointFieldMapper implement
     protected Explicit<Boolean> coerce;
 
     public LegacyGeoPointFieldMapper(String simpleName, MappedFieldType fieldType, MappedFieldType defaultFieldType,
-            Settings indexSettings, FieldMapper latMapper, FieldMapper lonMapper, KeywordFieldMapper geoHashMapper,
+            Settings indexSettings, FieldMapper latMapper, FieldMapper lonMapper, FieldMapper geoHashMapper,
             MultiFields multiFields, Explicit<Boolean> ignoreMalformed, Explicit<Boolean> coerce, CopyTo copyTo) {
         super(simpleName, fieldType, defaultFieldType, indexSettings, latMapper, lonMapper, geoHashMapper, multiFields,
                 ignoreMalformed, copyTo);

core/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java

@@ -21,6 +21,7 @@ package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.index.IndexOptions;
 import org.apache.lucene.index.IndexableField;
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
@@ -80,7 +81,7 @@ public class DynamicTemplatesTests extends ESSingleNodeTestCase {
 
         IndexableField f = doc.getField("name");
         assertThat(f.name(), equalTo("name"));
-        assertThat(f.stringValue(), equalTo("some name"));
+        assertThat(f.binaryValue(), equalTo(new BytesRef("some name")));
         assertNotSame(IndexOptions.NONE, f.fieldType().indexOptions());
         assertThat(f.fieldType().tokenized(), equalTo(false));
 
@@ -98,7 +99,7 @@ public class DynamicTemplatesTests extends ESSingleNodeTestCase {
 
         f = doc.getField("multi1.org");
         assertThat(f.name(), equalTo("multi1.org"));
-        assertThat(f.stringValue(), equalTo("multi 1"));
+        assertThat(f.binaryValue(), equalTo(new BytesRef("multi 1")));
         assertNotSame(IndexOptions.NONE, f.fieldType().indexOptions());
         assertThat(f.fieldType().tokenized(), equalTo(false));
 
@@ -116,7 +117,7 @@ public class DynamicTemplatesTests extends ESSingleNodeTestCase {
 
         f = doc.getField("multi2.org");
         assertThat(f.name(), equalTo("multi2.org"));
-        assertThat(f.stringValue(), equalTo("multi 2"));
+        assertThat(f.binaryValue(), equalTo(new BytesRef("multi 2")));
         assertNotSame(IndexOptions.NONE, f.fieldType().indexOptions());
         assertThat(f.fieldType().tokenized(), equalTo(false));
 
@@ -137,7 +138,7 @@ public class DynamicTemplatesTests extends ESSingleNodeTestCase {
 
         IndexableField f = doc.getField("name");
         assertThat(f.name(), equalTo("name"));
-        assertThat(f.stringValue(), equalTo("some name"));
+        assertThat(f.binaryValue(), equalTo(new BytesRef("some name")));
         assertNotSame(IndexOptions.NONE, f.fieldType().indexOptions());
         assertThat(f.fieldType().tokenized(), equalTo(false));
 
@@ -155,7 +156,7 @@ public class DynamicTemplatesTests extends ESSingleNodeTestCase {
 
         f = doc.getField("multi1.org");
         assertThat(f.name(), equalTo("multi1.org"));
-        assertThat(f.stringValue(), equalTo("multi 1"));
+        assertThat(f.binaryValue(), equalTo(new BytesRef("multi 1")));
         assertNotSame(IndexOptions.NONE, f.fieldType().indexOptions());
         assertThat(f.fieldType().tokenized(), equalTo(false));
 
@@ -173,7 +174,7 @@ public class DynamicTemplatesTests extends ESSingleNodeTestCase {
 
         f = doc.getField("multi2.org");
         assertThat(f.name(), equalTo("multi2.org"));
-        assertThat(f.stringValue(), equalTo("multi 2"));
+        assertThat(f.binaryValue(), equalTo(new BytesRef("multi 2")));
         assertNotSame(IndexOptions.NONE, f.fieldType().indexOptions());
         assertThat(f.fieldType().tokenized(), equalTo(false));
 

core/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.spatial.geopoint.document.GeoPointField;
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.compress.CompressedXContent;
@@ -155,7 +156,7 @@ public class ExternalFieldMapperTests extends ESSingleNodeTestCase {
         assertThat(doc.rootDoc().getField("field.field").stringValue(), is("foo"));
 
         assertThat(doc.rootDoc().getField("field.field.raw"), notNullValue());
-        assertThat(doc.rootDoc().getField("field.field.raw").stringValue(), is("foo"));
+        assertThat(doc.rootDoc().getField("field.field.raw").binaryValue(), is(new BytesRef("foo")));
     }
 
     public void testExternalValuesWithMultifieldTwoLevels() throws Exception {

core/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java

@@ -20,6 +20,7 @@ package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.index.IndexableField;
 import org.apache.lucene.spatial.geopoint.document.GeoPointField;
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder;
 import org.elasticsearch.action.search.SearchResponse;
@@ -112,7 +113,11 @@ public class GeoPointFieldMapperTests extends ESSingleNodeTestCase {
 
         assertThat(doc.rootDoc().getField("point.lat"), notNullValue());
         assertThat(doc.rootDoc().getField("point.lon"), notNullValue());
-        assertThat(doc.rootDoc().get("point.geohash"), equalTo(stringEncode(1.3, 1.2)));
+        if (version.onOrAfter(Version.V_5_0_0_alpha1)) {
+            assertThat(doc.rootDoc().getBinaryValue("point.geohash"), equalTo(new BytesRef(stringEncode(1.3, 1.2))));
+        } else {
+            assertThat(doc.rootDoc().get("point.geohash"), equalTo(stringEncode(1.3, 1.2)));
+        }
     }
 
     public void testLatLonInOneValueWithGeohash() throws Exception {
@@ -132,7 +137,11 @@ public class GeoPointFieldMapperTests extends ESSingleNodeTestCase {
 
         assertThat(doc.rootDoc().getField("point.lat"), notNullValue());
         assertThat(doc.rootDoc().getField("point.lon"), notNullValue());
-        assertThat(doc.rootDoc().get("point.geohash"), equalTo(stringEncode(1.3, 1.2)));
+        if (version.onOrAfter(Version.V_5_0_0_alpha1)) {
+            assertThat(doc.rootDoc().getBinaryValue("point.geohash"), equalTo(new BytesRef(stringEncode(1.3, 1.2))));
+        } else {
+            assertThat(doc.rootDoc().get("point.geohash"), equalTo(stringEncode(1.3, 1.2)));
+        }
     }
 
     public void testGeoHashIndexValue() throws Exception {
@@ -152,7 +161,11 @@ public class GeoPointFieldMapperTests extends ESSingleNodeTestCase {
 
         assertThat(doc.rootDoc().getField("point.lat"), notNullValue());
         assertThat(doc.rootDoc().getField("point.lon"), notNullValue());
-        assertThat(doc.rootDoc().get("point.geohash"), equalTo(stringEncode(1.3, 1.2)));
+        if (version.onOrAfter(Version.V_5_0_0_alpha1)) {
+            assertThat(doc.rootDoc().getBinaryValue("point.geohash"), equalTo(new BytesRef(stringEncode(1.3, 1.2))));
+        } else {
+            assertThat(doc.rootDoc().get("point.geohash"), equalTo(stringEncode(1.3, 1.2)));
+        }
     }
 
     public void testGeoHashValue() throws Exception {

core/src/test/java/org/elasticsearch/index/mapper/GeohashMappingGeoPointTests.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.spatial.geopoint.document.GeoPointField;
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.compress.CompressedXContent;
@@ -118,7 +119,7 @@ public class GeohashMappingGeoPointTests extends ESSingleNodeTestCase {
 
         assertThat(doc.rootDoc().getField("point.lat"), nullValue());
         assertThat(doc.rootDoc().getField("point.lon"), nullValue());
-        assertThat(doc.rootDoc().get("point.geohash"), equalTo(stringEncode(1.3, 1.2)));
+        assertThat(doc.rootDoc().getBinaryValue("point.geohash"), equalTo(new BytesRef(stringEncode(1.3, 1.2))));
         assertThat(doc.rootDoc().get("point"), notNullValue());
     }
 

core/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

@@ -78,7 +78,7 @@ public class KeywordFieldMapperTests extends ESSingleNodeTestCase {
         IndexableField[] fields = doc.rootDoc().getFields("field");
         assertEquals(2, fields.length);
         
-        assertEquals("1234", fields[0].stringValue());
+        assertEquals(new BytesRef("1234"), fields[0].binaryValue());
         IndexableFieldType fieldType = fields[0].fieldType();
         assertThat(fieldType.omitNorms(), equalTo(true));
         assertFalse(fieldType.tokenized());
@@ -163,7 +163,7 @@ public class KeywordFieldMapperTests extends ESSingleNodeTestCase {
 
         fields = doc.rootDoc().getFields("field");
         assertEquals(2, fields.length);
-        assertEquals("uri", fields[0].stringValue());
+        assertEquals(new BytesRef("uri"), fields[0].binaryValue());
     }
 
     public void testEnableStore() throws IOException {

core/src/test/java/org/elasticsearch/index/mapper/MultiFieldTests.java

@@ -21,6 +21,7 @@ package org.elasticsearch.index.mapper;
 
 import org.apache.lucene.index.IndexOptions;
 import org.apache.lucene.index.IndexableField;
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.compress.CompressedXContent;
@@ -89,7 +90,7 @@ public class MultiFieldTests extends ESSingleNodeTestCase {
 
         f = doc.getField("object1.multi1.string");
         assertThat(f.name(), equalTo("object1.multi1.string"));
-        assertThat(f.stringValue(), equalTo("2010-01-01"));
+        assertThat(f.binaryValue(), equalTo(new BytesRef("2010-01-01")));
 
         assertThat(docMapper.mappers().getMapper("name"), notNullValue());
         assertThat(docMapper.mappers().getMapper("name"), instanceOf(TextFieldMapper.class));