From 6b6f03368f49f5f8001d6d0ed85cd9af7bab76f6 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Mon, 14 Feb 2022 17:56:02 -0500 Subject: [PATCH] =?UTF-8?q?=20Mapping=20update=20for=20=E2=80=9Cdate=5Fran?= =?UTF-8?q?ge=E2=80=9D=20field=20type=20is=20not=20idempotent=20(#2094)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Andriy Redko --- .../percolator/PercolatorFieldMapper.java | 7 ++++++- .../org/opensearch/index/mapper/Mapper.java | 18 ++++++++++++++++++ .../index/mapper/RangeFieldMapper.java | 16 ++++++++++++---- .../index/mapper/IpRangeFieldTypeTests.java | 2 +- .../index/mapper/RangeFieldMapperTests.java | 9 +++++++++ .../index/mapper/RangeFieldTypeTests.java | 14 ++++++++------ 6 files changed, 54 insertions(+), 12 deletions(-) diff --git a/modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java index aaf81ad576f..a8b0395dd84 100644 --- a/modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java @@ -195,7 +195,12 @@ public class PercolatorFieldMapper extends ParametrizedFieldMapper { } static RangeFieldMapper createExtractedRangeFieldBuilder(String name, RangeType rangeType, BuilderContext context) { - RangeFieldMapper.Builder builder = new RangeFieldMapper.Builder(name, rangeType, true); + RangeFieldMapper.Builder builder = new RangeFieldMapper.Builder( + name, + rangeType, + true, + hasIndexCreated(context.indexSettings()) ? context.indexCreatedVersion() : null + ); // For now no doc values, because in processQuery(...) only the Lucene range fields get added: builder.docValues(false); return builder.build(context); diff --git a/server/src/main/java/org/opensearch/index/mapper/Mapper.java b/server/src/main/java/org/opensearch/index/mapper/Mapper.java index 27507ff78c7..f9f16a33a0c 100644 --- a/server/src/main/java/org/opensearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/Mapper.java @@ -33,6 +33,8 @@ package org.opensearch.index.mapper; import org.opensearch.Version; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.Nullable; import org.opensearch.common.settings.Settings; import org.opensearch.common.time.DateFormatter; import org.opensearch.common.xcontent.ToXContentFragment; @@ -69,6 +71,14 @@ public abstract class Mapper implements ToXContentFragment, Iterable { public Version indexCreatedVersion() { return Version.indexCreated(indexSettings); } + + public Version indexCreatedVersionOrDefault(@Nullable Version defaultValue) { + if (defaultValue == null || hasIndexCreated(indexSettings)) { + return indexCreatedVersion(); + } else { + return defaultValue; + } + } } public abstract static class Builder { @@ -240,4 +250,12 @@ public abstract class Mapper implements ToXContentFragment, Iterable { */ public abstract void validate(MappingLookup mappers); + /** + * Check if settings have IndexMetadata.SETTING_INDEX_VERSION_CREATED setting. + * @param settings settings + * @return "true" if settings have IndexMetadata.SETTING_INDEX_VERSION_CREATED setting, "false" otherwise + */ + protected static boolean hasIndexCreated(Settings settings) { + return settings.hasValue(IndexMetadata.SETTING_INDEX_VERSION_CREATED.getKey()); + } } diff --git a/server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java index 2464f482e33..6ddb6d21ed5 100644 --- a/server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java @@ -36,6 +36,7 @@ import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.Query; import org.apache.lucene.util.BytesRef; import org.opensearch.OpenSearchException; +import org.opensearch.Version; import org.opensearch.common.Explicit; import org.opensearch.common.collect.Tuple; import org.opensearch.common.geo.ShapeRelation; @@ -116,15 +117,17 @@ public class RangeFieldMapper extends ParametrizedFieldMapper { private final Parameter> meta = Parameter.metaParam(); private final RangeType type; + private final Version indexCreatedVersion; public Builder(String name, RangeType type, Settings settings) { - this(name, type, COERCE_SETTING.get(settings)); + this(name, type, COERCE_SETTING.get(settings), hasIndexCreated(settings) ? Version.indexCreated(settings) : null); } - public Builder(String name, RangeType type, boolean coerceByDefault) { + public Builder(String name, RangeType type, boolean coerceByDefault, Version indexCreatedVersion) { super(name); this.type = type; this.coerce = Parameter.explicitBoolParam("coerce", true, m -> toType(m).coerce, coerceByDefault); + this.indexCreatedVersion = indexCreatedVersion; if (this.type != RangeType.DATE) { format.neverSerialize(); locale.neverSerialize(); @@ -157,8 +160,11 @@ public class RangeFieldMapper extends ParametrizedFieldMapper { + " type" ); } + + // The builder context may not have index created version, falling back to indexCreatedVersion + // property of this mapper builder. DateFormatter dateTimeFormatter; - if (Joda.isJodaPattern(context.indexCreatedVersion(), format.getValue())) { + if (Joda.isJodaPattern(context.indexCreatedVersionOrDefault(indexCreatedVersion), format.getValue())) { dateTimeFormatter = Joda.forPattern(format.getValue()).withLocale(locale.getValue()); } else { dateTimeFormatter = DateFormatter.forPattern(format.getValue()).withLocale(locale.getValue()); @@ -371,6 +377,7 @@ public class RangeFieldMapper extends ParametrizedFieldMapper { private final Locale locale; private final boolean coerceByDefault; + private final Version indexCreatedVersion; private RangeFieldMapper( String simpleName, @@ -389,6 +396,7 @@ public class RangeFieldMapper extends ParametrizedFieldMapper { this.format = builder.format.getValue(); this.locale = builder.locale.getValue(); this.coerceByDefault = builder.coerce.getDefaultValue().value(); + this.indexCreatedVersion = builder.indexCreatedVersion; } boolean coerce() { @@ -397,7 +405,7 @@ public class RangeFieldMapper extends ParametrizedFieldMapper { @Override public ParametrizedFieldMapper.Builder getMergeBuilder() { - return new Builder(simpleName(), type, coerceByDefault).init(this); + return new Builder(simpleName(), type, coerceByDefault, indexCreatedVersion).init(this); } @Override diff --git a/server/src/test/java/org/opensearch/index/mapper/IpRangeFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/IpRangeFieldTypeTests.java index 98edd61e2fc..c2c6293eec4 100644 --- a/server/src/test/java/org/opensearch/index/mapper/IpRangeFieldTypeTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/IpRangeFieldTypeTests.java @@ -46,7 +46,7 @@ public class IpRangeFieldTypeTests extends FieldTypeTestCase { Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build(); Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath()); - RangeFieldMapper mapper = new RangeFieldMapper.Builder("field", RangeType.IP, true).build(context); + RangeFieldMapper mapper = new RangeFieldMapper.Builder("field", RangeType.IP, true, Version.V_EMPTY).build(context); Map range = org.opensearch.common.collect.Map.of("gte", "2001:db8:0:0:0:0:2:1"); assertEquals( Collections.singletonList(org.opensearch.common.collect.Map.of("gte", "2001:db8::2:1")), diff --git a/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java index 31c90d38053..0353173e256 100644 --- a/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java @@ -41,6 +41,7 @@ import org.opensearch.common.network.InetAddresses; import org.opensearch.common.xcontent.ToXContent; import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.index.mapper.MapperService.MergeReason; import java.io.IOException; import java.net.InetAddress; @@ -374,4 +375,12 @@ public class RangeFieldMapperTests extends AbstractNumericFieldMapperTestCase { assertThat(e.getMessage(), containsString("Invalid format: [[test_format]]: Unknown pattern letter: t")); } + public void testUpdatesWithSameMappings() throws Exception { + for (final String type : types()) { + final DocumentMapper mapper = createDocumentMapper(rangeFieldMapping(type, b -> { b.field("store", true); })); + + final Mapping mapping = mapper.mapping(); + mapper.merge(mapping, MergeReason.MAPPING_UPDATE); + } + } } diff --git a/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java index c35830c5089..d4772f24cca 100644 --- a/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java @@ -536,16 +536,17 @@ public class RangeFieldTypeTests extends FieldTypeTestCase { Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build(); Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath()); - MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true).build(context).fieldType(); + MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true, Version.V_EMPTY).build(context) + .fieldType(); Map longRange = org.opensearch.common.collect.Map.of("gte", 3.14, "lt", "42.9"); assertEquals( Collections.singletonList(org.opensearch.common.collect.Map.of("gte", 3L, "lt", 42L)), fetchSourceValue(longMapper, longRange) ); - MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true).format("yyyy/MM/dd||epoch_millis") - .build(context) - .fieldType(); + MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true, Version.V_EMPTY).format( + "yyyy/MM/dd||epoch_millis" + ).build(context).fieldType(); Map dateRange = org.opensearch.common.collect.Map.of("lt", "1990/12/29", "gte", 597429487111L); assertEquals( Collections.singletonList(org.opensearch.common.collect.Map.of("lt", "1990/12/29", "gte", "1988/12/06")), @@ -557,14 +558,15 @@ public class RangeFieldTypeTests extends FieldTypeTestCase { Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build(); Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath()); - MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true).build(context).fieldType(); + MappedFieldType longMapper = new RangeFieldMapper.Builder("field", RangeType.LONG, true, Version.V_EMPTY).build(context) + .fieldType(); Map longRange = org.opensearch.common.collect.Map.of("gte", 3.14, "lt", "42.9"); assertEquals( Collections.singletonList(org.opensearch.common.collect.Map.of("gte", 3L, "lt", 42L)), fetchSourceValue(longMapper, longRange) ); - MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true).format("strict_date_time") + MappedFieldType dateMapper = new RangeFieldMapper.Builder("field", RangeType.DATE, true, Version.V_EMPTY).format("strict_date_time") .build(context) .fieldType(); Map dateRange = org.opensearch.common.collect.Map.of("lt", "1990-12-29T00:00:00.000Z");