Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mapping update for “date_range” field type is not idempotent #2094

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Mapping update for “date_range” field type is not idempotent #2094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion modules/percolator/src/main/java/org/opensearch/percolator/PercolatorFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,12 @@ static BinaryFieldMapper createQueryBuilderFieldBuilder(BuilderContext context)
}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about how this change works. This builder.build(context) is where the failure was coming from previously, right? (this build() method would call the setupFieldType() method which would call context.indexCreatedVersion() which would trigger the "[index.version.created] is not present in the index settings..." exception). Won't this change trigger the same exception on line 198 when it calls context.indexCreatedVersion()?

Copy link
Collaborator Author

@reta reta Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct but the important part is what context is used when method is called. The exception was raised because BuilderContext was created out of empty settings, but I will double check where this method is called to make sure no exceptions are going to be raised. Thanks for spotting it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrross made a change to fetch index created only when it is available, should be safer, thanks a lot!

Expand Down
18 changes: 18 additions & 0 deletions server/src/main/java/org/opensearch/index/mapper/Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -69,6 +71,14 @@ public Settings indexSettings() {
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<T extends Builder> {
Expand Down Expand Up @@ -240,4 +250,12 @@ public final String simpleName() {
*/
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());
}
}
16 changes: 12 additions & 4 deletions server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
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;
Expand Down Expand Up @@ -116,15 +117,17 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
private final Parameter<Map<String, String>> 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();
Expand Down Expand Up @@ -157,8 +160,11 @@ protected RangeFieldType setupFieldType(BuilderContext context) {
+ " 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());
Expand Down Expand Up @@ -371,6 +377,7 @@ public Query rangeQuery(
private final Locale locale;

private final boolean coerceByDefault;
private final Version indexCreatedVersion;

private RangeFieldMapper(
String simpleName,
Expand All @@ -389,6 +396,7 @@ private RangeFieldMapper(
this.format = builder.format.getValue();
this.locale = builder.locale.getValue();
this.coerceByDefault = builder.coerce.getDefaultValue().value();
this.indexCreatedVersion = builder.indexCreatedVersion;
}

boolean coerce() {
Expand All @@ -397,7 +405,7 @@ boolean coerce() {

@Override
public ParametrizedFieldMapper.Builder getMergeBuilder() {
return new Builder(simpleName(), type, coerceByDefault).init(this);
return new Builder(simpleName(), type, coerceByDefault, indexCreatedVersion).init(this);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/opensearch/index/mapper/IpRangeFieldTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public void testFetchSourceValue() throws IOException {
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<String, Object> 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")),
Expand Down
9 changes: 9 additions & 0 deletions server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
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;
Expand Down Expand Up @@ -374,4 +375,12 @@ public void testIllegalFormatField() throws Exception {
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);
}
}
}
14 changes: 8 additions & 6 deletions server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,16 +536,17 @@ public void testFetchSourceValue() throws IOException {
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<String, Object> 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<String, Object> 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")),
Expand All @@ -557,14 +558,15 @@ public void testParseSourceValueWithFormat() throws IOException {
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<String, Object> 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<String, Object> dateRange = org.opensearch.common.collect.Map.of("lt", "1990-12-29T00:00:00.000Z");
Expand Down