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.

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 support for a 'format' option in fields retrieval. #57855

Merged
merged 4 commits into from
Jun 9, 2020
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -214,7 +214,10 @@ private Float objectToFloat(Object value) {
}

@Override
protected Float parseSourceValue(Object value) {
protected Float parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return objectToFloat(value);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,10 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,11 @@ private static double objectToDouble(Object value) {
}

@Override
protected Double parseSourceValue(Object value) {
protected Double parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}

double doubleValue = objectToDouble(value);
double scalingFactor = fieldType().getScalingFactor();
return Math.round(doubleValue * scalingFactor) / scalingFactor;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ protected void parseCreateField(ParseContext context) {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
throw new UnsupportedOperationException();
}

Expand Down Expand Up @@ -516,7 +516,7 @@ protected void mergeOptions(FieldMapper other, List<String> conflicts) {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
throw new UnsupportedOperationException();
}

Expand Down Expand Up @@ -676,7 +676,10 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected String parseSourceValue(Object value) {
protected String parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value.toString();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,11 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected String parseSourceValue(Object value) {
protected String parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}

return value.toString();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public void testParseSourceValue() {
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
RankFeatureFieldMapper mapper = new RankFeatureFieldMapper.Builder("field").build(context);

assertEquals(3.14f, mapper.parseSourceValue(3.14), 0.0001);
assertEquals(42.9f, mapper.parseSourceValue("42.9"), 0.0001);
assertEquals(3.14f, mapper.parseSourceValue(3.14, null), 0.0001);
assertEquals(42.9f, mapper.parseSourceValue("42.9", null), 0.0001);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ public void testParseSourceValue() {
.scalingFactor(100)
.build(context);

assertEquals(3.14, mapper.parseSourceValue(3.1415926), 0.00001);
assertEquals(3.14, mapper.parseSourceValue("3.1415"), 0.00001);
assertEquals(3.14, mapper.parseSourceValue(3.1415926, null), 0.00001);
assertEquals(3.14, mapper.parseSourceValue("3.1415", null), 0.00001);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
throw new UnsupportedOperationException("The " + typeName() + " field is not stored in _source.");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
throw new UnsupportedOperationException("The " + typeName() + " field is not stored in _source.");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,10 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,10 @@ public void parse(ParseContext context) throws IOException {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,10 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected String parseSourceValue(Object value) {
protected String parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -485,8 +485,8 @@ public void testParseSourceValue() {
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
ICUCollationKeywordFieldMapper mapper = new ICUCollationKeywordFieldMapper.Builder("field").build(context);

assertEquals("value", mapper.parseSourceValue("value"));
assertEquals("42", mapper.parseSourceValue(42L));
assertEquals("true", mapper.parseSourceValue(true));
assertEquals("value", mapper.parseSourceValue("value", null));
assertEquals("42", mapper.parseSourceValue(42L, null));
assertEquals("true", mapper.parseSourceValue(true, null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,10 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected String parseSourceValue(Object value) {
protected String parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value.toString();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,8 +685,8 @@ public void testParseSourceValue() {
.searchQuoteAnalyzer(indexService.getIndexAnalyzers().getDefaultSearchQuoteAnalyzer())
.build(context);

assertEquals("value", mapper.parseSourceValue("value"));
assertEquals("42", mapper.parseSourceValue(42L));
assertEquals("true", mapper.parseSourceValue(true));
assertEquals("value", mapper.parseSourceValue("value", null));
assertEquals("42", mapper.parseSourceValue(42L, null));
assertEquals("true", mapper.parseSourceValue(true, null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,10 @@ protected void parseCreateField(ParseContext context)
}

@Override
protected String parseSourceValue(Object value) {
protected String parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value.toString();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,53 @@ setup:
- match: { hits.hits.0.fields.integer_range.0.lte: 42 }

---
"Test date formatting":
- do:
indices.create:
index: test
body:
settings:
index.number_of_shards: 1
mappings:
properties:
keyword:
type: keyword
date:
type: date

- do:
index:
index: test
id: 1
body:
keyword: "value"
date: "1990-12-29T22:30:00.000Z"

- do:
indices.refresh:
index: [ test ]

- do:
search:
index: test
body:
fields:
- field: date
format: "yyyy/MM/dd"

- is_true: hits.hits.0._id
- is_true: hits.hits.0._source
- match: { hits.hits.0.fields.date.0: "1990/12/29" }

- do:
catch: bad_request
search:
index: test
body:
fields:
- field: keyword
format: "yyyy/MM/dd"
---
"Test disable source":
- do:
indices.create:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,24 @@ public SearchRequestBuilder addDocValueField(String name) {
return addDocValueField(name, null);
}

/**
* Adds a field to load and return. The field must be present in the document _source.
*
* @param name The field to load
*/
public SearchRequestBuilder addFetchField(String name) {
sourceBuilder().fetchField(name);
sourceBuilder().fetchField(name, null);
return this;
}

/**
* Adds a field to load and return. The field must be present in the document _source.
*
* @param name The field to load
* @param format TODO(jtibs): fill this in
*/
public SearchRequestBuilder addFetchField(String name, String format) {
sourceBuilder().fetchField(name, format);
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public FT fieldType() {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
throw new UnsupportedOperationException();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,10 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
protected Object parseSourceValue(Object value) {
protected Object parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
return value;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,11 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public Boolean parseSourceValue(Object value) {
public Boolean parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}

if (value instanceof Boolean) {
return (Boolean) value;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,11 @@ private void parse(ParseContext parseContext, Token token,
}

@Override
protected List<?> parseSourceValue(Object value) {
protected List<?> parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}

if (value instanceof List) {
return (List<?>) value;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -628,12 +628,16 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public String parseSourceValue(Object value) {
public String parseSourceValue(Object value, String format) {
String date = value.toString();
long timestamp = fieldType().parse(date);

ZonedDateTime dateTime = fieldType().resolution().toInstant(timestamp).atZone(ZoneOffset.UTC);
return fieldType().dateTimeFormatter().format(dateTime);

DateFormatter dateTimeFormatter = fieldType().dateTimeFormatter();
if (format != null) {
dateTimeFormatter = DateFormatter.forPattern(format).withLocale(dateTimeFormatter.locale());
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid this one day, but for now it is cool. And, I think it'd be a bit overboard to try and avoid it now. If I did want to avoid it maybe something like:

public Function<Object, String> sourceParser(String format);

I don't know. Just thinking out loud. Not needed now.

Copy link
Contributor Author

@jtibshirani jtibshirani Jun 9, 2020

Choose a reason for hiding this comment

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

I agree, instead of a function we could even introduce an object like SourceValueParser. This could also help clean up some of the fragility around the dual lookupValues and parseSourceValue methods. It did feel like overkill to do this refactor now, though.

}
return dateTimeFormatter.format(dateTime);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.lucene.document.FieldType;
import org.apache.lucene.index.IndexOptions;
import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.lucene.Lucene;
import org.elasticsearch.common.settings.Setting;
Expand Down Expand Up @@ -332,21 +333,22 @@ public void parse(ParseContext context) throws IOException {
* Some mappers may need more flexibility and can override this entire method instead.
*
* @param lookup a lookup structure over the document's source.
* @param format an optional format string used when formatting values, for example a date format.
* @return a list a standardized field values.
*/
public List<?> lookupValues(SourceLookup lookup) {
public List<?> lookupValues(SourceLookup lookup, @Nullable String format) {
Object sourceValue = lookup.extractValue(name());
if (sourceValue == null) {
return List.of();
}

List<Object> values = new ArrayList<>();
if (parsesArrayValue()) {
return (List<?>) parseSourceValue(sourceValue);
return (List<?>) parseSourceValue(sourceValue, format);
} else {
List<?> sourceValues = sourceValue instanceof List ? (List<?>) sourceValue : List.of(sourceValue);
for (Object value : sourceValues) {
Object parsedValue = parseSourceValue(value);
Object parsedValue = parseSourceValue(value, format);
values.add(parsedValue);
}
}
Expand All @@ -360,7 +362,7 @@ public List<?> lookupValues(SourceLookup lookup) {
*
* Note that when overriding this method, {@link #lookupValues} should *not* be overridden.
*/
protected abstract Object parseSourceValue(Object value);
protected abstract Object parseSourceValue(Object value, @Nullable String format);

protected void createFieldNamesField(ParseContext context) {
FieldNamesFieldType fieldNamesFieldType = context.docMapper().metadataMapper(FieldNamesFieldMapper.class).fieldType();
Expand Down
Loading