From c4019b85739a363066bf77d4f5a09ca23395cecc Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 20 Apr 2021 09:18:40 -0400 Subject: [PATCH] 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`. --- .../elasticsearch/search/sort/SortValue.java | 6 ++-- .../topmetrics/InternalTopMetrics.java | 9 +++-- .../topmetrics/InternalTopMetricsTests.java | 33 ++++++++++++++----- 3 files changed, 35 insertions(+), 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 index 87c2f268514f6..890c61e9bd55e 100644 --- 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/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java index 2a870e8d72123..49fd8984fcf30 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java @@ -95,9 +95,10 @@ public Object getProperty(List 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/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetricsTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetricsTests.java index ef3b535b1bad9..f5d51d6b8d0db 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetricsTests.java +++ b/x-pack/plugin/analytics/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