From 90da268237525dcc89d2e09a3f77b5a3262cf6f7 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Fri, 31 Jan 2014 12:33:38 -0500 Subject: [PATCH] Remove support for boost in copy_to field Currently, boosting on `copy_to` is misleading and does not work as originally specified in #4520. Instead of boosting just the terms from the origin field, it boosts the whole destination field. If two fields copy_to a third field, one with a boost of 2 and another with a boost of 3, all the terms in the third field end up with a boost of 6. This was not the intention. The alternative: to store the boost in a payload for every term, results in poor performance and inflexibility. Instead, users should either (1) query the common field AND the field that requires boosting, or (2) the multi_match query will soon be able to perform term-centric cross-field matching that will allow per-field boosting at query time (coming in 1.1). --- .../mapping/types/core-types.asciidoc | 23 +------- .../index/mapper/ParseContext.java | 14 +---- .../mapper/core/AbstractFieldMapper.java | 56 +++++-------------- .../index/mapper/core/ByteFieldMapper.java | 2 +- .../index/mapper/core/DateFieldMapper.java | 2 +- .../index/mapper/core/DoubleFieldMapper.java | 2 +- .../index/mapper/core/FloatFieldMapper.java | 2 +- .../index/mapper/core/IntegerFieldMapper.java | 2 +- .../index/mapper/core/LongFieldMapper.java | 2 +- .../index/mapper/core/ShortFieldMapper.java | 2 +- .../index/mapper/core/StringFieldMapper.java | 2 +- .../mapper/core/TokenCountFieldMapper.java | 2 +- .../index/mapper/core/TypeParsers.java | 37 ++---------- .../index/mapper/geo/GeoShapeFieldMapper.java | 2 +- .../index/mapper/ip/IpFieldMapper.java | 2 +- .../mapper/copyto/CopyToMapperTests.java | 34 ++++------- 16 files changed, 44 insertions(+), 142 deletions(-) diff --git a/docs/reference/mapping/types/core-types.asciidoc b/docs/reference/mapping/types/core-types.asciidoc index 1322fd8feb0..98267d553fe 100644 --- a/docs/reference/mapping/types/core-types.asciidoc +++ b/docs/reference/mapping/types/core-types.asciidoc @@ -637,33 +637,14 @@ the parameter. In the following example all values from fields `title` and `abst } -------------------------------------------------- -Multiple fields with optional boost factors are also supported: +Multiple fields are also supported: [source,js] -------------------------------------------------- { "book" : { "properties" : { - "title" : { "type" : "string", "copy_to" : ["meta_data", "article_info^10.0"] }, - } -} --------------------------------------------------- - -The same mapping can be also defined using extended syntax: - -[source,js] --------------------------------------------------- -{ - "book" : { - "properties" : { - "title" : { - "type" : "string", - "copy_to" : [{ - "field": "meta_data" - }, { - "field": "article_info" - "boost": 10.0 - } + "title" : { "type" : "string", "copy_to" : ["meta_data", "article_info"] }, } } -------------------------------------------------- diff --git a/src/main/java/org/elasticsearch/index/mapper/ParseContext.java b/src/main/java/org/elasticsearch/index/mapper/ParseContext.java index a917d8f9925..973f28967ee 100644 --- a/src/main/java/org/elasticsearch/index/mapper/ParseContext.java +++ b/src/main/java/org/elasticsearch/index/mapper/ParseContext.java @@ -165,8 +165,6 @@ public class ParseContext { private float docBoost = 1.0f; - private float copyToBoost = 1.0f; - public ParseContext(String index, @Nullable Settings indexSettings, DocumentMapperParser docMapperParser, DocumentMapper docMapper, ContentPath path) { this.index = index; this.indexSettings = indexSettings; @@ -227,14 +225,12 @@ public class ParseContext { return withinNewMapper; } - public void setWithinCopyTo(float copyToBoost) { + public void setWithinCopyTo() { this.withinCopyTo = true; - this.copyToBoost = copyToBoost; } public void clearWithinCopyTo() { this.withinCopyTo = false; - this.copyToBoost = 1.0f; } public boolean isWithinCopyTo() { @@ -400,14 +396,6 @@ public class ParseContext { return externalValue; } - public float fieldBoost(AbstractFieldMapper mapper) { - if (withinCopyTo) { - return copyToBoost; - } else { - return mapper.boost(); - } - } - public float docBoost() { return this.docBoost; } diff --git a/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java index b7e64ae793b..b2b2409501b 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java @@ -236,6 +236,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { multiFieldsBuilder.add(mapperBuilder); return builder; } + public T copyTo(CopyTo copyTo) { this.copyTo = copyTo; return builder; @@ -283,7 +284,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { this(names, boost, fieldType, docValues, indexAnalyzer, searchAnalyzer, postingsFormat, docValuesFormat, similarity, normsLoading, fieldDataSettings, indexSettings, MultiFields.empty(), null); } - + protected AbstractFieldMapper(Names names, float boost, FieldType fieldType, Boolean docValues, NamedAnalyzer indexAnalyzer, NamedAnalyzer searchAnalyzer, PostingsFormatProvider postingsFormat, DocValuesFormatProvider docValuesFormat, SimilarityProvider similarity, @@ -996,9 +997,9 @@ public abstract class AbstractFieldMapper implements FieldMapper { */ public static class CopyTo { - private final ImmutableList copyToFields; + private final ImmutableList copyToFields; - private CopyTo(ImmutableList copyToFields) { + private CopyTo(ImmutableList copyToFields) { this.copyToFields = copyToFields; } @@ -1007,8 +1008,8 @@ public abstract class AbstractFieldMapper implements FieldMapper { */ public void parse(ParseContext context) throws IOException { if (!context.isWithinCopyTo()) { - for (CopyToField field : copyToFields) { - field.parse(context); + for (String field : copyToFields) { + parse(field, context); } } } @@ -1016,8 +1017,8 @@ public abstract class AbstractFieldMapper implements FieldMapper { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { if (!copyToFields.isEmpty()) { builder.startArray("copy_to"); - for (CopyToField field : copyToFields) { - field.toXContent(builder, params); + for (String field : copyToFields) { + builder.value(field); } builder.endArray(); } @@ -1025,10 +1026,10 @@ public abstract class AbstractFieldMapper implements FieldMapper { } public static class Builder { - private final ImmutableList.Builder copyToBuilders = ImmutableList.builder(); + private final ImmutableList.Builder copyToBuilders = ImmutableList.builder(); - public Builder add(String field, float boost) { - copyToBuilders.add(new CopyToField(field, boost)); + public Builder add(String field) { + copyToBuilders.add(field); return this; } @@ -1037,28 +1038,15 @@ public abstract class AbstractFieldMapper implements FieldMapper { } } - public ImmutableList copyToFields() { + public ImmutableList copyToFields() { return copyToFields; } - } - - public static class CopyToField { - - private final String field; - - protected final float boost; - - public CopyToField(String field, float boost) { - this.field = field; - this.boost = boost; - } - /** * Creates an copy of the current field with given field name and boost */ - public void parse(ParseContext context) throws IOException { - context.setWithinCopyTo(boost); + public void parse(String field, ParseContext context) throws IOException { + context.setWithinCopyTo(); FieldMappers mappers = context.docMapper().mappers().indexName(field); if (mappers != null && !mappers.isEmpty()) { mappers.mapper().parse(context); @@ -1115,23 +1103,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { context.clearWithinCopyTo(); } - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(); - builder.field("field", field); - if (boost != 1.0 || params.paramAsBoolean("include_defaults", false)) { - builder.field("boost", boost); - } - builder.endObject(); - return builder; - } - public String field() { - return field; - } - - public float boost() { - return boost; - } } } diff --git a/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java index 1bde8dc48a4..196bb8e57ed 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java @@ -249,7 +249,7 @@ public class ByteFieldMapper extends NumberFieldMapper { @Override protected void innerParseCreateField(ParseContext context, List fields) throws IOException { byte value; - float boost = context.fieldBoost(this); + float boost = this.boost; if (context.externalValueSet()) { Object externalValue = context.externalValue(); if (externalValue == null) { diff --git a/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java index f54aefb4337..431c48e53d2 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java @@ -458,7 +458,7 @@ public class DateFieldMapper extends NumberFieldMapper { protected void innerParseCreateField(ParseContext context, List fields) throws IOException { String dateAsString = null; Long value = null; - float boost = context.fieldBoost(this); + float boost = this.boost; if (context.externalValueSet()) { Object externalValue = context.externalValue(); if (externalValue instanceof Number) { diff --git a/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java index 59455d9e400..6f449a9dbd2 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java @@ -244,7 +244,7 @@ public class DoubleFieldMapper extends NumberFieldMapper { @Override protected void innerParseCreateField(ParseContext context, List fields) throws IOException { double value; - float boost = context.fieldBoost(this); + float boost = this.boost; if (context.externalValueSet()) { Object externalValue = context.externalValue(); if (externalValue == null) { diff --git a/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java index d434ddd33fb..1e9de2baa9d 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java @@ -249,7 +249,7 @@ public class FloatFieldMapper extends NumberFieldMapper { @Override protected void innerParseCreateField(ParseContext context, List fields) throws IOException { float value; - float boost = context.fieldBoost(this); + float boost = this.boost; if (context.externalValueSet()) { Object externalValue = context.externalValue(); if (externalValue == null) { diff --git a/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java index dd9c398fd82..1d8efa7f896 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java @@ -244,7 +244,7 @@ public class IntegerFieldMapper extends NumberFieldMapper { @Override protected void innerParseCreateField(ParseContext context, List fields) throws IOException { int value; - float boost = context.fieldBoost(this); + float boost = this.boost; if (context.externalValueSet()) { Object externalValue = context.externalValue(); if (externalValue == null) { diff --git a/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java index 0d1cc33810b..4ada6196a53 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java @@ -234,7 +234,7 @@ public class LongFieldMapper extends NumberFieldMapper { @Override protected void innerParseCreateField(ParseContext context, List fields) throws IOException { long value; - float boost = context.fieldBoost(this); + float boost = this.boost; if (context.externalValueSet()) { Object externalValue = context.externalValue(); if (externalValue == null) { diff --git a/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java index 44f41ab6794..1edc7ac2e71 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java @@ -249,7 +249,7 @@ public class ShortFieldMapper extends NumberFieldMapper { @Override protected void innerParseCreateField(ParseContext context, List fields) throws IOException { short value; - float boost = context.fieldBoost(this); + float boost = this.boost; if (context.externalValueSet()) { Object externalValue = context.externalValue(); if (externalValue == null) { diff --git a/src/main/java/org/elasticsearch/index/mapper/core/StringFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/StringFieldMapper.java index 46adc792e76..a108fcb4139 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/StringFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/StringFieldMapper.java @@ -274,7 +274,7 @@ public class StringFieldMapper extends AbstractFieldMapper implements Al @Override protected void parseCreateField(ParseContext context, List fields) throws IOException { - ValueAndBoost valueAndBoost = parseCreateFieldForString(context, nullValue, context.fieldBoost(this)); + ValueAndBoost valueAndBoost = parseCreateFieldForString(context, nullValue, boost); if (valueAndBoost.value() == null) { return; } diff --git a/src/main/java/org/elasticsearch/index/mapper/core/TokenCountFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/TokenCountFieldMapper.java index 7f60eccf9cc..513e9118924 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/TokenCountFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/TokenCountFieldMapper.java @@ -126,7 +126,7 @@ public class TokenCountFieldMapper extends IntegerFieldMapper { @Override protected void parseCreateField(ParseContext context, List fields) throws IOException { - ValueAndBoost valueAndBoost = StringFieldMapper.parseCreateFieldForString(context, null /* Out null value is an int so we convert*/, context.fieldBoost(this)); + ValueAndBoost valueAndBoost = StringFieldMapper.parseCreateFieldForString(context, null /* Out null value is an int so we convert*/, boost); if (valueAndBoost.value() == null && nullValue() == null) { return; } diff --git a/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java b/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java index 05b84357fcc..5bfc7803f10 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java @@ -240,7 +240,7 @@ public class TypeParsers { final Settings settings = ImmutableSettings.builder().put(SettingsLoader.Helper.loadNestedFromMap(nodeMapValue(propNode, "fielddata"))).build(); builder.fieldDataSettings(settings); } else if (propName.equals("copy_to")) { - parseCopyFields(name, propNode, builder); + parseCopyFields(propNode, builder); } } } @@ -367,43 +367,16 @@ public class TypeParsers { } @SuppressWarnings("unchecked") - public static void parseCopyFields(String fieldName, Object propNode, AbstractFieldMapper.Builder builder) { + public static void parseCopyFields(Object propNode, AbstractFieldMapper.Builder builder) { AbstractFieldMapper.CopyTo.Builder copyToBuilder = new AbstractFieldMapper.CopyTo.Builder(); - if (isObject(propNode)) { - parseCopyToField(fieldName, propNode, copyToBuilder); - } else if (isArray(propNode)) { + if (isArray(propNode)) { for(Object node : (List) propNode) { - if (isObject(node)) { - parseCopyToField(fieldName, node, copyToBuilder); - } else { - parseCopyToField(nodeStringValue(node, null), copyToBuilder); - } + copyToBuilder.add(nodeStringValue(node, null)); } } else { - parseCopyToField(nodeStringValue(propNode, null), copyToBuilder); + copyToBuilder.add(nodeStringValue(propNode, null)); } builder.copyTo(copyToBuilder.build()); } - public static void parseCopyToField(String fieldValue, AbstractFieldMapper.CopyTo.Builder builder) { - int carrotPos = fieldValue.indexOf('^'); - if (carrotPos > 0) { - builder.add(fieldValue.substring(0, carrotPos), Float.parseFloat(fieldValue.substring(carrotPos + 1))); - } else { - builder.add(fieldValue, 1.0f); - } - } - - public static void parseCopyToField(String fieldName, Object propNode, AbstractFieldMapper.CopyTo.Builder builder) { - Map copyToNode = (Map) propNode; - String field = nodeStringValue(copyToNode.get("field"), null); - if (field == null) { - throw new MapperParsingException("No type specified for property [" + fieldName + "]"); - } - float boost = nodeFloatValue(copyToNode.get("boost"), 1.0f); - builder.add(field, boost); - - - } - } diff --git a/src/main/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapper.java index d7ae1e7612f..91ac65be186 100644 --- a/src/main/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapper.java @@ -234,7 +234,7 @@ public class GeoShapeFieldMapper extends AbstractFieldMapper { } for (Field field : fields) { if (!customBoost()) { - field.setBoost(context.fieldBoost(this)); + field.setBoost(boost); } if (context.listener().beforeFieldAdded(this, field, context)) { context.doc().add(field); diff --git a/src/main/java/org/elasticsearch/index/mapper/ip/IpFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/ip/IpFieldMapper.java index a58831f3f5c..4d11c8a3bf3 100644 --- a/src/main/java/org/elasticsearch/index/mapper/ip/IpFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/ip/IpFieldMapper.java @@ -295,7 +295,7 @@ public class IpFieldMapper extends NumberFieldMapper { final long value = ipToLong(ipAsString); if (fieldType.indexed() || fieldType.stored()) { CustomLongNumericField field = new CustomLongNumericField(this, value, fieldType); - field.setBoost(context.fieldBoost(this)); + field.setBoost(boost); fields.add(field); } if (hasDocValues()) { diff --git a/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java b/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java index e1117fe1a60..35fca85b70a 100644 --- a/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java +++ b/src/test/java/org/elasticsearch/index/mapper/copyto/CopyToMapperTests.java @@ -50,7 +50,7 @@ public class CopyToMapperTests extends ElasticsearchTestCase { String mapping = jsonBuilder().startObject().startObject("type1").startObject("properties") .startObject("copy_test") .field("type", "string") - .array("copy_to", "another_field^10", "cyclic_test") + .array("copy_to", "another_field", "cyclic_test") .endObject() .startObject("another_field") @@ -64,16 +64,7 @@ public class CopyToMapperTests extends ElasticsearchTestCase { .startObject("int_to_str_test") .field("type", "integer") - .startArray("copy_to") - .startObject() - .field("field", "another_field") - .field("boost", 5.5f) - .endObject() - .startObject() - .field("field", "new_field") - .endObject() - .endArray() - + .array("copy_to", "another_field", "new_field") .endObject() .endObject().endObject().endObject().string(); @@ -89,11 +80,10 @@ public class CopyToMapperTests extends ElasticsearchTestCase { Map serializedMap = JsonXContent.jsonXContent.createParser(builder.bytes()).mapAndClose(); Map copyTestMap = (Map) serializedMap.get("copy_test"); assertThat(copyTestMap.get("type").toString(), is("string")); - List> copyToMap = (List>) copyTestMap.get("copy_to"); - assertThat(copyToMap.size(), equalTo(2)); - assertThat(copyToMap.get(0).get("field").toString(), equalTo("another_field")); - assertThat((Double) copyToMap.get(0).get("boost"), closeTo(10.0, 0.001)); - assertThat(copyToMap.get(1).get("field").toString(), equalTo("cyclic_test")); + List copyToList = (List) copyTestMap.get("copy_to"); + assertThat(copyToList.size(), equalTo(2)); + assertThat(copyToList.get(0).toString(), equalTo("another_field")); + assertThat(copyToList.get(1).toString(), equalTo("cyclic_test")); // Check data parsing BytesReference json = jsonBuilder().startObject() @@ -109,9 +99,7 @@ public class CopyToMapperTests extends ElasticsearchTestCase { assertThat(doc.getFields("another_field").length, equalTo(2)); assertThat(doc.getFields("another_field")[0].stringValue(), equalTo("foo")); - assertThat((double) (doc.getFields("another_field")[0].boost()), closeTo(10.0, 0.001)); assertThat(doc.getFields("another_field")[1].stringValue(), equalTo("42")); - assertThat((double) (doc.getFields("another_field")[1].boost()), closeTo(5.5, 0.001)); assertThat(doc.getFields("cyclic_test").length, equalTo(2)); assertThat(doc.getFields("cyclic_test")[0].stringValue(), equalTo("foo")); @@ -213,11 +201,11 @@ public class CopyToMapperTests extends ElasticsearchTestCase { DocumentMapper docMapperBefore = MapperTestUtils.newParser().parse(mappingBefore); - ImmutableList fields = docMapperBefore.mappers().name("copy_test").mapper().copyTo().copyToFields(); + ImmutableList fields = docMapperBefore.mappers().name("copy_test").mapper().copyTo().copyToFields(); assertThat(fields.size(), equalTo(2)); - assertThat(fields.get(0).field(), equalTo("foo")); - assertThat(fields.get(1).field(), equalTo("bar")); + assertThat(fields.get(0), equalTo("foo")); + assertThat(fields.get(1), equalTo("bar")); DocumentMapper docMapperAfter = MapperTestUtils.newParser().parse(mappingAfter); @@ -231,8 +219,8 @@ public class CopyToMapperTests extends ElasticsearchTestCase { fields = docMapperBefore.mappers().name("copy_test").mapper().copyTo().copyToFields(); assertThat(fields.size(), equalTo(2)); - assertThat(fields.get(0).field(), equalTo("baz")); - assertThat(fields.get(1).field(), equalTo("bar")); + assertThat(fields.get(0), equalTo("baz")); + assertThat(fields.get(1), equalTo("bar")); } }