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).
elastic · Jan 31, 2014 · ee65026 · ee65026
1 parent 767c2e9
commit ee65026
Show file tree

Hide file tree

Showing 16 changed files with 44 additions and 142 deletions.
diff --git 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
@@ -165,8 +165,6 @@ public BytesRef getBinaryValue(String name) {
 
     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 boolean isWithinNewMapper() {
         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 Object externalValue() {
         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
@@ -236,6 +236,7 @@ public T addMultiField(Mapper.Builder mapperBuilder) {
             multiFieldsBuilder.add(mapperBuilder);
             return builder;
         }
+
         public T copyTo(CopyTo copyTo) {
             this.copyTo = copyTo;
             return builder;
@@ -283,7 +284,7 @@ protected AbstractFieldMapper(Names names, float boost, FieldType fieldType, Boo
         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 XContentBuilder toXContent(XContentBuilder builder, Params params) throws
      */
     public static class CopyTo {
 
-        private final ImmutableList<CopyToField> copyToFields;
+        private final ImmutableList<String> copyToFields;
 
-        private CopyTo(ImmutableList<CopyToField> copyToFields) {
+        private CopyTo(ImmutableList<String> copyToFields) {
             this.copyToFields = copyToFields;
         }
 
@@ -1007,28 +1008,28 @@ private CopyTo(ImmutableList<CopyToField> copyToFields) {
          */
         public void parse(ParseContext context) throws IOException {
             if (!context.isWithinCopyTo()) {
-                for (CopyToField field : copyToFields) {
-                    field.parse(context);
+                for (String field : copyToFields) {
+                    parse(field, context);
                 }
             }
         }
 
         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();
             }
             return builder;
         }
 
         public static class Builder {
-            private final ImmutableList.Builder<CopyToField> copyToBuilders = ImmutableList.builder();
+            private final ImmutableList.Builder<String> 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 CopyTo build() {
             }
         }
 
-        public ImmutableList<CopyToField> copyToFields() {
+        public ImmutableList<String> 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 void parse(ParseContext context) throws IOException {
             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
@@ -249,7 +249,7 @@ protected boolean customBoost() {
     @Override
     protected void innerParseCreateField(ParseContext context, List<Field> 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
@@ -458,7 +458,7 @@ protected boolean customBoost() {
     protected void innerParseCreateField(ParseContext context, List<Field> 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
@@ -244,7 +244,7 @@ protected boolean customBoost() {
     @Override
     protected void innerParseCreateField(ParseContext context, List<Field> 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
@@ -249,7 +249,7 @@ protected boolean customBoost() {
     @Override
     protected void innerParseCreateField(ParseContext context, List<Field> 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
@@ -244,7 +244,7 @@ protected boolean customBoost() {
     @Override
     protected void innerParseCreateField(ParseContext context, List<Field> 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
@@ -234,7 +234,7 @@ protected boolean customBoost() {
     @Override
     protected void innerParseCreateField(ParseContext context, List<Field> 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
@@ -249,7 +249,7 @@ protected boolean customBoost() {
     @Override
     protected void innerParseCreateField(ParseContext context, List<Field> 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
@@ -274,7 +274,7 @@ public Filter nullValueFilter() {
 
     @Override
     protected void parseCreateField(ParseContext context, List<Field> 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
@@ -126,7 +126,7 @@ protected TokenCountFieldMapper(Names names, int precisionStep, float boost, Fie
 
     @Override
     protected void parseCreateField(ParseContext context, List<Field> 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
@@ -240,7 +240,7 @@ public static void parseField(AbstractFieldMapper.Builder builder, String name,
                 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 static ContentPath.Type parsePathType(String name, String path) throws Ma
     }
 
     @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<Object>) 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<String, Object> copyToNode = (Map<String, Object>) 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
@@ -234,7 +234,7 @@ public void parse(ParseContext context) throws IOException {
             }
             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
@@ -295,7 +295,7 @@ protected void innerParseCreateField(ParseContext context, List<Field> fields) t
         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()) {