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.

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

Merged
merged 15 commits into from
Sep 18, 2020
Merged

Allow metadata fields in the _source #61590

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8a65d1c
Configurable metadata field mappers in the _source
csoulios Aug 26, 2020
d83b677
Changes to support metadata fields in _source
csoulios Sep 2, 2020
76f3dc3
Merge branch 'master' into metadata-source
csoulios Sep 8, 2020
4934129
Merged DocumentParserTests from master
csoulios Sep 9, 2020
a168a1b
Merge branch 'master' into metadata-source
csoulios Sep 9, 2020
2effa2f
Handle non string values
csoulios Sep 10, 2020
1f5aa63
Allow metadata fields to parse values/objects/arrays/null
csoulios Sep 16, 2020
78faca3
Removed MetadataFieldMapper.isAllowedInSource() method
csoulios Sep 16, 2020
1346865
Merge branch 'master' into metadata-source
csoulios Sep 16, 2020
39d19fe
Fixed bug that caused tests to break
csoulios Sep 16, 2020
f0bb957
Cleanup parsing for existing metadata fields
csoulios Sep 17, 2020
a734ea8
Cleanup parsing for existing metadata fields
csoulios Sep 17, 2020
b8b3a9d
Remove doParse() method
csoulios Sep 17, 2020
1560e73
Fix broken test
csoulios Sep 17, 2020
0c962c4
Lookup metadata mapper by name
csoulios Sep 17, 2020
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
6 changes: 0 additions & 6 deletions ...apper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,11 @@ 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
11 changes: 1 addition & 10 deletions plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,10 @@ 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
doParse(context);
}

@Override
Expand Down
25 changes: 16 additions & 9 deletions server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Open in desktop
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,8 @@ 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 +611,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 +879,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 •
edited
Loading

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:

elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java

Line 101 in b8b3a9d

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 •
edited
Loading

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
14 changes: 0 additions & 14 deletions server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ private IdFieldMapper() {

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

@Override
Expand Down
11 changes: 1 addition & 10 deletions server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,9 @@ 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
doParse(context);
}

@Override
Expand Down
7 changes: 0 additions & 7 deletions server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java
View file
Open in desktop
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
4 changes: 4 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/Mapping.java
View file
Open in desktop
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
28 changes: 26 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,35 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params)
return builder.endObject();
}

/*
* By default metadata fields cannot be set through the document source and parse() method
* throws an exception. To enable a metadata field to parse the document source, this
* method must be overridden and doParse() should be called.
*/
@Override
public void parse(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.");
}

/**
* Do the actual parse of the field by calling {@link FieldMapper#parse(ParseContext)}
*/
protected void doParse(ParseContext context) throws IOException {
super.parse(context);
}

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

Choose a reason for hiding this comment

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

Instead of overriding parse here, could we override parseCreateField?

@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.");
}

Then we could do the following:

  • For meta fields that cannot be specified in _source, move all the relevant logic out of parseCreateField and into preParse or postParse as appropriate.
  • Remove the new doParse method, as it's no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I had a hard time thinking a simple and clean way to refactor this part.

Initially, I tried the approach you suggested, but FieldMapper#parse() does all the exception handling by encapsulating all exceptions thrown by parseCreateField() offering a preview of the parsed value.

(See

elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

Line 249 in 9a127ad

} catch (Exception e) {
)

This means that the

    throw new MapperParsingException("Field [" + name() + "] is a metadata field and cannot be added inside"
        + " a document. Use the index API request parameters.");

will be encapsulated in the following exception:

     throw new MapperParsingException("failed to parse field [{}] of type [{}] in document with id '{}'. " +
                "Preview of field's value: '{}'", e, fieldType().name(), fieldType().typeName(),
                context.sourceToParse().id(), valuePreview);

On the other hand, methods preParse() and postParse() delegate parsing to parse() for the same reason (to handle parsing exception). If we were to move all parsing in those two methods, we would have to replicate exception handling in those methods as well.

Since all FieldMapper#parse() does is to delegate parsing to parseCreateField() and only do the exception handling, I decided to redirect this functionality in 'MetadataFieldMapper#doParse()and call this method frompreParse()andpostParse()`.

Copy link
Contributor

@jtibshirani jtibshirani Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

To me that approach you tried/ I suggested is the cleanest way to go. My reasoning...

This means that the ... will be encapsulated in the following exception:

This seems like an okay compromise to me, all the exception information is there so the user can determine the cause. It's a little confusing that we wrap the message, but this should be a rare situation.

On the other hand, methods preParse() and postParse() delegate parsing to parse() for the same reason (to handle parsing exception).

I think that exception handling code is only really helpful when dealing with incorrect user-supplied values. Since preParse and postParse work with internal data such as context.sourceToParse().id(), it would indicate a serious logic error. So I don't think we'd need the same general exception handling strategy where we create a nice user-facing message.

// do nothing
}

/**
* 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 +194,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() + "].");
}

}
12 changes: 1 addition & 11 deletions server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,22 +95,12 @@ private NestedPathFieldMapper(Settings 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 {
doParse(context);
}

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


}
9 changes: 1 addition & 8 deletions server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,7 @@ 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
doParse(context);
}

@Override
Expand Down
7 changes: 1 addition & 6 deletions server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ public SeqNoFieldMapper() {

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

@Override
Expand All @@ -196,11 +196,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
7 changes: 1 addition & 6 deletions server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,12 +156,7 @@ 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
doParse(context);
}

@Override
Expand Down
12 changes: 1 addition & 11 deletions server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,17 +170,7 @@ private TypeFieldMapper() {

@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 {

doParse(context);
}

@Override
Expand Down
7 changes: 1 addition & 6 deletions server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ private VersionFieldMapper() {

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

@Override
Expand All @@ -79,11 +79,6 @@ protected void parseCreateField(ParseContext context) throws IOException {
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
11 changes: 0 additions & 11 deletions ...r/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
View file
Open in desktop
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