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

Format support for script doc fields #60465

Merged
merged 3 commits into from
Aug 3, 2020
Merged

Format support for script doc fields #60465

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ef23c6
format support for script doc fields
nik9000 Jul 30, 2020
b1c02f6
Change error message
nik9000 Jul 31, 2020
8f40cb4
Add duel tests
nik9000 Jul 31, 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
18 changes: 18 additions & 0 deletions ...main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.io.IOException;
import java.time.ZoneId;
import java.util.List;
import java.util.Locale;
import java.util.Map;

import static org.elasticsearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES;
Expand Down Expand Up @@ -141,4 +142,21 @@ protected final void checkAllowExpensiveQueries(QueryShardContext context) {
);
}
}

/**
* The format that this field should use. The default implementation is
* {@code null} because most fields don't support formats.
*/
protected String format() {
return null;
}

/**
* The locale that this field's format should use. The default
* implementation is {@code null} because most fields don't
* support formats.
*/
protected Locale formatLocale() {
return null;
}
}
55 changes: 50 additions & 5 deletions .../src/main/java/org/elasticsearch/xpack/runtimefields/mapper/RuntimeScriptFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@

package org.elasticsearch.xpack.runtimefields.mapper;

import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.util.LocaleUtils;
import org.elasticsearch.index.mapper.DateFieldMapper;
import org.elasticsearch.index.mapper.FieldMapper;
import org.elasticsearch.index.mapper.KeywordFieldMapper;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.Mapper;
import org.elasticsearch.index.mapper.NumberFieldMapper.NumberType;
import org.elasticsearch.index.mapper.ParametrizedFieldMapper;
Expand All @@ -23,6 +24,7 @@
import org.elasticsearch.xpack.runtimefields.StringScriptFieldScript;

import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.BiFunction;

Expand All @@ -43,7 +45,7 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp

protected RuntimeScriptFieldMapper(
String simpleName,
MappedFieldType mappedFieldType,
AbstractScriptMappedFieldType mappedFieldType,
MultiFields multiFields,
CopyTo copyTo,
String runtimeType,
Expand Down Expand Up @@ -78,22 +80,33 @@ protected String contentType() {

public static class Builder extends ParametrizedFieldMapper.Builder {

static final Map<String, BiFunction<Builder, BuilderContext, MappedFieldType>> FIELD_TYPE_RESOLVER = Map.of(
static final Map<String, BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType>> FIELD_TYPE_RESOLVER = Map.of(
DateFieldMapper.CONTENT_TYPE,
(builder, context) -> {
DateScriptFieldScript.Factory factory = builder.scriptCompiler.compile(
builder.script.getValue(),
DateScriptFieldScript.CONTEXT
);
String format = builder.format.getValue();
if (format == null) {
format = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.pattern();
}
Locale locale = builder.locale.getValue();
if (locale == null) {
locale = Locale.ROOT;
}
DateFormatter dateTimeFormatter = DateFormatter.forPattern(format).withLocale(locale);
return new ScriptDateMappedFieldType(
builder.buildFullName(context),
builder.script.getValue(),
factory,
dateTimeFormatter,
builder.meta.getValue()
);
},
NumberType.DOUBLE.typeName(),
(builder, context) -> {
builder.formatAndLocaleNotSupported();
DoubleScriptFieldScript.Factory factory = builder.scriptCompiler.compile(
builder.script.getValue(),
DoubleScriptFieldScript.CONTEXT
Expand All @@ -107,6 +120,7 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
},
KeywordFieldMapper.CONTENT_TYPE,
(builder, context) -> {
builder.formatAndLocaleNotSupported();
StringScriptFieldScript.Factory factory = builder.scriptCompiler.compile(
builder.script.getValue(),
StringScriptFieldScript.CONTEXT
Expand All @@ -120,6 +134,7 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
},
NumberType.LONG.typeName(),
(builder, context) -> {
builder.formatAndLocaleNotSupported();
LongScriptFieldScript.Factory factory = builder.scriptCompiler.compile(
builder.script.getValue(),
LongScriptFieldScript.CONTEXT
Expand Down Expand Up @@ -159,6 +174,27 @@ private static RuntimeScriptFieldMapper toType(FieldMapper in) {
throw new IllegalArgumentException("script must be specified for " + CONTENT_TYPE + " field [" + name + "]");
}
});
private final Parameter<String> format = Parameter.stringParam(
"format",
true,
mapper -> ((AbstractScriptMappedFieldType) mapper.fieldType()).format(),
null
).setSerializer((b, n, v) -> {
if (v != null && false == v.equals(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.pattern())) {
b.field(n, v);
}
}).acceptsNull();
private final Parameter<Locale> locale = new Parameter<>(
"locale",
true,
() -> null,
(n, c, o) -> o == null ? null : LocaleUtils.parse(o.toString()),
mapper -> ((AbstractScriptMappedFieldType) mapper.fieldType()).formatLocale()
).setSerializer((b, n, v) -> {
if (v != null && false == v.equals(Locale.ROOT)) {
b.field(n, v.toString());
}
}).acceptsNull();

private final ScriptCompiler scriptCompiler;

Expand All @@ -169,12 +205,12 @@ protected Builder(String name, ScriptCompiler scriptCompiler) {

@Override
protected List<Parameter<?>> getParameters() {
return List.of(meta, runtimeType, script);
return List.of(meta, runtimeType, script, format, locale);
}

@Override
public RuntimeScriptFieldMapper build(BuilderContext context) {
BiFunction<Builder, BuilderContext, MappedFieldType> fieldTypeResolver = Builder.FIELD_TYPE_RESOLVER.get(
BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType> fieldTypeResolver = Builder.FIELD_TYPE_RESOLVER.get(
runtimeType.getValue()
);
if (fieldTypeResolver == null) {
Expand Down Expand Up @@ -203,6 +239,15 @@ static Script parseScript(String name, Mapper.TypeParser.ParserContext parserCon
}
return script;
}

private void formatAndLocaleNotSupported() {
if (format.getValue() != null) {
throw new IllegalArgumentException("format not supported by runtime_type [" + runtimeType.getValue() + "]");
}
if (locale.getValue() != null) {
throw new IllegalArgumentException("locale not supported by runtime_type [" + runtimeType.getValue() + "]");
}
}
}

@FunctionalInterface
Expand Down
37 changes: 26 additions & 11 deletions ...src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDateMappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,24 @@
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.Supplier;

public class ScriptDateMappedFieldType extends AbstractScriptMappedFieldType {
private final DateScriptFieldScript.Factory scriptFactory;

ScriptDateMappedFieldType(String name, Script script, DateScriptFieldScript.Factory scriptFactory, Map<String, String> meta) {
private final DateFormatter dateTimeFormatter;

ScriptDateMappedFieldType(
String name,
Script script,
DateScriptFieldScript.Factory scriptFactory,
DateFormatter dateTimeFormatter,
Map<String, String> meta
) {
super(name, script, meta);
this.scriptFactory = scriptFactory;
}

private DateFormatter dateTimeFormatter() {
return DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER; // TODO make configurable
this.dateTimeFormatter = dateTimeFormatter;
}

@Override
Expand All @@ -58,12 +63,12 @@ public Object valueForDisplay(Object value) {
if (val == null) {
return null;
}
return dateTimeFormatter().format(Resolution.MILLISECONDS.toInstant(val).atZone(ZoneOffset.UTC));
return dateTimeFormatter.format(Resolution.MILLISECONDS.toInstant(val).atZone(ZoneOffset.UTC));
}

@Override
public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) {
DateFormatter dateTimeFormatter = dateTimeFormatter();
DateFormatter dateTimeFormatter = this.dateTimeFormatter;
if (format != null) {
dateTimeFormatter = DateFormatter.forPattern(format).withLocale(dateTimeFormatter.locale());
}
Expand Down Expand Up @@ -99,7 +104,7 @@ public Query rangeQuery(
@Nullable DateMathParser parser,
QueryShardContext context
) {
parser = parser == null ? DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.toDateMathParser() : parser;
parser = parser == null ? dateTimeFormatter.toDateMathParser() : parser;
checkAllowExpensiveQueries(context);
return DateFieldType.dateRangeQuery(
lowerTerm,
Expand All @@ -121,7 +126,7 @@ public Query termQuery(Object value, QueryShardContext context) {
value,
false,
null,
dateTimeFormatter().toDateMathParser(),
dateTimeFormatter.toDateMathParser(),
now,
DateFieldMapper.Resolution.MILLISECONDS
);
Expand All @@ -143,7 +148,7 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
value,
false,
null,
DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.toDateMathParser(),
dateTimeFormatter.toDateMathParser(),
now,
DateFieldMapper.Resolution.MILLISECONDS
)
Expand All @@ -153,4 +158,14 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
return new LongScriptFieldTermsQuery(script, leafFactory(context.lookup())::newInstance, name(), terms);
});
}

@Override
protected String format() {
return dateTimeFormatter.pattern();
}

@Override
protected Locale formatLocale() {
return dateTimeFormatter.locale();
}
}
53 changes: 53 additions & 0 deletions ...test/java/org/elasticsearch/xpack/runtimefields/mapper/RuntimeScriptFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import org.elasticsearch.action.fieldcaps.FieldCapabilities;
import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse;
import org.elasticsearch.common.CheckedConsumer;
import org.elasticsearch.common.CheckedSupplier;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
Expand All @@ -34,6 +36,7 @@
import java.util.Set;

import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;

public class RuntimeScriptFieldMapperTests extends ESSingleNodeTestCase {
Expand Down Expand Up @@ -145,6 +148,51 @@ public void testDate() throws IOException {
assertEquals(Strings.toString(mapping("date")), Strings.toString(mapperService.documentMapper()));
}

public void testDateWithFormat() throws IOException {
CheckedSupplier<XContentBuilder, IOException> mapping = () -> mapping("date", b -> b.field("format", "yyyy-MM-dd"));
MapperService mapperService = createIndex("test", Settings.EMPTY, mapping.get()).mapperService();
FieldMapper mapper = (FieldMapper) mapperService.documentMapper().mappers().getMapper("field");
assertThat(mapper, instanceOf(RuntimeScriptFieldMapper.class));
assertEquals(Strings.toString(mapping.get()), Strings.toString(mapperService.documentMapper()));
}

public void testDateWithLocale() throws IOException {
CheckedSupplier<XContentBuilder, IOException> mapping = () -> mapping("date", b -> b.field("locale", "en_GB"));
MapperService mapperService = createIndex("test", Settings.EMPTY, mapping.get()).mapperService();
FieldMapper mapper = (FieldMapper) mapperService.documentMapper().mappers().getMapper("field");
assertThat(mapper, instanceOf(RuntimeScriptFieldMapper.class));
assertEquals(Strings.toString(mapping.get()), Strings.toString(mapperService.documentMapper()));
}

public void testDateWithLocaleAndFormat() throws IOException {
CheckedSupplier<XContentBuilder, IOException> mapping = () -> mapping(
"date",
b -> b.field("format", "yyyy-MM-dd").field("locale", "en_GB")
);
MapperService mapperService = createIndex("test", Settings.EMPTY, mapping.get()).mapperService();
FieldMapper mapper = (FieldMapper) mapperService.documentMapper().mappers().getMapper("field");
assertThat(mapper, instanceOf(RuntimeScriptFieldMapper.class));
assertEquals(Strings.toString(mapping.get()), Strings.toString(mapperService.documentMapper()));
}

public void testNonDateWithFormat() throws IOException {
String runtimeType = randomValueOtherThan("date", () -> randomFrom(runtimeTypes));
Exception e = expectThrows(
MapperParsingException.class,
() -> createIndex("test", Settings.EMPTY, mapping(runtimeType, b -> b.field("format", "yyyy-MM-dd")))
);
assertThat(e.getMessage(), equalTo("Failed to parse mapping: format not supported by runtime_type [" + runtimeType + "]"));
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think that looking at this error message, there is a slight change that it makes users think that the specific value they provided for the format parameter is not supported, more than providing any format. same for locale. Maybe we can rephrase it?

}

public void testNonDateWithLocale() throws IOException {
String runtimeType = randomValueOtherThan("date", () -> randomFrom(runtimeTypes));
Exception e = expectThrows(
MapperParsingException.class,
() -> createIndex("test", Settings.EMPTY, mapping(runtimeType, b -> b.field("locale", "en_GB")))
);
assertThat(e.getMessage(), equalTo("Failed to parse mapping: locale not supported by runtime_type [" + runtimeType + "]"));
}

public void testFieldCaps() throws Exception {
for (String runtimeType : runtimeTypes) {
String scriptIndex = "test_" + runtimeType + "_script";
Expand Down Expand Up @@ -178,6 +226,10 @@ public void testFieldCaps() throws Exception {
}

private XContentBuilder mapping(String type) throws IOException {
return mapping(type, builder -> {});
}

private XContentBuilder mapping(String type, CheckedConsumer<XContentBuilder, IOException> extra) throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject();
{
mapping.startObject("_doc");
Expand All @@ -192,6 +244,7 @@ private XContentBuilder mapping(String type) throws IOException {
mapping.field("source", "dummy_source").field("lang", "test");
}
mapping.endObject();
extra.accept(mapping);
}
mapping.endObject();
}
Expand Down
Loading