From b521638f52b359de7a25f5d7719e0ec9e87d3cad Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Thu, 25 Aug 2016 13:12:05 +0200 Subject: [PATCH] Revert "Revert "Save one utf8 conversion in KeywordFieldMapper. #19867"" This reverts commit d805266d94adcf3643b77194a7895de6200f2914. --- .../index/get/ShardGetService.java | 5 +---- .../index/mapper/BaseGeoPointFieldMapper.java | 20 +++++++++++++------ .../index/mapper/GeoPointFieldMapper.java | 4 ++-- .../index/mapper/KeywordFieldMapper.java | 16 +++++++++++++-- .../mapper/LegacyGeoPointFieldMapper.java | 6 +++--- .../index/mapper/DynamicTemplatesTests.java | 13 ++++++------ .../mapper/ExternalFieldMapperTests.java | 3 ++- .../mapper/GeoPointFieldMapperTests.java | 19 +++++++++++++++--- .../mapper/GeohashMappingGeoPointTests.java | 3 ++- .../index/mapper/KeywordFieldMapperTests.java | 4 ++-- .../index/mapper/MultiFieldTests.java | 3 ++- 11 files changed, 65 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/get/ShardGetService.java b/core/src/main/java/org/elasticsearch/index/get/ShardGetService.java index 011ac7bcdc0..67237864595 100644 --- a/core/src/main/java/org/elasticsearch/index/get/ShardGetService.java +++ b/core/src/main/java/org/elasticsearch/index/get/ShardGetService.java @@ -239,10 +239,7 @@ public final class ShardGetService extends AbstractIndexShardComponent { } List values = searchLookup.source().extractRawValues(field); - if (!values.isEmpty()) { - for (int i = 0; i < values.size(); i++) { - values.set(i, fieldMapper.fieldType().valueForSearch(values.get(i))); - } + if (values.isEmpty() == false) { value = values; } } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/BaseGeoPointFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/BaseGeoPointFieldMapper.java index 72dc0f99801..2a24b6a94c6 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/BaseGeoPointFieldMapper.java +++ b/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 ignoreMalformed, CopyTo copyTo); + FieldMapper geoHashMapper, MultiFields multiFields, Explicit 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 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 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 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 diff --git a/core/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java index 590679cb989..c27ddc1811b 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java +++ b/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 ignoreMalformed, + FieldMapper lonMapper, FieldMapper geoHashMapper, MultiFields multiFields, Explicit 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 ignoreMalformed, CopyTo copyTo) { + FieldMapper geoHashMapper, MultiFields multiFields, Explicit ignoreMalformed, CopyTo copyTo) { super(simpleName, fieldType, defaultFieldType, indexSettings, latMapper, lonMapper, geoHashMapper, multiFields, ignoreMalformed, copyTo); } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java index b0468e8f83d..56c55e1475b 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java +++ b/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)); } } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/LegacyGeoPointFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/LegacyGeoPointFieldMapper.java index fda352069a4..3fe195c5d91 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/LegacyGeoPointFieldMapper.java +++ b/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 COERCE = new Explicit(false, false); + public static final Explicit 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 ignoreMalformed, + FieldMapper geoHashMapper, MultiFields multiFields, Explicit ignoreMalformed, CopyTo copyTo) { fieldType.setTokenized(false); setupFieldType(context); @@ -261,7 +261,7 @@ public class LegacyGeoPointFieldMapper extends BaseGeoPointFieldMapper implement protected Explicit 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 ignoreMalformed, Explicit coerce, CopyTo copyTo) { super(simpleName, fieldType, defaultFieldType, indexSettings, latMapper, lonMapper, geoHashMapper, multiFields, ignoreMalformed, copyTo); diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java index 845125849c1..9e4eb14e378 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java +++ b/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)); diff --git a/core/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java index f523f15820e..f1541ed02b9 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java +++ b/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 { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java index ce30fd2f8b7..9262c6d0d6d 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java +++ b/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 { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/GeohashMappingGeoPointTests.java b/core/src/test/java/org/elasticsearch/index/mapper/GeohashMappingGeoPointTests.java index b3fd8ae2c39..05581e79021 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/GeohashMappingGeoPointTests.java +++ b/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()); } diff --git a/core/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java index a32e1702490..9c4eed15bc9 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java +++ b/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 { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/MultiFieldTests.java b/core/src/test/java/org/elasticsearch/index/mapper/MultiFieldTests.java index 871bd20844d..4bf1995722b 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/MultiFieldTests.java +++ b/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));