Removes toString->parse roundtrip for missing value in terms agg (ela…

…stic#67954) Removes unnecessary and confusing roundtrip that missing values are undertaking in the terms agg if they are already specified as numbers. It also improves a somewhat confusing error message when the missing value cannot be parsed according to the specified format. Closes elastic#67197
original-brownbear · Jan 26, 2021 · 03d4ba5 · 03d4ba5
1 parent 1143769
commit 03d4ba5
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 3 deletions.
diff --git a/server/src/main/java/org/elasticsearch/search/DocValueFormat.java b/server/src/main/java/org/elasticsearch/search/DocValueFormat.java
@@ -433,7 +433,7 @@ public long parseLong(String value, boolean roundUp, LongSupplier now) {
             try {
                 n = format.parse(value);
             } catch (ParseException e) {
-                throw new RuntimeException(e);
+                throw new RuntimeException("Cannot parse the value [" + value + "] using the pattern [" + pattern + "]", e);
             }
             if (format.isParseIntegerOnly()) {
                 return n.longValue();
@@ -454,7 +454,7 @@ public double parseDouble(String value, boolean roundUp, LongSupplier now) {
             try {
                 n = format.parse(value);
             } catch (ParseException e) {
-                throw new RuntimeException(e);
+                throw new RuntimeException("Cannot parse the value [" + value + "] using the pattern [" + pattern + "]", e);
             }
             return n.doubleValue();
         }

diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java
@@ -85,7 +85,12 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa
         @Override
         public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat,
                                            AggregationContext context) {
-            Number missing = docValueFormat.parseDouble(rawMissing.toString(), false, context::nowInMillis);
+            Number missing;
+            if (rawMissing instanceof Number) {
+                missing = (Number) rawMissing;
+            } else {
+                missing = docValueFormat.parseDouble(rawMissing.toString(), false, context::nowInMillis);
+            }
             return MissingValues.replaceMissing((ValuesSource.Numeric) valuesSource, missing);
         }
 

diff --git a/...rc/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java b/...rc/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java
@@ -1421,6 +1421,40 @@ public void testOrderByPipelineAggregation() throws Exception {
         }
     }
 
+    public void testFormatWithMissing() throws IOException {
+        MappedFieldType fieldType
+            = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.INTEGER);
+
+        TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("name")
+            .field("number")
+            .format("$###.00")
+            .missing(randomFrom(42, "$42", 42.0));
+
+        testCase(aggregationBuilder, new MatchAllDocsQuery(), iw -> {
+            final int numDocs = 10;
+            iw.addDocument(singleton(new NumericDocValuesField("not_number", 0)));
+            for (int i = 1; i < numDocs; i++) {
+                iw.addDocument(singleton(new NumericDocValuesField("number", i + 1)));
+            }
+        }, (Consumer<InternalTerms<?, ?>>) terms -> assertTrue(AggregationInspectionHelper.hasValue(terms)), fieldType);
+    }
+
+    public void testFormatCannotParseMissing() throws IOException {
+        MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.INTEGER);
+
+        TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("name").field("number").format("$###.00").missing("42");
+
+        RuntimeException ex = expectThrows(RuntimeException.class, () -> testCase(aggregationBuilder, new MatchAllDocsQuery(), iw -> {
+            final int numDocs = 10;
+            iw.addDocument(singleton(new NumericDocValuesField("not_number", 0)));
+            for (int i = 1; i < numDocs; i++) {
+                iw.addDocument(singleton(new NumericDocValuesField("number", i + 1)));
+            }
+        }, (Consumer<InternalTerms<?, ?>>) terms -> fail("Should have thrown"), fieldType));
+
+        assertThat(ex.getMessage(), equalTo("Cannot parse the value [42] using the pattern [$###.00]"));
+    }
+
     public void testOrderByCardinality() throws IOException {
         boolean bIsString = randomBoolean();
         TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("a").field("a")