Return null_value when the source contains a 'null' for the field. (#…

…58623) This PR adds a version of `XContentMapValues.extractValue` that accepts a default value to return in place of 'null'. It then uses this method when looking up source values to return the configured `null_value` instead of 'null' when retrieving fields.
elastic · Jul 15, 2020 · 4616cc7 · 4616cc7
1 parent 267d819
commit 4616cc7
Show file tree

Hide file tree

Showing 22 changed files with 301 additions and 33 deletions.
diff --git a/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java b/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java
@@ -355,6 +355,11 @@ protected ScaledFloatFieldMapper clone() {
         return (ScaledFloatFieldMapper) super.clone();
     }
 
+    @Override
+    protected Double nullValue() {
+        return nullValue;
+    }
+
     @Override
     protected void parseCreateField(ParseContext context) throws IOException {
 
@@ -479,7 +484,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;
     }

diff --git a/...pper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapperTests.java b/...pper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapperTests.java
@@ -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;
 
@@ -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));
     }
 }
diff --git a/...ysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java b/...ysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java
@@ -577,6 +577,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;

diff --git a/...icu/src/test/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapperTests.java b/...icu/src/test/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapperTests.java
@@ -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;
@@ -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));
 
@@ -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));
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java b/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java
@@ -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("\\."));
     }
@@ -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"})
-    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;
@@ -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;
     }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java
@@ -281,4 +281,8 @@ protected String contentType() {
         return CONTENT_TYPE;
     }
 
+    @Override
+    protected Object nullValue() {
+        return nullValue;
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java
@@ -538,6 +538,11 @@ protected DateFieldMapper clone() {
         return (DateFieldMapper) super.clone();
     }
 
+    @Override
+    protected String nullValue() {
+        return nullValueAsString;
+    }
+
     @Override
     protected void parseCreateField(ParseContext context) throws IOException {
         String dateAsString;
@@ -588,8 +593,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());

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java
@@ -224,6 +224,13 @@ public CopyTo copyTo() {
         return copyTo;
     }
 
+    /**
+     * A value to use in place of a {@code null} value in the document source.
+     */
+    protected Object nullValue() {
+        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
@@ -286,7 +293,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();
         }
@@ -338,6 +345,7 @@ protected FieldMapper clone() {
         }
     }
 
+
     @Override
     public FieldMapper merge(Mapper mergeWith) {
         FieldMapper merged = clone();

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java
@@ -348,6 +348,11 @@ protected String contentType() {
         return fieldType().typeName();
     }
 
+    @Override
+    protected Object nullValue() {
+        return nullValue;
+    }
+
     @Override
     protected IpFieldMapper clone() {
         return (IpFieldMapper) super.clone();
@@ -406,7 +411,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);
     }
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
@@ -416,6 +416,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;

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
@@ -1036,6 +1036,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();
@@ -1090,6 +1095,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());
     }
 

diff --git a/server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java b/server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java
@@ -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;
@@ -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) {