From 67055877a50e8f758935710926235bdac7455d5b Mon Sep 17 00:00:00 2001 From: David Roberts Date: Tue, 19 Sep 2017 10:30:43 +0100 Subject: [PATCH] [ML] Avoid creating spurious 0 "actual" values for model plot documents (elastic/x-pack-elasticsearch#2535) Some model plot documents should not have an "actual" value, for example when no input events were seen for a meean/min/max detector in a particular bucket. Prior to this change we would set the "actual" value to 0 for such model plot documents. Following this change no "actual" value will be present in these documents. Only newly created model plot documents are affected. Model plot documents that were incorrectly written in the past will remain wrong forever. relates elastic/x-pack-elasticsearch#2528 Original commit: elastic/x-pack-elasticsearch@47a7365f5965f5a8ce24d4b0174a5af985b5c1f4 --- .../xpack/ml/job/results/ModelPlot.java | 33 +++++++++++++++---- .../xpack/ml/job/results/ModelPlotTests.java | 27 +++++++++++++++ 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/results/ModelPlot.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/results/ModelPlot.java index 1a34194de7b..7027c550717 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/results/ModelPlot.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/results/ModelPlot.java @@ -87,7 +87,10 @@ public class ModelPlot implements ToXContentObject, Writeable { private double modelLower; private double modelUpper; private double modelMedian; - private double actual; + /** + * This can be null because buckets where no values were observed will still have a model, but no actual + */ + private Double actual; public ModelPlot(String jobId, Date timestamp, long bucketSpan) { this.jobId = jobId; @@ -121,7 +124,11 @@ public class ModelPlot implements ToXContentObject, Writeable { modelLower = in.readDouble(); modelUpper = in.readDouble(); modelMedian = in.readDouble(); - actual = in.readDouble(); + if (in.getVersion().before(Version.V_6_0_0_rc1)) { + actual = in.readDouble(); + } else { + actual = in.readOptionalDouble(); + } if (in.getVersion().onOrAfter(Version.V_5_5_0)) { bucketSpan = in.readLong(); } else { @@ -156,7 +163,17 @@ public class ModelPlot implements ToXContentObject, Writeable { out.writeDouble(modelLower); out.writeDouble(modelUpper); out.writeDouble(modelMedian); - out.writeDouble(actual); + if (out.getVersion().before(Version.V_6_0_0_rc1)) { + if (actual == null) { + // older versions cannot accommodate null, so we have no choice but to propagate the bug of + // https://github.com/elastic/x-pack-elasticsearch/issues/2528 + out.writeDouble(0.0); + } else { + out.writeDouble(actual); + } + } else { + out.writeOptionalDouble(actual); + } if (out.getVersion().onOrAfter(Version.V_5_5_0)) { out.writeLong(bucketSpan); } @@ -196,7 +213,9 @@ public class ModelPlot implements ToXContentObject, Writeable { builder.field(MODEL_LOWER.getPreferredName(), modelLower); builder.field(MODEL_UPPER.getPreferredName(), modelUpper); builder.field(MODEL_MEDIAN.getPreferredName(), modelMedian); - builder.field(ACTUAL.getPreferredName(), actual); + if (actual != null) { + builder.field(ACTUAL.getPreferredName(), actual); + } builder.endObject(); return builder; } @@ -302,11 +321,11 @@ public class ModelPlot implements ToXContentObject, Writeable { this.modelMedian = modelMedian; } - public double getActual() { + public Double getActual() { return actual; } - public void setActual(double actual) { + public void setActual(Double actual) { this.actual = actual; } @@ -331,7 +350,7 @@ public class ModelPlot implements ToXContentObject, Writeable { this.modelLower == that.modelLower && this.modelUpper == that.modelUpper && this.modelMedian == that.modelMedian && - this.actual == that.actual && + Objects.equals(this.actual, that.actual) && this.bucketSpan == that.bucketSpan; } diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/results/ModelPlotTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/results/ModelPlotTests.java index 0354a5933e4..c6e316ed11f 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/job/results/ModelPlotTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/job/results/ModelPlotTests.java @@ -6,12 +6,19 @@ package org.elasticsearch.xpack.ml.job.results; import org.elasticsearch.common.io.stream.Writeable.Reader; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.AbstractSerializingTestCase; +import java.io.IOException; import java.util.Date; import java.util.Objects; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; + public class ModelPlotTests extends AbstractSerializingTestCase { @Override @@ -172,6 +179,15 @@ public class ModelPlotTests extends AbstractSerializingTestCase { assertFalse(modelPlot2.equals(modelPlot1)); } + public void testEquals_GivenDifferentOneNullActual() { + ModelPlot modelPlot1 = createFullyPopulated(); + ModelPlot modelPlot2 = createFullyPopulated(); + modelPlot2.setActual(null); + + assertFalse(modelPlot1.equals(modelPlot2)); + assertFalse(modelPlot2.equals(modelPlot1)); + } + public void testEquals_GivenEqualmodelPlots() { ModelPlot modelPlot1 = createFullyPopulated(); ModelPlot modelPlot2 = createFullyPopulated(); @@ -181,6 +197,17 @@ public class ModelPlotTests extends AbstractSerializingTestCase { assertEquals(modelPlot1.hashCode(), modelPlot2.hashCode()); } + public void testToXContent_GivenNullActual() throws IOException { + XContentBuilder builder = JsonXContent.contentBuilder(); + + ModelPlot modelPlot = createFullyPopulated(); + modelPlot.setActual(null); + modelPlot.toXContent(builder, ToXContent.EMPTY_PARAMS); + + String json = builder.string(); + assertThat(json, not(containsString("actual"))); + } + public void testId() { ModelPlot plot = new ModelPlot("job-foo", new Date(100L), 60L);