From 33339ab288be675f92635b52d1a992c72a8a55ab Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 23 Jun 2015 08:59:30 -0700 Subject: [PATCH] Addressed PR comments --- .../create/CreateIndexClusterStateUpdateRequest.java | 1 + .../elasticsearch/index/mapper/DocumentParser.java | 4 ++-- .../elasticsearch/index/mapper/FieldTypeLookup.java | 8 +++----- .../index/mapper/MappedFieldTypeReference.java | 12 ++++++------ .../index/mapper/core/AbstractFieldMapper.java | 11 +++++++---- .../index/mapper/core/NumberFieldMapper.java | 2 +- .../index/mapper/internal/AllFieldMapper.java | 3 +-- .../index/mapper/internal/IdFieldMapper.java | 5 ++--- .../index/mapper/internal/IndexFieldMapper.java | 5 ++--- .../index/mapper/internal/ParentFieldMapper.java | 3 +-- .../index/mapper/internal/TimestampFieldMapper.java | 5 ++--- .../index/mapper/internal/UidFieldMapper.java | 5 ++--- 12 files changed, 30 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexClusterStateUpdateRequest.java b/core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexClusterStateUpdateRequest.java index d915ad98165..16a399faff1 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexClusterStateUpdateRequest.java +++ b/core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexClusterStateUpdateRequest.java @@ -129,6 +129,7 @@ public class CreateIndexClusterStateUpdateRequest extends ClusterStateUpdateRequ return blocks; } + /** True if all fields that span multiple types should be updated, false otherwise */ public boolean updateAllTypes() { return updateAllTypes; } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index c400f12e41e..441a798fa0f 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -529,7 +529,7 @@ class DocumentParser implements Closeable { builder = MapperBuilders.longField(currentFieldName); } return builder; - } catch (Exception e) { + } catch (NumberFormatException e) { // not a long number } try { @@ -539,7 +539,7 @@ class DocumentParser implements Closeable { builder = MapperBuilders.doubleField(currentFieldName); } return builder; - } catch (Exception e) { + } catch (NumberFormatException e) { // not a long number } } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java b/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java index 23c77fd5248..c4eab151a56 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -24,10 +24,8 @@ import com.google.common.collect.Iterators; import com.google.common.collect.Sets; import org.elasticsearch.common.collect.CopyOnWriteHashMap; import org.elasticsearch.common.regex.Regex; -import org.elasticsearch.index.mapper.object.ObjectMapper; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Iterator; import java.util.List; @@ -101,7 +99,7 @@ class FieldTypeLookup implements Iterable { } /** - * Checks if the give mapper's field types are compatibile with existing field types. + * Checks if the given mappers' field types are compatible with existing field types. * If any are not compatible, an IllegalArgumentException is thrown. * If updateAllTypes is true, only basic compatibility is checked. */ @@ -109,7 +107,7 @@ class FieldTypeLookup implements Iterable { for (FieldMapper fieldMapper : newFieldMappers) { MappedFieldTypeReference ref = fullNameToFieldType.get(fieldMapper.fieldType().names().fullName()); if (ref != null) { - boolean strict = ref.getRefCount() > 1 && updateAllTypes == false; + boolean strict = ref.getNumAssociatedMappers() > 1 && updateAllTypes == false; List conflicts = new ArrayList<>(); ref.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict); if (conflicts.isEmpty() == false) { @@ -120,7 +118,7 @@ class FieldTypeLookup implements Iterable { // field type for the index name must be compatible too MappedFieldTypeReference indexNameRef = fullNameToFieldType.get(fieldMapper.fieldType().names().indexName()); if (indexNameRef != null) { - boolean strict = indexNameRef.getRefCount() > 1 && updateAllTypes == false; + boolean strict = indexNameRef.getNumAssociatedMappers() > 1 && updateAllTypes == false; List conflicts = new ArrayList<>(); indexNameRef.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict); if (conflicts.isEmpty() == false) { diff --git a/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldTypeReference.java b/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldTypeReference.java index 8153f004cee..d3c6b83a6a3 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldTypeReference.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/MappedFieldTypeReference.java @@ -23,12 +23,12 @@ package org.elasticsearch.index.mapper; */ public class MappedFieldTypeReference { private MappedFieldType fieldType; // the current field type this reference points to - private int refCount; + private int numAssociatedMappers; public MappedFieldTypeReference(MappedFieldType fieldType) { fieldType.freeze(); // ensure frozen this.fieldType = fieldType; - this.refCount = 1; + this.numAssociatedMappers = 1; } public MappedFieldType get() { @@ -40,11 +40,11 @@ public class MappedFieldTypeReference { this.fieldType = fieldType; } - public int getRefCount() { - return refCount; + public int getNumAssociatedMappers() { + return numAssociatedMappers; } - public void incRefCount() { - ++refCount; + public void incrementAssociatedMappers() { + ++numAssociatedMappers; } } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java index fe24c10b4af..e60cba25c87 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/core/AbstractFieldMapper.java @@ -350,7 +350,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { if (ref.get().equals(fieldType()) == false) { throw new IllegalStateException("Cannot overwrite field type reference to unequal reference"); } - ref.incRefCount(); + ref.incrementAssociatedMappers(); this.fieldTypeRef = ref; } @@ -409,7 +409,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { } AbstractFieldMapper fieldMergeWith = (AbstractFieldMapper) mergeWith; List subConflicts = new ArrayList<>(); // TODO: just expose list from MergeResult? - boolean strict = this.fieldTypeRef.getRefCount() > 1 && mergeResult.updateAllTypes() == false; + boolean strict = this.fieldTypeRef.getNumAssociatedMappers() > 1 && mergeResult.updateAllTypes() == false; fieldType().checkCompatibility(fieldMergeWith.fieldType(), subConflicts, strict); for (String conflict : subConflicts) { mergeResult.addConflict(conflict); @@ -482,8 +482,7 @@ public abstract class AbstractFieldMapper implements FieldMapper { } TreeMap orderedFielddataSettings = new TreeMap<>(); - boolean hasCustomFieldDataSettings = customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; - if (hasCustomFieldDataSettings) { + if (hasCustomFieldDataSettings()) { orderedFielddataSettings.putAll(customFieldDataSettings.getAsMap()); builder.field("fielddata", orderedFielddataSettings); } else if (includeDefaults) { @@ -563,6 +562,10 @@ public abstract class AbstractFieldMapper implements FieldMapper { } } + protected boolean hasCustomFieldDataSettings() { + return customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; + } + protected abstract String contentType(); @Override diff --git a/core/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java index 71bf4694a39..070d0d0a001 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java @@ -319,7 +319,7 @@ public abstract class NumberFieldMapper extends AbstractFieldMapper implements A return; } NumberFieldMapper nfmMergeWith = (NumberFieldMapper) mergeWith; - if (this.fieldTypeRef.getRefCount() > 1 && mergeResult.updateAllTypes() == false) { + if (this.fieldTypeRef.getNumAssociatedMappers() > 1 && mergeResult.updateAllTypes() == false) { if (fieldType().numericPrecisionStep() != nfmMergeWith.fieldType().numericPrecisionStep()) { mergeResult.addConflict("mapper [" + fieldType().names().fullName() + "] is used by multiple types. Set update_all_types to true to update precision_step across all types."); } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/internal/AllFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/internal/AllFieldMapper.java index 282ea212246..68539208fd3 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/internal/AllFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/internal/AllFieldMapper.java @@ -314,8 +314,7 @@ public class AllFieldMapper extends AbstractFieldMapper implements RootMapper { builder.field("similarity", SimilarityLookupService.DEFAULT_SIMILARITY); } - boolean hasCustomFieldDataSettings = customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; - if (hasCustomFieldDataSettings) { + if (hasCustomFieldDataSettings()) { builder.field("fielddata", (Map) customFieldDataSettings.getAsMap()); } else if (includeDefaults) { builder.field("fielddata", (Map) fieldType().fieldDataType().getSettings().getAsMap()); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java index c66c02e826d..5a6c7e1c121 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/internal/IdFieldMapper.java @@ -310,13 +310,12 @@ public class IdFieldMapper extends AbstractFieldMapper implements RootMapper { return builder; } boolean includeDefaults = params.paramAsBoolean("include_defaults", false); - boolean hasCustomFieldDataSettings = customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; // if all are defaults, no sense to write it at all if (!includeDefaults && fieldType().stored() == Defaults.FIELD_TYPE.stored() && fieldType().indexOptions() == Defaults.FIELD_TYPE.indexOptions() && path == Defaults.PATH - && hasCustomFieldDataSettings == false) { + && hasCustomFieldDataSettings() == false) { return builder; } builder.startObject(CONTENT_TYPE); @@ -330,7 +329,7 @@ public class IdFieldMapper extends AbstractFieldMapper implements RootMapper { builder.field("path", path); } - if (hasCustomFieldDataSettings) { + if (hasCustomFieldDataSettings()) { builder.field("fielddata", (Map) customFieldDataSettings.getAsMap()); } else if (includeDefaults) { builder.field("fielddata", (Map) fieldType().fieldDataType().getSettings().getAsMap()); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/internal/IndexFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/internal/IndexFieldMapper.java index 4c650e3dea7..5f60e1abcc7 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/internal/IndexFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/internal/IndexFieldMapper.java @@ -206,10 +206,9 @@ public class IndexFieldMapper extends AbstractFieldMapper implements RootMapper @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { boolean includeDefaults = params.paramAsBoolean("include_defaults", false); - boolean hasCustomFieldDataSettings = customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; // if all defaults, no need to write it at all - if (!includeDefaults && fieldType().stored() == Defaults.FIELD_TYPE.stored() && enabledState == Defaults.ENABLED_STATE && hasCustomFieldDataSettings == false) { + if (!includeDefaults && fieldType().stored() == Defaults.FIELD_TYPE.stored() && enabledState == Defaults.ENABLED_STATE && hasCustomFieldDataSettings() == false) { return builder; } builder.startObject(CONTENT_TYPE); @@ -221,7 +220,7 @@ public class IndexFieldMapper extends AbstractFieldMapper implements RootMapper } if (indexCreatedBefore2x) { - if (hasCustomFieldDataSettings) { + if (hasCustomFieldDataSettings()) { builder.field("fielddata", (Map) customFieldDataSettings.getAsMap()); } else if (includeDefaults) { builder.field("fielddata", (Map) fieldType().fieldDataType().getSettings().getAsMap()); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java index bfde40a91c9..29d81a5ed44 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java @@ -324,11 +324,10 @@ public class ParentFieldMapper extends AbstractFieldMapper implements RootMapper return builder; } boolean includeDefaults = params.paramAsBoolean("include_defaults", false); - boolean hasCustomFieldDataSettings = customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; builder.startObject(CONTENT_TYPE); builder.field("type", type); - if (hasCustomFieldDataSettings) { + if (hasCustomFieldDataSettings()) { builder.field("fielddata", (Map) customFieldDataSettings.getAsMap()); } else if (includeDefaults) { builder.field("fielddata", (Map) fieldType().fieldDataType().getSettings().getAsMap()); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java index f6624b600cb..bb55e06b72f 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java @@ -331,10 +331,9 @@ public class TimestampFieldMapper extends DateFieldMapper implements RootMapper boolean includeDefaults = params.paramAsBoolean("include_defaults", false); boolean indexed = fieldType().indexOptions() != IndexOptions.NONE; boolean indexedDefault = Defaults.FIELD_TYPE.indexOptions() != IndexOptions.NONE; - boolean hasCustomFieldDataSettings = customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; // if all are defaults, no sense to write it at all - if (!includeDefaults && indexed == indexedDefault && hasCustomFieldDataSettings == false && + if (!includeDefaults && indexed == indexedDefault && hasCustomFieldDataSettings() == false && fieldType().stored() == Defaults.FIELD_TYPE.stored() && enabledState == Defaults.ENABLED && path == Defaults.PATH && fieldType().dateTimeFormatter().format().equals(Defaults.DATE_TIME_FORMATTER.format()) && Defaults.DEFAULT_TIMESTAMP.equals(defaultTimestamp) @@ -367,7 +366,7 @@ public class TimestampFieldMapper extends DateFieldMapper implements RootMapper builder.field("ignore_missing", ignoreMissing); } if (indexCreatedBefore2x) { - if (hasCustomFieldDataSettings) { + if (hasCustomFieldDataSettings()) { builder.field("fielddata", (Map) customFieldDataSettings.getAsMap()); } else if (includeDefaults) { builder.field("fielddata", (Map) fieldType().fieldDataType().getSettings().getAsMap()); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/internal/UidFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/internal/UidFieldMapper.java index 8690e184c19..8203622444f 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/internal/UidFieldMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/internal/UidFieldMapper.java @@ -219,16 +219,15 @@ public class UidFieldMapper extends AbstractFieldMapper implements RootMapper { return builder; } boolean includeDefaults = params.paramAsBoolean("include_defaults", false); - boolean hasCustomFieldDataSettings = customFieldDataSettings != null && customFieldDataSettings.equals(Settings.EMPTY) == false; // if defaults, don't output - if (!includeDefaults && hasCustomFieldDataSettings == false) { + if (!includeDefaults && hasCustomFieldDataSettings() == false) { return builder; } builder.startObject(CONTENT_TYPE); - if (hasCustomFieldDataSettings) { + if (hasCustomFieldDataSettings()) { builder.field("fielddata", (Map) customFieldDataSettings.getAsMap()); } else if (includeDefaults) { builder.field("fielddata", (Map) fieldType().fieldDataType().getSettings().getAsMap());