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

[Remove] Default Mapping #2151

Merged
merged 1 commit into from
Feb 18, 2022
Merged

[Remove] Default Mapping #2151

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
4 changes: 2 additions & 2 deletions modules/parent-join/src/test/java/org/opensearch/join/mapper/ParentJoinFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ public void testUpdateRelations() throws Exception {
.endObject()
);
docMapper = indexService.mapperService()
.merge("_doc", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE);
Copy link
Collaborator

@reta reta Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@nknize I am a bit confused by this change: the only mapping which should left at this point is _doc, would we still let anyone to change the (single) mapping type for index (as here fe, _doc -> type)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I think I found the answer to my question)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

type support hasn't been completely removed yet so this will get cleaned up in a follow up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

.merge("type", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE);
ParentJoinFieldMapper mapper = ParentJoinFieldMapper.getMapper(indexService.mapperService());
assertNotNull(mapper);
assertEquals("join_field", mapper.name());
Expand All @@ -501,7 +501,7 @@ public void testUpdateRelations() throws Exception {
.endObject()
);
docMapper = indexService.mapperService()
.merge("_doc", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE);
.merge("type", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE);
ParentJoinFieldMapper mapper = ParentJoinFieldMapper.getMapper(indexService.mapperService());
assertNotNull(mapper);
assertEquals("join_field", mapper.name());
Expand Down
14 changes: 11 additions & 3 deletions server/src/internalClusterTest/java/org/opensearch/search/morelikethis/MoreLikeThisIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.index.query.MoreLikeThisQueryBuilder;
import org.opensearch.index.query.MoreLikeThisQueryBuilder.Item;
import org.opensearch.index.query.QueryBuilder;
Expand Down Expand Up @@ -740,16 +741,23 @@ public void testMoreLikeThisArtificialDocs() throws Exception {
assertHitCount(response, 1);
}

@AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/2107")
public void testMoreLikeThisMalformedArtificialDocs() throws Exception {
logger.info("Creating the index ...");
assertAcked(prepareCreate("test").addMapping("type1", "text", "type=text,analyzer=whitespace", "date", "type=date"));
assertAcked(
prepareCreate("test").addMapping(
MapperService.SINGLE_MAPPING_NAME,
"text",
"type=text,analyzer=whitespace",
"date",
"type=date"
)
);
ensureGreen("test");

logger.info("Creating an index with a single document ...");
indexRandom(
true,
client().prepareIndex("test", "type1", "1")
client().prepareIndex("test", MapperService.SINGLE_MAPPING_NAME, "1")
.setSource(jsonBuilder().startObject().field("text", "Hello World!").field("date", "2009-01-01").endObject())
);

Expand Down
7 changes: 2 additions & 5 deletions server/src/main/java/org/opensearch/action/admin/indices/get/GetIndexResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentParser.Token;
import org.opensearch.index.mapper.MapperService;

import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -301,10 +300,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
} else {
MappingMetadata mappings = null;
for (final ObjectObjectCursor<String, MappingMetadata> typeEntry : indexMappings) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably future change, but I believe we would significantly reduce the amount of "heap garbage" when replace ImmutableOpenMap<String, MappingMetadata> with just MappingMetadata since there is only one mapping from now on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think that would be a good followup PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

if (typeEntry.key.equals(MapperService.DEFAULT_MAPPING) == false) {
assert mappings == null;
mappings = typeEntry.value;
}
assert mappings == null;
mappings = typeEntry.value;
}
if (mappings == null) {
// no mappings yet
Expand Down
7 changes: 2 additions & 5 deletions ...c/main/java/org/opensearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.index.mapper.Mapper;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.rest.BaseRestHandler;

import java.io.IOException;
Expand Down Expand Up @@ -165,10 +164,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
if (includeTypeName == false) {
Map<String, FieldMappingMetadata> mappings = null;
for (Map.Entry<String, Map<String, FieldMappingMetadata>> typeEntry : indexEntry.getValue().entrySet()) {
if (typeEntry.getKey().equals(MapperService.DEFAULT_MAPPING) == false) {
assert mappings == null;
mappings = typeEntry.getValue();
}
assert mappings == null;
mappings = typeEntry.getValue();
}
if (mappings != null) {
addFieldMappingsToBuilder(builder, params, mappings);
Expand Down
6 changes: 2 additions & 4 deletions ...er/src/main/java/org/opensearch/action/admin/indices/mapping/get/GetMappingsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
if (includeTypeName == false) {
MappingMetadata mappings = null;
for (final ObjectObjectCursor<String, MappingMetadata> typeEntry : indexEntry.value) {
if (typeEntry.key.equals("_default_") == false) {
assert mappings == null;
mappings = typeEntry.value;
}
assert mappings == null;
mappings = typeEntry.value;
}
if (mappings == null) {
// no mappings yet
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/action/bulk/TransportBulkAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ protected void doRun() {
prohibitCustomRoutingOnDataStream(docWriteRequest, metadata);
IndexRequest indexRequest = (IndexRequest) docWriteRequest;
final IndexMetadata indexMetadata = metadata.index(concreteIndex);
MappingMetadata mappingMd = indexMetadata.mappingOrDefault();
MappingMetadata mappingMd = indexMetadata.mapping();
Version indexCreated = indexMetadata.getCreationVersion();
indexRequest.resolveRouting(metadata);
indexRequest.process(indexCreated, mappingMd, concreteIndex.getName());
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/action/bulk/TransportShardBulkAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ static boolean executeBulkItemRequest(
case UPDATED:
IndexRequest indexRequest = updateResult.action();
IndexMetadata metadata = context.getPrimary().indexSettings().getIndexMetadata();
MappingMetadata mappingMd = metadata.mappingOrDefault();
MappingMetadata mappingMd = metadata.mapping();
indexRequest.process(metadata.getCreationVersion(), mappingMd, updateRequest.concreteIndex());
context.setRequestToExecute(indexRequest);
break;
Expand Down
4 changes: 0 additions & 4 deletions server/src/main/java/org/opensearch/cluster/action/index/MappingUpdatedAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import org.opensearch.common.util.concurrent.UncategorizedExecutionException;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.index.Index;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.index.mapper.Mapping;

import java.util.concurrent.Semaphore;
Expand Down Expand Up @@ -111,9 +110,6 @@ public void setClient(Client client) {
* potentially waiting for a master node to be available.
*/
public void updateMappingOnMaster(Index index, String type, Mapping mappingUpdate, ActionListener<Void> listener) {
if (type.equals(MapperService.DEFAULT_MAPPING)) {
throw new IllegalArgumentException("_default_ mapping should not be updated");
}

final RunOnce release = new RunOnce(() -> semaphore.release());
try {
Expand Down
41 changes: 1 addition & 40 deletions server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.gateway.MetadataStateFormat;
import org.opensearch.index.Index;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.index.seqno.SequenceNumbers;
import org.opensearch.index.shard.ShardId;
import org.opensearch.rest.RestStatus;
Expand Down Expand Up @@ -677,22 +676,11 @@ public ImmutableOpenMap<String, MappingMetadata> getMappings() {
@Nullable
public MappingMetadata mapping() {
for (ObjectObjectCursor<String, MappingMetadata> cursor : mappings) {
if (cursor.key.equals(MapperService.DEFAULT_MAPPING) == false) {
return cursor.value;
}
return cursor.value;
}
return null;
}

/**
* Get the default mapping.
* NOTE: this is always {@code null} for 7.x indices which are disallowed to have a default mapping.
*/
@Nullable
public MappingMetadata defaultMapping() {
return mappings.get(MapperService.DEFAULT_MAPPING);
}

public static final String INDEX_RESIZE_SOURCE_UUID_KEY = "index.resize.source.uuid";
public static final String INDEX_RESIZE_SOURCE_NAME_KEY = "index.resize.source.name";
public static final Setting<String> INDEX_RESIZE_SOURCE_UUID = Setting.simpleString(INDEX_RESIZE_SOURCE_UUID_KEY);
Expand All @@ -704,25 +692,6 @@ public Index getResizeSourceIndex() {
: null;
}

/**
* Sometimes, the default mapping exists and an actual mapping is not created yet (introduced),
* in this case, we want to return the default mapping in case it has some default mapping definitions.
* <p>
* Note, once the mapping type is introduced, the default mapping is applied on the actual typed MappingMetadata,
* setting its routing, timestamp, and so on if needed.
*/
@Nullable
public MappingMetadata mappingOrDefault() {
MappingMetadata mapping = null;
for (ObjectCursor<MappingMetadata> m : mappings.values()) {
if (mapping == null || mapping.type().equals(MapperService.DEFAULT_MAPPING)) {
mapping = m.value;
}
}

return mapping;
}

ImmutableOpenMap<String, DiffableStringMap> getCustomData() {
return this.customData;
}
Expand Down Expand Up @@ -1337,14 +1306,6 @@ public IndexMetadata build() {
ImmutableOpenMap.Builder<String, AliasMetadata> tmpAliases = aliases;
Settings tmpSettings = settings;

// update default mapping on the MappingMetadata
if (mappings.containsKey(MapperService.DEFAULT_MAPPING)) {
MappingMetadata defaultMapping = mappings.get(MapperService.DEFAULT_MAPPING);
for (ObjectCursor<MappingMetadata> cursor : mappings.values()) {
cursor.value.updateDefaultMapping(defaultMapping);
}
}

/*
* We expect that the metadata has been properly built to set the number of shards and the number of replicas, and do not rely
* on the default values here. Those must have been set upstream.
Expand Down
9 changes: 3 additions & 6 deletions server/src/main/java/org/opensearch/cluster/metadata/IndexTemplateMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.common.xcontent.XContentHelper;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.index.mapper.MapperService;

import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -432,11 +431,9 @@ private static void toInnerXContent(
if (includeTypeName == false) {
Map<String, Object> documentMapping = null;
for (ObjectObjectCursor<String, CompressedXContent> cursor : indexTemplateMetadata.mappings()) {
if (!cursor.key.equals(MapperService.DEFAULT_MAPPING)) {
assert documentMapping == null;
Map<String, Object> mapping = XContentHelper.convertToMap(cursor.value.uncompressed(), true).v2();
documentMapping = reduceMapping(cursor.key, mapping);
}
assert documentMapping == null;
Map<String, Object> mapping = XContentHelper.convertToMap(cursor.value.uncompressed(), true).v2();
documentMapping = reduceMapping(cursor.key, mapping);
}

if (documentMapping != null) {
Expand Down
6 changes: 0 additions & 6 deletions server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,6 @@ private void initMappers(Map<String, Object> withoutType) {
}
}

void updateDefaultMapping(MappingMetadata defaultMapping) {
if (routing == Routing.EMPTY) {
routing = defaultMapping.routing();
}
}

public String type() {
return this.type;
}
Expand Down
12 changes: 4 additions & 8 deletions server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@
import java.nio.file.Path;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -474,7 +473,6 @@ private ClusterState applyCreateIndexWithTemporaryService(
request.index(),
aliases,
indexService.mapperService()::documentMapper,
() -> indexService.mapperService().documentMapper(MapperService.DEFAULT_MAPPING),
temporaryIndexMeta.getSettings(),
temporaryIndexMeta.getRoutingNumShards(),
sourceMetadata,
Expand Down Expand Up @@ -1099,7 +1097,6 @@ static IndexMetadata buildIndexMetadata(
String indexName,
List<AliasMetadata> aliases,
Supplier<DocumentMapper> documentMapperSupplier,
Supplier<DocumentMapper> defaultDocumentMapperSupplier,
Settings indexSettings,
int routingNumShards,
@Nullable IndexMetadata sourceMetadata,
Expand All @@ -1109,11 +1106,10 @@ static IndexMetadata buildIndexMetadata(
indexMetadataBuilder.system(isSystem);
// now, update the mappings with the actual source
Map<String, MappingMetadata> mappingsMetadata = new HashMap<>();
for (DocumentMapper mapper : Arrays.asList(documentMapperSupplier.get(), defaultDocumentMapperSupplier.get())) {
if (mapper != null) {
MappingMetadata mappingMd = new MappingMetadata(mapper);
mappingsMetadata.put(mapper.type(), mappingMd);
}
DocumentMapper mapper = documentMapperSupplier.get();
if (mapper != null) {
MappingMetadata mappingMd = new MappingMetadata(mapper);
mappingsMetadata.put(mapper.type(), mappingMd);
}

for (MappingMetadata mappingMd : mappingsMetadata.values()) {
Expand Down
51 changes: 15 additions & 36 deletions server/src/main/java/org/opensearch/cluster/metadata/MetadataMappingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,29 +190,20 @@ private boolean refreshIndexMapping(IndexService indexService, IndexMetadata.Bui
try {
List<String> updatedTypes = new ArrayList<>();
MapperService mapperService = indexService.mapperService();
for (DocumentMapper mapper : Arrays.asList(
mapperService.documentMapper(),
mapperService.documentMapper(MapperService.DEFAULT_MAPPING)
)) {
if (mapper != null) {
final String type = mapper.type();
if (!mapper.mappingSource().equals(builder.mapping(type).source())) {
updatedTypes.add(type);
}
DocumentMapper mapper = mapperService.documentMapper();
if (mapper != null) {
final String type = mapper.type();
if (!mapper.mappingSource().equals(builder.mapping(type).source())) {
updatedTypes.add(type);
}
}

// if a single type is not up-to-date, re-send everything
if (updatedTypes.isEmpty() == false) {
logger.warn("[{}] re-syncing mappings with cluster state because of types [{}]", index, updatedTypes);
dirty = true;
for (DocumentMapper mapper : Arrays.asList(
mapperService.documentMapper(),
mapperService.documentMapper(MapperService.DEFAULT_MAPPING)
)) {
if (mapper != null) {
builder.putMapping(new MappingMetadata(mapper));
}
if (mapper != null) {
builder.putMapping(new MappingMetadata(mapper));
}
}
} catch (Exception e) {
Expand Down Expand Up @@ -286,7 +277,6 @@ private ClusterState applyRequest(
// we used for the validation, it makes this mechanism little less scary (a little)
updateList.add(indexMetadata);
// try and parse it (no need to add it here) so we can bail early in case of parsing exception
DocumentMapper newMapper;
DocumentMapper existingMapper = mapperService.documentMapper();

String typeForUpdate = mapperService.getTypeForUpdate(mappingType, mappingUpdateSource);
Expand All @@ -299,15 +289,10 @@ private ClusterState applyRequest(
);
}

if (MapperService.DEFAULT_MAPPING.equals(request.type())) {
// _default_ types do not go through merging, but we do test the new settings. Also don't apply the old default
newMapper = mapperService.parse(request.type(), mappingUpdateSource, false);
} else {
newMapper = mapperService.parse(request.type(), mappingUpdateSource, existingMapper == null);
if (existingMapper != null) {
// first, simulate: just call merge and ignore the result
existingMapper.merge(newMapper.mapping(), MergeReason.MAPPING_UPDATE);
}
DocumentMapper newMapper = mapperService.parse(request.type(), mappingUpdateSource);
if (existingMapper != null) {
// first, simulate: just call merge and ignore the result
existingMapper.merge(newMapper.mapping(), MergeReason.MAPPING_UPDATE);
}
if (mappingType == null) {
mappingType = newMapper.type();
Expand All @@ -319,9 +304,7 @@ private ClusterState applyRequest(
}
assert mappingType != null;

if (MapperService.DEFAULT_MAPPING.equals(mappingType) == false
&& MapperService.SINGLE_MAPPING_NAME.equals(mappingType) == false
&& mappingType.charAt(0) == '_') {
if (MapperService.SINGLE_MAPPING_NAME.equals(mappingType) == false && mappingType.charAt(0) == '_') {
throw new InvalidTypeNameException("Document mapping type name can't start with '_', found: [" + mappingType + "]");
}
Metadata.Builder builder = Metadata.builder(metadata);
Expand Down Expand Up @@ -367,13 +350,9 @@ private ClusterState applyRequest(
IndexMetadata.Builder indexMetadataBuilder = IndexMetadata.builder(indexMetadata);
// Mapping updates on a single type may have side-effects on other types so we need to
// update mapping metadata on all types
for (DocumentMapper mapper : Arrays.asList(
mapperService.documentMapper(),
mapperService.documentMapper(MapperService.DEFAULT_MAPPING)
)) {
if (mapper != null) {
indexMetadataBuilder.putMapping(new MappingMetadata(mapper.mappingSource()));
}
DocumentMapper mapper = mapperService.documentMapper();
if (mapper != null) {
indexMetadataBuilder.putMapping(new MappingMetadata(mapper.mappingSource()));
}
if (updatedMapping) {
indexMetadataBuilder.mappingVersion(1 + indexMetadataBuilder.mappingVersion());
Expand Down
Loading