Skip to content

Allow metadata fields in the _source #61590

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.

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

Merged
merged 15 commits into from
Sep 18, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,6 @@ private RankFeatureMetaFieldMapper() {
super(RankFeatureMetaFieldType.INSTANCE);
}

@Override
public void preParse(ParseContext context) {}

@Override
protected void parseCreateField(ParseContext context) {
throw new AssertionError("Should never be called");
}

@Override
public void postParse(ParseContext context) {}

@Override
protected String contentType() {
return CONTENT_TYPE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,9 @@ public boolean enabled() {
return this.enabled.value();
}

@Override
public void preParse(ParseContext context) {
}

@Override
public void postParse(ParseContext context) throws IOException {
// we post parse it so we get the size stored, possibly compressed (source will be preParse)
super.parse(context);
}

@Override
public void parse(ParseContext context) {
// nothing to do here, we call the parent in postParse
}

@Override
protected void parseCreateField(ParseContext context) {
if (enabled.value() == false) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,7 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper,
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
paths = splitAndValidatePath(currentFieldName);
if (context.mapperService().isMetadataField(context.path().pathAsText(currentFieldName))) {
throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside"
+ " a document. Use the index API request parameters.");
} else if (containsDisabledObjectMapper(mapper, paths)) {
if (containsDisabledObjectMapper(mapper, paths)) {
parser.nextToken();
parser.skipChildren();
}
Expand Down Expand Up @@ -482,7 +479,7 @@ private static void parseObject(final ParseContext context, ObjectMapper mapper,
String[] paths) throws IOException {
assert currentFieldName != null;

Mapper objectMapper = getMapper(mapper, currentFieldName, paths);
Mapper objectMapper = getMapper(context, mapper, currentFieldName, paths);
if (objectMapper != null) {
context.path().add(currentFieldName);
parseObjectOrField(context, objectMapper);
Expand Down Expand Up @@ -519,7 +516,7 @@ private static void parseArray(ParseContext context, ObjectMapper parentMapper,
String[] paths) throws IOException {
String arrayFieldName = lastFieldName;

Mapper mapper = getMapper(parentMapper, lastFieldName, paths);
Mapper mapper = getMapper(context, parentMapper, lastFieldName, paths);
if (mapper != null) {
// There is a concrete mapper for this field already. Need to check if the mapper
// expects an array, if so we pass the context straight to the mapper and if not
Expand Down Expand Up @@ -596,7 +593,7 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa
throw new MapperParsingException("object mapping [" + parentMapper.name() + "] trying to serialize a value with"
+ " no field associated with it, current value [" + context.parser().textOrNull() + "]");
}
Mapper mapper = getMapper(parentMapper, currentFieldName, paths);
Mapper mapper = getMapper(context, parentMapper, currentFieldName, paths);
if (mapper != null) {
parseObjectOrField(context, mapper);
} else {
Expand All @@ -613,7 +610,7 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa
private static void parseNullValue(ParseContext context, ObjectMapper parentMapper, String lastFieldName,
String[] paths) throws IOException {
// we can only handle null values if we have mappings for them
Mapper mapper = getMapper(parentMapper, lastFieldName, paths);
Mapper mapper = getMapper(context, parentMapper, lastFieldName, paths);
if (mapper != null) {
// TODO: passing null to an object seems bogus?
parseObjectOrField(context, mapper);
Expand Down Expand Up @@ -881,7 +878,16 @@ private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper parentMapper,
}

// looks up a child mapper, but takes into account field names that expand to objects
private static Mapper getMapper(ObjectMapper objectMapper, String fieldName, String[] subfields) {
private static Mapper getMapper(final ParseContext context, ObjectMapper objectMapper, String fieldName, String[] subfields) {
String fieldPath = context.path().pathAsText(fieldName);
if (context.mapperService().isMetadataField(fieldPath)) {
for (MetadataFieldMapper metadataFieldMapper : context.docMapper().mapping().getMetadataMappers()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed we're doing a linear scan through all the metadata mappers. Maybe we should make sure to store them as a map from name -> mapper to avoid this overhead?

Copy link
Contributor Author

@csoulios csoulios Sep 17, 2020

Choose a reason for hiding this comment

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

You are right. I had this point in mind for improvement. I just saw that linear scan happens at:

for (MetadataFieldMapper metadataMapper : metadataFieldsMappers) {

I understand that pre/postParse() internalParseDocument() is called once per document, while the linear scan in getMapper() is executed once per metadata field.

I will fix this asap!

Copy link
Contributor Author

@csoulios csoulios Sep 17, 2020

Choose a reason for hiding this comment

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

Fixed this last bit. Can you please have one last look?

Thank you for your patience and excellent guidance.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change makes sense to me.

It feels redundant that we store an array of metadata mappers, a map from class -> mapper, and a map from name -> mapper. This could be simplified, but we don't necessarily need to do it in this PR.

if (fieldPath.equals(metadataFieldMapper.name())) {
return metadataFieldMapper;
}
}
}

for (int i = 0; i < subfields.length - 1; ++i) {
Mapper mapper = objectMapper.getMapper(subfields[i]);
if (mapper == null || (mapper instanceof ObjectMapper) == false) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.index.query.QueryShardContext;

import java.io.IOException;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -160,19 +159,6 @@ public FieldNamesFieldType fieldType() {
return (FieldNamesFieldType) super.fieldType();
}

@Override
public void preParse(ParseContext context) {
}

@Override
public void postParse(ParseContext context) {
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
// Adding values to the _field_names field is handled by the mappers for each field type
}

static Iterable<String> extractFieldNames(final String fullPath) {
return new Iterable<String>() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,6 @@ private IdFieldMapper() {

@Override
public void preParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
BytesRef id = Uid.encodeId(context.sourceToParse().id());
context.doc().add(new Field(NAME, id, Defaults.FIELD_TYPE));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,8 @@ private IgnoredFieldMapper() {
super(IgnoredFieldType.INSTANCE);
}

@Override
public void preParse(ParseContext context) throws IOException {
}

@Override
public void postParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
public void parse(ParseContext context) throws IOException {
// done in post-parse
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
for (String field : context.getIgnoredFields()) {
context.doc().add(new Field(NAME, field, Defaults.FIELD_TYPE));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.util.Collections;
import java.util.function.Supplier;

Expand Down Expand Up @@ -73,12 +72,6 @@ public IndexFieldMapper() {
super(IndexFieldType.INSTANCE);
}

@Override
public void preParse(ParseContext context) throws IOException {}

@Override
protected void parseCreateField(ParseContext context) throws IOException {}

@Override
protected String contentType() {
return CONTENT_TYPE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ public Mapping merge(Mapping mergeWith, MergeReason reason) {
return new Mapping(indexCreated, mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta);
}

public MetadataFieldMapper[] getMetadataMappers() {
return metadataMappers;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
root.toXContent(builder, params, new ToXContent() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,18 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params)
return builder.endObject();
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
throw new MapperParsingException("Field [" + name() + "] is a metadata field and cannot be added inside"
+ " a document. Use the index API request parameters.");
}

/**
* Called before {@link FieldMapper#parse(ParseContext)} on the {@link RootObjectMapper}.
*/
public abstract void preParse(ParseContext context) throws IOException;
public void preParse(ParseContext context) throws IOException {
// do nothing
}

/**
* Called after {@link FieldMapper#parse(ParseContext)} on the {@link RootObjectMapper}.
Expand All @@ -169,5 +177,4 @@ public void postParse(ParseContext context) throws IOException {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup lookup, String format) {
throw new UnsupportedOperationException("Cannot fetch values for internal field [" + name() + "].");
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.query.QueryShardContext;

import java.io.IOException;
import java.util.Collections;

public class NestedPathFieldMapper extends MetadataFieldMapper {
Expand Down Expand Up @@ -93,24 +92,9 @@ private NestedPathFieldMapper(Settings settings) {
super(new NestedPathFieldType(settings));
}

@Override
public void preParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
public void parse(ParseContext context) throws IOException {
// we parse in pre parse
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected String contentType() {
return NAME;
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -117,18 +117,6 @@ public boolean required() {

@Override
public void preParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
public void parse(ParseContext context) throws IOException {
// no need ot parse here, we either get the routing in the sourceToParse
// or we don't have routing, if we get it in sourceToParse, we process it in preParse
// which will always be called
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
String routing = context.sourceToParse().routing();
if (routing != null) {
context.doc().add(new Field(fieldType().name(), routing, Defaults.FIELD_TYPE));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,6 @@ public SeqNoFieldMapper() {

@Override
public void preParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
// see InternalEngine.innerIndex to see where the real version value is set
// also see ParsedDocument.updateSeqID (called by innerIndex)
SequenceIDFields seqID = SequenceIDFields.emptySeqID();
Expand All @@ -196,11 +191,6 @@ protected void parseCreateField(ParseContext context) throws IOException {
context.doc().add(seqID.primaryTerm);
}

@Override
public void parse(ParseContext context) throws IOException {
// fields are added in parseCreateField
}

@Override
public void postParse(ParseContext context) throws IOException {
// In the case of nested docs, let's fill nested docs with the original
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,16 +156,6 @@ public boolean isComplete() {

@Override
public void preParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
public void parse(ParseContext context) throws IOException {
// nothing to do here, we will call it in pre parse
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
BytesReference originalSource = context.sourceToParse().source();
XContentType contentType = context.sourceToParse().getXContentType();
final BytesReference adaptedSource = applyFilters(originalSource, contentType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,21 +168,6 @@ private TypeFieldMapper() {
super(new TypeFieldType());
}

@Override
public void preParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
public void parse(ParseContext context) throws IOException {
// we parse in pre parse
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {

}

@Override
protected String contentType() {
return CONTENT_TYPE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,12 @@ private VersionFieldMapper() {

@Override
public void preParse(ParseContext context) throws IOException {
super.parse(context);
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
// see InternalEngine.updateVersion to see where the real version value is set
final Field version = new NumericDocValuesField(NAME, -1L);
context.version(version);
context.doc().add(version);
}

@Override
public void parse(ParseContext context) throws IOException {
// _version added in preparse
}

@Override
public void postParse(ParseContext context) throws IOException {
// In the case of nested docs, let's fill nested docs with version=1 so that Lucene doesn't write a Bitset for documents
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.MetadataFieldMapper;
import org.elasticsearch.index.mapper.ParametrizedFieldMapper;
import org.elasticsearch.index.mapper.ParseContext;
import org.elasticsearch.index.mapper.TextSearchInfo;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.indices.IndexTemplateMissingException;
Expand Down Expand Up @@ -1554,16 +1553,6 @@ public ParametrizedFieldMapper.Builder getMergeBuilder() {
return new MetadataTimestampFieldBuilder().init(this);
}

@Override
public void preParse(ParseContext context) {

}

@Override
protected void parseCreateField(ParseContext context) {

}

@Override
protected String contentType() {
return "_data_stream_timestamp";
Expand Down
Loading