Clean up top_metric's internal API (backport of #71738) (#71862)

This changes `top_metrics`'s implementation of `getProperty` and `value` to be a bit more consisntent which shold make it easier to integrate into transform. We expect `value` to return `NaN` when the value isn't a number or is null but was throwing exceptions in both that cases. `getProperty` was throwing exceptions on null and non-numeric values as well. Now it returns `null` or `BytesRef`.
elastic · Apr 20, 2021 · c4019b8 · c4019b8
1 parent 6093c3c
commit c4019b8
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 13 deletions.
diff --git a/server/src/main/java/org/elasticsearch/search/sort/SortValue.java b/server/src/main/java/org/elasticsearch/search/sort/SortValue.java
@@ -118,7 +118,9 @@ public final XContentBuilder toXContent(XContentBuilder builder, DocValueFormat
     public abstract String toString();
 
     /**
-     * Return this {@linkplain SortValue} as a boxed {@linkplain Number}.
+     * Return this {@linkplain SortValue} as a boxed {@linkplain Number}
+     * or {@link Double#NaN} if it isn't a number. Or if it is actually
+     * {@link Double#NaN}.
      */
     public abstract Number numberValue();
 
@@ -332,7 +334,7 @@ public String toString() {
 
         @Override
         public Number numberValue() {
-            throw new UnsupportedOperationException();
+            return Double.NaN;
         }
     }
 }
diff --git a/...lytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java b/...lytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java
@@ -95,9 +95,10 @@ public Object getProperty(List<String> path) {
         assert topMetrics.size() == 1 : "property paths should only resolve against top metrics with size == 1.";
         MetricValue metric = topMetrics.get(0).metricValues.get(index);
         if (metric == null) {
+            // We've found the name but it doesn't have a value.
             return Double.NaN;
         }
-        return metric.numberValue();
+        return metric.getValue().getKey();
     }
 
     @Override
@@ -171,7 +172,11 @@ public double value(String name) {
         }
         assert topMetrics.size() == 1 : "property paths should only resolve against top metrics with size == 1.";
         // TODO it'd probably be nicer to have "compareTo" instead of assuming a double.
-        return topMetrics.get(0).metricValues.get(index).numberValue().doubleValue();
+        MetricValue value = topMetrics.get(0).metricValues.get(index);
+        if (value == null) {
+            return Double.NaN;
+        }
+        return value.numberValue().doubleValue();
     }
 
     @Override

diff --git a/...s/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetricsTests.java b/...s/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetricsTests.java
@@ -7,6 +7,7 @@
 
 package org.elasticsearch.xpack.analytics.topmetrics;
 
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.client.analytics.ParsedTopMetrics;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.Strings;
@@ -218,27 +219,41 @@ public void testToXContentManyTopMetrics() throws IOException {
     }
 
     public void testGetProperty() {
-        InternalTopMetrics metrics = new InternalTopMetrics(
+        InternalTopMetrics metrics = resultWithAllTypes();
+        assertThat(metrics.getProperty("int"), equalTo(1L));
+        assertThat(metrics.getProperty("double"), equalTo(5.0));
+        assertThat(metrics.getProperty("bytes"), equalTo(new BytesRef("cat")));
+        assertThat((Double) metrics.getProperty("null"), notANumber());
+    }
+
+    public void testValue() {
+        InternalTopMetrics metrics = resultWithAllTypes();
+        assertThat(metrics.value("int"), equalTo(1.0));
+        assertThat(metrics.value("double"), equalTo(5.0));
+        assertThat(metrics.value("bytes"), notANumber());
+        assertThat(metrics.value("null"), notANumber());
+    }
+
+    private InternalTopMetrics resultWithAllTypes() {
+        return new InternalTopMetrics(
             "test",
             SortOrder.ASC,
-            Arrays.asList("foo", "bar", "baz"),
+            org.elasticsearch.common.collect.List.of("int", "double", "bytes", "null"),
             1,
-            Arrays.asList(
+            org.elasticsearch.common.collect.List.of(
                 new InternalTopMetrics.TopMetric(
                     DocValueFormat.RAW,
                     SortValue.from(1),
                     Arrays.asList(
-                        new MetricValue(DocValueFormat.RAW, SortValue.from(1)),   // foo
-                        new MetricValue(DocValueFormat.RAW, SortValue.from(5.0)), // bar
-                        null                                                      // baz
+                        new MetricValue(DocValueFormat.RAW, SortValue.from(1)),   // int
+                        new MetricValue(DocValueFormat.RAW, SortValue.from(5.0)), // double
+                        new MetricValue(DocValueFormat.RAW, SortValue.from(new BytesRef("cat"))), // str
+                        null                                                      // null
                     )
                 )
             ),
             null
         );
-        assertThat(metrics.getProperty("foo"), equalTo(1L));
-        assertThat(metrics.getProperty("bar"), equalTo(5.0));
-        assertThat((Double) metrics.getProperty("baz"), notANumber());
     }
 
     @Override