[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@47a7365f59

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 <code>null</code> 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;
     }
 

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<ModelPlot> {
 
     @Override
@@ -172,6 +179,15 @@ public class ModelPlotTests extends AbstractSerializingTestCase<ModelPlot> {
         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<ModelPlot> {
         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);