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

Add declarative parameters to FieldMappers #58663

Merged
merged 17 commits into from
Jul 9, 2020
Merged

Add declarative parameters to FieldMappers #58663

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9dda2f3
First go: need to fix merging with BuilderContext
romseygeek Jun 23, 2020
6c118f3
Merge remote-tracking branch 'origin/master' into mapper/mergebuilders
romseygeek Jun 28, 2020
56ad229
dedicated tests for parametrized mapper
romseygeek Jun 29, 2020
da0aca5
tests
romseygeek Jun 29, 2020
6e99626
precommit
romseygeek Jun 29, 2020
7326c49
meta parsing
romseygeek Jun 29, 2020
ae14ac5
Merge remote-tracking branch 'origin/master' into mapper/mergebuilders
romseygeek Jun 29, 2020
5bb39b2
Use simple name in merge builder
romseygeek Jun 30, 2020
a31b1a6
Merge remote-tracking branch 'origin/master' into mapper/mergebuilders
romseygeek Jun 30, 2020
928a15f
feedback
romseygeek Jun 30, 2020
47c17e4
checkstyle
romseygeek Jun 30, 2020
4bf9327
wut
romseygeek Jul 1, 2020
3011064
Merge remote-tracking branch 'origin/master' into mapper/mergebuilders
romseygeek Jul 2, 2020
bb35eda
fix object serialization; javadocs
romseygeek Jul 2, 2020
b9dd287
Merge branch 'master' into mapper/mergebuilders
elasticmachine Jul 7, 2020
8f1853f
Merge remote-tracking branch 'origin/master' into mapper/mergebuilders
romseygeek Jul 8, 2020
37ea8d6
feedback
romseygeek Jul 8, 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
5 changes: 1 addition & 4 deletions modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,7 @@ static KeywordFieldMapper createExtractQueryFieldBuilder(String name, BuilderCon
}

static BinaryFieldMapper createQueryBuilderFieldBuilder(BuilderContext context) {
BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(QUERY_BUILDER_FIELD_NAME);
builder.docValues(true);
builder.indexOptions(IndexOptions.NONE);
Copy link
Member

Choose a reason for hiding this comment

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

Note to self - these were the defaults so they are safe to drop.

builder.store(false);
BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(QUERY_BUILDER_FIELD_NAME, true);
return builder.build(context);
}

Expand Down
7 changes: 7 additions & 0 deletions server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,13 @@ public static String nodeStringValue(Object node, String defaultValue) {
return node.toString();
}

public static String nodeStringValue(Object node) {
Copy link
Member

Choose a reason for hiding this comment

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

Its probably worth adding javadoc about how this is different from Objects.toString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

if (node == null) {
return null;
}
return node.toString();
}

public static float nodeFloatValue(Object node, float defaultValue) {
if (node == null) {
return defaultValue;
Expand Down
78 changes: 34 additions & 44 deletions server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
package org.elasticsearch.index.mapper;

import com.carrotsearch.hppc.ObjectArrayList;
import org.apache.lucene.document.FieldType;
import org.apache.lucene.document.StoredField;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.DocValuesFieldExistsQuery;
import org.apache.lucene.search.Query;
Expand All @@ -48,42 +46,39 @@
import java.util.List;
import java.util.Map;

import static org.elasticsearch.index.mapper.TypeParsers.parseField;

public class BinaryFieldMapper extends FieldMapper {
public class BinaryFieldMapper extends ParametrizedFieldMapper {

public static final String CONTENT_TYPE = "binary";

public static class Defaults {
public static final FieldType FIELD_TYPE = new FieldType();

static {
FIELD_TYPE.setIndexOptions(IndexOptions.NONE);
FIELD_TYPE.setOmitNorms(true);
FIELD_TYPE.freeze();
}
private static BinaryFieldMapper toType(FieldMapper in) {
return (BinaryFieldMapper) in;
}

public static class Builder extends FieldMapper.Builder<Builder> {
public static class Builder extends ParametrizedFieldMapper.Builder {

private final Parameter<Boolean> stored = Parameter.boolParam("store", false, m -> toType(m).stored, false);
private final Parameter<Boolean> hasDocValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues, false);
private final Parameter<Map<String, String>> meta
= new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta());
Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to eventually move meta over to the FieldMapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

meta is used in field caps, which currently are based off MappedFieldType so I think it will probably stay there?


public Builder(String name) {
super(name, Defaults.FIELD_TYPE);
hasDocValues = false;
builder = this;
this(name, false);
}

public Builder(String name, boolean hasDocValues) {
super(name);
this.hasDocValues.setValue(hasDocValues);
}

@Override
public BinaryFieldMapper build(BuilderContext context) {
return new BinaryFieldMapper(name, fieldType, new BinaryFieldType(buildFullName(context), hasDocValues, meta),
multiFieldsBuilder.build(this, context), copyTo);
public List<Parameter<?>> getParameters() {
return List.of(meta, stored, hasDocValues);
}

@Override
public Builder index(boolean index) {
if (index) {
throw new MapperParsingException("Binary field [" + name() + "] cannot be indexed");
}
return builder;
public BinaryFieldMapper build(BuilderContext context) {
return new BinaryFieldMapper(name, new BinaryFieldType(buildFullName(context), hasDocValues.getValue(), meta.getValue()),
multiFieldsBuilder.build(this, context), copyTo.build(), this);
}
}

Expand All @@ -92,7 +87,7 @@ public static class TypeParser implements Mapper.TypeParser {
public BinaryFieldMapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext)
throws MapperParsingException {
BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(name);
parseField(builder, name, node, parserContext);
builder.parse(name, parserContext, node);
return builder;
}
}
Expand Down Expand Up @@ -167,14 +162,19 @@ public Query termQuery(Object value, QueryShardContext context) {
}
}

protected BinaryFieldMapper(String simpleName, FieldType fieldType, MappedFieldType mappedFieldType,
MultiFields multiFields, CopyTo copyTo) {
super(simpleName, fieldType, mappedFieldType, multiFields, copyTo);
private final boolean stored;
private final boolean hasDocValues;

protected BinaryFieldMapper(String simpleName, MappedFieldType mappedFieldType,
MultiFields multiFields, CopyTo copyTo, Builder builder) {
super(simpleName, mappedFieldType, multiFields, copyTo);
this.stored = builder.stored.getValue();
this.hasDocValues = builder.hasDocValues.getValue();
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
if (!fieldType.stored() && !fieldType().hasDocValues()) {
if (stored == false && hasDocValues == false) {
return;
}
byte[] value = context.parseExternalValue(byte[].class);
Expand All @@ -188,11 +188,11 @@ protected void parseCreateField(ParseContext context) throws IOException {
if (value == null) {
return;
}
if (fieldType.stored()) {
if (stored) {
context.doc().add(new StoredField(fieldType().name(), value));
}

if (fieldType().hasDocValues()) {
if (hasDocValues) {
CustomBinaryDocValuesField field = (CustomBinaryDocValuesField) context.doc().getByKey(fieldType().name());
if (field == null) {
field = new CustomBinaryDocValuesField(fieldType().name(), value);
Expand All @@ -210,18 +210,8 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected boolean indexedByDefault() {
return false;
}

@Override
protected boolean docValuesByDefault() {
return false;
}

@Override
protected void mergeOptions(FieldMapper other, List<String> conflicts) {

public ParametrizedFieldMapper.Builder getMergeBuilder() {
return new BinaryFieldMapper.Builder(simpleName()).init(this);
}

@Override
Expand Down
83 changes: 39 additions & 44 deletions server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.lucene.document.Field;
import org.apache.lucene.document.FieldType;
import org.apache.lucene.document.SortedNumericDocValuesField;
import org.apache.lucene.document.StoredField;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.DocValuesFieldExistsQuery;
Expand All @@ -30,7 +31,6 @@
import org.apache.lucene.search.TermRangeQuery;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.fielddata.IndexFieldData;
Expand All @@ -42,16 +42,13 @@
import java.io.IOException;
import java.time.ZoneId;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.index.mapper.TypeParsers.parseField;

/**
* A field mapper for boolean fields.
*/
public class BooleanFieldMapper extends FieldMapper {
public class BooleanFieldMapper extends ParametrizedFieldMapper {

public static final String CONTENT_TYPE = "boolean";

Expand All @@ -71,25 +68,37 @@ public static class Values {
public static final BytesRef FALSE = new BytesRef("F");
}

public static class Builder extends FieldMapper.Builder<Builder> {
private static BooleanFieldMapper toType(FieldMapper in) {
return (BooleanFieldMapper) in;
}

public static class Builder extends ParametrizedFieldMapper.Builder {

private Boolean nullValue;
private final Parameter<Float> boost = Parameter.floatParam("boost", true, m -> m.fieldType().boost(), 1.0f);
private final Parameter<Boolean> docValues
= Parameter.boolParam("doc_values", false, m -> m.fieldType().hasDocValues(), true);
private final Parameter<Boolean> indexed
= Parameter.boolParam("index", false, m -> m.fieldType().isSearchable(), true);
private final Parameter<Boolean> nullValue
= new Parameter<>("null_value", false, null, (n, o) -> XContentMapValues.nodeBooleanValue(o), m -> toType(m).nullValue);
private final Parameter<Boolean> stored = Parameter.boolParam("store", false, m -> toType(m).stored, false);
private final Parameter<Map<String, String>> meta
= new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta());
Copy link
Member

Choose a reason for hiding this comment

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

I get confused here that stored seems to be treated differently from e.g. doc_values. Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Convenience, mainly - doc_values and index can be derived from the field type values, but stored can't be. I can change it so that doc_values and index are fields directly on the mapper as well if you think that's clearer? Certainly for things like searchable docvalues fields then we'll need to distinguish between whether or not something is indexed and whether it's searchable.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I am trying to see what the difference is between stored and indexed/doc_values. I would expect them to be treated in the same way/same place. Why can't stored be derived from the field type values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it stands, MappedFieldType doesn't have anything that tells you if the field is stored or not.

Copy link
Member

Choose a reason for hiding this comment

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

You made a boolean hasDocValues on BinaryFieldMapper for this. Is it worth making one here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add indexed and hasDocValues directly to the mappers as it sounds as though that will be less confusing.


public Builder(String name) {
super(name, Defaults.FIELD_TYPE);
this.builder = this;
super(name);
}

public Builder nullValue(Boolean nullValue) {
this.nullValue = nullValue;
return builder;
@Override
protected List<Parameter<?>> getParameters() {
return List.of(meta, boost, docValues, indexed, nullValue, stored);
}

@Override
public BooleanFieldMapper build(BuilderContext context) {
return new BooleanFieldMapper(name, fieldType,
new BooleanFieldType(buildFullName(context), indexed, hasDocValues, meta),
multiFieldsBuilder.build(this, context), copyTo, nullValue);
return new BooleanFieldMapper(name,
new BooleanFieldType(buildFullName(context), indexed.getValue(), docValues.getValue(), meta.getValue()),
multiFieldsBuilder.build(this, context), copyTo.build(), this);
}
}

Expand All @@ -98,19 +107,7 @@ public static class TypeParser implements Mapper.TypeParser {
public BooleanFieldMapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext)
throws MapperParsingException {
BooleanFieldMapper.Builder builder = new BooleanFieldMapper.Builder(name);
parseField(builder, name, node, parserContext);
for (Iterator<Map.Entry<String, Object>> iterator = node.entrySet().iterator(); iterator.hasNext();) {
Map.Entry<String, Object> entry = iterator.next();
String propName = entry.getKey();
Object propNode = entry.getValue();
if (propName.equals("null_value")) {
if (propNode == null) {
throw new MapperParsingException("Property [null_value] cannot be null.");
}
builder.nullValue(XContentMapValues.nodeBooleanValue(propNode, name + ".null_value"));
iterator.remove();
}
}
builder.parse(name, parserContext, node);
return builder;
}
}
Expand Down Expand Up @@ -217,11 +214,13 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower
}

private final Boolean nullValue;
private final boolean stored;

protected BooleanFieldMapper(String simpleName, FieldType fieldType, MappedFieldType mappedFieldType,
MultiFields multiFields, CopyTo copyTo, Boolean nullValue) {
super(simpleName, fieldType, mappedFieldType, multiFields, copyTo);
this.nullValue = nullValue;
protected BooleanFieldMapper(String simpleName, MappedFieldType mappedFieldType,
MultiFields multiFields, CopyTo copyTo, Builder builder) {
super(simpleName, mappedFieldType, multiFields, copyTo);
this.nullValue = builder.nullValue.getValue();
this.stored = builder.stored.getValue();
}

@Override
Expand All @@ -231,7 +230,7 @@ public BooleanFieldType fieldType() {

@Override
protected void parseCreateField(ParseContext context) throws IOException {
if (fieldType().isSearchable() == false && !fieldType.stored() && !fieldType().hasDocValues()) {
if (fieldType().isSearchable() == false && stored == false && !fieldType().hasDocValues()) {
return;
}

Expand All @@ -250,8 +249,11 @@ protected void parseCreateField(ParseContext context) throws IOException {
if (value == null) {
return;
}
if (fieldType().isSearchable() || fieldType.stored()) {
context.doc().add(new Field(fieldType().name(), value ? "T" : "F", fieldType));
if (fieldType().isSearchable()) {
context.doc().add(new Field(fieldType().name(), value ? "T" : "F", Defaults.FIELD_TYPE));
}
if (stored) {
context.doc().add(new StoredField(fieldType().name(), value ? "T" : "F"));
Copy link
Member

Choose a reason for hiding this comment

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

seems like if the field is searchable and stored too, we end up adding two fields, while before we would only add one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a field with a FieldType that has stored set to true and adding a separate StoredField amount to the same thing. This way, we don't need to change the field type at all, and it's clearer what's happening at the point where we're adding documents.

Copy link
Member

Choose a reason for hiding this comment

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

It was odd that we were using Field to get stored fields but doc values. Field just does a bunch of stuff.

}
if (fieldType().hasDocValues()) {
context.doc().add(new SortedNumericDocValuesField(fieldType().name(), value ? 1 : 0));
Expand All @@ -261,20 +263,13 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected void mergeOptions(FieldMapper other, List<String> conflicts) {
// TODO ban updating null values
public ParametrizedFieldMapper.Builder getMergeBuilder() {
return new Builder(simpleName()).init(this);
}

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

@Override
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);
if (includeDefaults || nullValue != null) {
builder.field("null_value", nullValue);
}
}
}
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserCont
} else if (Fields.CONTEXTS.match(fieldName, LoggingDeprecationHandler.INSTANCE)) {
builder.contextMappings(ContextMappings.load(fieldNode, parserContext.indexVersionCreated()));
iterator.remove();
} else if (parseMultiField(builder, name, parserContext, fieldName, fieldNode)) {
} else if (parseMultiField(builder::addMultiField, name, parserContext, fieldName, fieldNode)) {
iterator.remove();
}
}
Expand Down
7 changes: 7 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ public ContentPath(int offset) {
this.index = 0;
}

public ContentPath(String path) {
this.sb = new StringBuilder();
this.offset = 0;
this.index = 0;
add(path);
}

public void add(String name) {
path[index++] = name;
if (index == path.length) { // expand if needed
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserCont
} else if (propName.equals("format")) {
builder.format(propNode.toString());
iterator.remove();
} else if (TypeParsers.parseMultiField(builder, name, parserContext, propName, propNode)) {
} else if (TypeParsers.parseMultiField(builder::addMultiField, name, parserContext, propName, propNode)) {
iterator.remove();
}
}
Expand Down
Loading