Skip to content

Respect null_value parameter in the fields retrieval API. #58623

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 1 commit into from
Jun 30, 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 @@ -378,6 +378,11 @@ protected ScaledFloatFieldMapper clone() {
return (ScaledFloatFieldMapper) super.clone();
}

@Override
protected Double nullValue() {
return nullValue;
}

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

Expand Down Expand Up @@ -502,7 +507,16 @@ protected Double parseSourceValue(Object value, String format) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}

double doubleValue = objectToDouble(value);
double doubleValue;
if (value.equals("")) {
if (nullValue == null) {
return null;
}
doubleValue = nullValue;
} else {
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 @@ -32,6 +32,7 @@
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.mapper.MapperService.MergeReason;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.search.lookup.SourceLookup;
import org.elasticsearch.test.InternalSettingsPlugin;
import org.junit.Before;

Expand Down Expand Up @@ -405,11 +406,22 @@ public void testMeta() throws Exception {
public void testParseSourceValue() {
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());

ScaledFloatFieldMapper mapper = new ScaledFloatFieldMapper.Builder("field")
.scalingFactor(100)
.build(context);

assertEquals(3.14, mapper.parseSourceValue(3.1415926, null), 0.00001);
assertEquals(3.14, mapper.parseSourceValue("3.1415", null), 0.00001);
assertNull(mapper.parseSourceValue("", null));

ScaledFloatFieldMapper nullValueMapper = new ScaledFloatFieldMapper.Builder("field")
.scalingFactor(100)
.nullValue(2.71)
.build(context);
assertEquals(2.71, nullValueMapper.parseSourceValue("", null), 0.00001);

SourceLookup sourceLookup = new SourceLookup();
sourceLookup.setSource(Collections.singletonMap("field", null));
assertEquals(List.of(2.71), nullValueMapper.lookupValues(sourceLookup, null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,11 @@ protected String contentType() {
return CONTENT_TYPE;
}

@Override
protected String nullValue() {
return nullValue;
}

@Override
protected void mergeOptions(FieldMapper other, List<String> conflicts) {
ICUCollationKeywordFieldMapper icuMergeWith = (ICUCollationKeywordFieldMapper) other;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,15 @@
import org.elasticsearch.index.mapper.MapperService.MergeReason;
import org.elasticsearch.plugin.analysis.icu.AnalysisICUPlugin;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.search.lookup.SourceLookup;
import org.elasticsearch.test.InternalSettingsPlugin;
import org.junit.Before;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Set;

import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -489,9 +492,8 @@ public void testUpdateIgnoreAbove() throws IOException {
public void testParseSourceValue() {
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
ICUCollationKeywordFieldMapper mapper = new ICUCollationKeywordFieldMapper.Builder("field").build(context);

assertEquals("value", mapper.parseSourceValue("value", null));
ICUCollationKeywordFieldMapper mapper = new ICUCollationKeywordFieldMapper.Builder("field").build(context);
assertEquals("42", mapper.parseSourceValue(42L, null));
assertEquals("true", mapper.parseSourceValue(true, null));

Expand All @@ -501,5 +503,12 @@ public void testParseSourceValue() {
assertNull(ignoreAboveMapper.parseSourceValue("value", null));
assertEquals("42", ignoreAboveMapper.parseSourceValue(42L, null));
assertEquals("true", ignoreAboveMapper.parseSourceValue(true, null));

ICUCollationKeywordFieldMapper nullValueMapper = new ICUCollationKeywordFieldMapper.Builder("field")
.nullValue("NULL")
.build(context);
SourceLookup sourceLookup = new SourceLookup();
sourceLookup.setSource(Collections.singletonMap("field", null));
assertEquals(List.of("NULL"), nullValueMapper.lookupValues(sourceLookup, null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@ private static void extractRawValues(List values, List<Object> part, String[] pa
}
}

/**
* For the provided path, return its value in the xContent map.
*
* Note that in contrast with {@link XContentMapValues#extractRawValues}, array and object values
* can be returned.
*
* @param path the value's path in the map.
*
* @return the value associated with the path in the map or 'null' if the path does not exist.
*/
public static Object extractValue(String path, Map<?, ?> map) {
return extractValue(map, path.split("\\."));
}
Expand All @@ -105,19 +115,51 @@ public static Object extractValue(Map<?, ?> map, String... pathElements) {
if (pathElements.length == 0) {
return null;
}
return extractValue(pathElements, 0, map);
return XContentMapValues.extractValue(pathElements, 0, map, null);
}

@SuppressWarnings({"unchecked"})
Copy link
Member

Choose a reason for hiding this comment

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

👍

private static Object extractValue(String[] pathElements, int index, Object currentValue) {
if (index == pathElements.length) {
return currentValue;
}
if (currentValue == null) {
/**
* For the provided path, return its value in the xContent map.
*
* Note that in contrast with {@link XContentMapValues#extractRawValues}, array and object values
* can be returned.
*
* @param path the value's path in the map.
* @param nullValue a value to return if the path exists, but the value is 'null'. This helps
* in distinguishing between a path that doesn't exist vs. a value of 'null'.
*
* @return the value associated with the path in the map or 'null' if the path does not exist.
*/
public static Object extractValue(String path, Map<?, ?> map, Object nullValue) {
String[] pathElements = path.split("\\.");
if (pathElements.length == 0) {
return null;
}
return extractValue(pathElements, 0, map, nullValue);
}

private static Object extractValue(String[] pathElements,
int index,
Object currentValue,
Object nullValue) {
if (currentValue instanceof List) {
List<?> valueList = (List<?>) currentValue;
List<Object> newList = new ArrayList<>(valueList.size());
for (Object o : valueList) {
Object listValue = extractValue(pathElements, index, o, nullValue);
if (listValue != null) {
newList.add(listValue);
}
}
return newList;
}

if (index == pathElements.length) {
return currentValue != null ? currentValue : nullValue;
}

if (currentValue instanceof Map) {
Map map = (Map) currentValue;
Map<?, ?> map = (Map<?, ?>) currentValue;
String key = pathElements[index];
Object mapValue = map.get(key);
int nextIndex = index + 1;
Expand All @@ -126,18 +168,12 @@ private static Object extractValue(String[] pathElements, int index, Object curr
mapValue = map.get(key);
nextIndex++;
}
return extractValue(pathElements, nextIndex, mapValue);
}
if (currentValue instanceof List) {
List valueList = (List) currentValue;
List newList = new ArrayList(valueList.size());
for (Object o : valueList) {
Object listValue = extractValue(pathElements, index, o);
if (listValue != null) {
newList.add(listValue);
}

if (map.containsKey(key) == false) {
return null;
}
return newList;

return extractValue(pathElements, nextIndex, mapValue, nullValue);
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,11 @@ protected String contentType() {
return CONTENT_TYPE;
}

@Override
protected Object nullValue() {
return nullValue;
}

@Override
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
super.doXContentBody(builder, includeDefaults, params);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,11 @@ protected DateFieldMapper clone() {
return (DateFieldMapper) super.clone();
}

@Override
protected String nullValue() {
return nullValueAsString;
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
String dateAsString;
Expand Down Expand Up @@ -631,8 +636,8 @@ protected void parseCreateField(ParseContext context) throws IOException {
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);

ZonedDateTime dateTime = fieldType().resolution().toInstant(timestamp).atZone(ZoneOffset.UTC);
DateFormatter dateTimeFormatter = fieldType().dateTimeFormatter();
if (format != null) {
dateTimeFormatter = DateFormatter.forPattern(format).withLocale(dateTimeFormatter.locale());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,13 @@ public CopyTo copyTo() {
return copyTo;
}

/**
* A value to use in place of a {@code null} value in the document source.
*/
protected Object nullValue() {
Copy link
Member

Choose a reason for hiding this comment

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

How bad would it be to make this abstract so implemeneters need to think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, null_value isn't a cross-cutting feature: only 9 out of ~40 field mappers implement it. In many cases it doesn't make that much sense (or could be harmful?), like setting a null_value for a geopoint. So I'm inclined to avoid making it abstract and encouraging mappers to implement it.

return null;
}

/**
* Whether this mapper can handle an array value during document parsing. If true,
* when an array is encountered during parsing, the document parser will pass the
Expand Down Expand Up @@ -285,7 +292,7 @@ public void parse(ParseContext context) throws IOException {
* @return a list a standardized field values.
*/
public List<?> lookupValues(SourceLookup lookup, @Nullable String format) {
Object sourceValue = lookup.extractValue(name());
Object sourceValue = lookup.extractValue(name(), nullValue());
if (sourceValue == null) {
return List.of();
}
Expand Down Expand Up @@ -337,6 +344,7 @@ protected FieldMapper clone() {
}
}


@Override
public final FieldMapper merge(Mapper mergeWith) {
FieldMapper merged = clone();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ protected String contentType() {
return fieldType().typeName();
}

@Override
protected Object nullValue() {
return nullValue;
}

@Override
protected IpFieldMapper clone() {
return (IpFieldMapper) super.clone();
Expand Down Expand Up @@ -415,7 +420,12 @@ protected String parseSourceValue(Object value, String format) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}

InetAddress address = InetAddresses.forString(value.toString());
InetAddress address;
if (value instanceof InetAddress) {
address = (InetAddress) value;
} else {
address = InetAddresses.forString(value.toString());
}
return InetAddresses.toAddrString(address);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,11 @@ protected String contentType() {
return CONTENT_TYPE;
}

@Override
protected String nullValue() {
return nullValue;
}

@Override
protected void mergeOptions(FieldMapper other, List<String> conflicts) {
KeywordFieldMapper k = (KeywordFieldMapper) other;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,11 @@ protected NumberFieldMapper clone() {
return (NumberFieldMapper) super.clone();
}

@Override
protected Number nullValue() {
return nullValue;
}

@Override
protected void parseCreateField(ParseContext context) throws IOException {
XContentParser parser = context.parser();
Expand Down Expand Up @@ -1106,6 +1111,11 @@ protected Number parseSourceValue(Object value, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}

if (value.equals("")) {
return nullValue;
}

return fieldType().type.parse(value, coerce.value());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.apache.lucene.index.LeafReader;
import org.apache.lucene.index.LeafReaderContext;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.xcontent.XContentHelper;
Expand Down Expand Up @@ -133,11 +134,19 @@ public List<Object> extractRawValues(String path) {
}

/**
* For the provided path, return its value in the source. Note that in contrast with
* {@link SourceLookup#extractRawValues}, array and object values can be returned.
* For the provided path, return its value in the source.
*
* Note that in contrast with {@link SourceLookup#extractRawValues}, array and object values
* can be returned.
*
* @param path the value's path in the source.
* @param nullValue a value to return if the path exists, but the value is 'null'. This helps
* in distinguishing between a path that doesn't exist vs. a value of 'null'.
*
* @return the value associated with the path in the source or 'null' if the path does not exist.
*/
public Object extractValue(String path) {
return XContentMapValues.extractValue(path, loadSourceIfNeeded());
public Object extractValue(String path, @Nullable Object nullValue) {
return XContentMapValues.extractValue(path, loadSourceIfNeeded(), nullValue);
}

public Object filter(FetchSourceContext context) {
Expand Down
Loading