From d0f4bc16caf9d9da3319b42bc35e9382f37704c9 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Fri, 16 Sep 2016 16:15:46 +0200 Subject: [PATCH] Fix FieldStats deserialization of `ip` field (#20522) * Fix FieldStats deserialization of `ip` field Add missing readBytes in `ip` field deserialization Add (de)serialization tests for all types This change also removes the ability to set FieldStats.minValue or FieldStats.maxValue to null. This is not required anymore since the stats are built on fields with values only. Fixes #20516 --- .../action/fieldstats/FieldStats.java | 172 ++++++++---------- .../fieldstats/FieldStatsTests.java | 54 ++++++ 2 files changed, 125 insertions(+), 101 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/fieldstats/FieldStats.java b/core/src/main/java/org/elasticsearch/action/fieldstats/FieldStats.java index 4a4f106b085..1b2f1dc5ed5 100644 --- a/core/src/main/java/org/elasticsearch/action/fieldstats/FieldStats.java +++ b/core/src/main/java/org/elasticsearch/action/fieldstats/FieldStats.java @@ -34,6 +34,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; import java.net.InetAddress; +import java.util.Objects; public abstract class FieldStats implements Writeable, ToXContent { private final byte type; @@ -46,13 +47,11 @@ public abstract class FieldStats implements Writeable, ToXContent { protected T minValue; protected T maxValue; - FieldStats(byte type, long maxDoc, boolean isSearchable, boolean isAggregatable) { - this(type, maxDoc, 0, 0, 0, isSearchable, isAggregatable, null, null); - } - FieldStats(byte type, long maxDoc, long docCount, long sumDocFreq, long sumTotalTermFreq, boolean isSearchable, boolean isAggregatable, T minValue, T maxValue) { + Objects.requireNonNull(minValue, "minValue must not be null"); + Objects.requireNonNull(maxValue, "maxValue must not be null"); this.type = type; this.maxDoc = maxDoc; this.docCount = docCount; @@ -220,14 +219,10 @@ public abstract class FieldStats implements Writeable, ToXContent { } private void updateMinMax(T min, T max) { - if (minValue == null) { - minValue = min; - } else if (min != null && compare(minValue, min) > 0) { + if (compare(minValue, min) > 0) { minValue = min; } - if (maxValue == null) { - maxValue = max; - } else if (max != null && compare(maxValue, max) < 0) { + if (compare(maxValue, max) < 0) { maxValue = max; } } @@ -266,11 +261,7 @@ public abstract class FieldStats implements Writeable, ToXContent { out.writeLong(sumTotalTermFreq); out.writeBoolean(isSearchable); out.writeBoolean(isAggregatable); - boolean hasMinMax = minValue != null; - out.writeBoolean(hasMinMax); - if (hasMinMax) { - writeMinMax(out); - } + writeMinMax(out); } protected abstract void writeMinMax(StreamOutput out) throws IOException; @@ -280,9 +271,6 @@ public abstract class FieldStats implements Writeable, ToXContent { * otherwise false is returned */ public boolean match(IndexConstraint constraint) { - if (minValue == null) { - return false; - } int cmp; T value = valueOf(constraint.getValue(), constraint.getOptionalFormat()); if (constraint.getProperty() == IndexConstraint.Property.MIN) { @@ -307,6 +295,31 @@ public abstract class FieldStats implements Writeable, ToXContent { } } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + FieldStats that = (FieldStats) o; + + if (type != that.type) return false; + if (maxDoc != that.maxDoc) return false; + if (docCount != that.docCount) return false; + if (sumDocFreq != that.sumDocFreq) return false; + if (sumTotalTermFreq != that.sumTotalTermFreq) return false; + if (isSearchable != that.isSearchable) return false; + if (isAggregatable != that.isAggregatable) return false; + if (!minValue.equals(that.minValue)) return false; + return maxValue.equals(that.maxValue); + + } + + @Override + public int hashCode() { + return Objects.hash(type, maxDoc, docCount, sumDocFreq, sumTotalTermFreq, isSearchable, isAggregatable, + minValue, maxValue); + } + public static class Long extends FieldStats { public Long(long maxDoc, long docCount, long sumDocFreq, long sumTotalTermFreq, boolean isSearchable, boolean isAggregatable, @@ -315,17 +328,6 @@ public abstract class FieldStats implements Writeable, ToXContent { isSearchable, isAggregatable, minValue, maxValue); } - public Long(long maxDoc, long docCount, long sumDocFreq, long sumTotalTermFreq, - boolean isSearchable, boolean isAggregatable) { - super((byte) 0, maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable, null, null); - } - - public Long(long maxDoc, - boolean isSearchable, boolean isAggregatable) { - super((byte) 0, maxDoc, isSearchable, isAggregatable); - } - @Override public int compare(java.lang.Long o1, java.lang.Long o2) { return o1.compareTo(o2); @@ -344,12 +346,12 @@ public abstract class FieldStats implements Writeable, ToXContent { @Override public String getMinValueAsString() { - return minValue != null ? java.lang.Long.toString(minValue) : null; + return java.lang.Long.toString(minValue); } @Override public String getMaxValueAsString() { - return maxValue != null ? java.lang.Long.toString(maxValue) : null; + return java.lang.Long.toString(maxValue); } } @@ -361,15 +363,6 @@ public abstract class FieldStats implements Writeable, ToXContent { minValue, maxValue); } - public Double(long maxDoc, long docCount, long sumDocFreq, long sumTotalTermFreq, - boolean isSearchable, boolean isAggregatable) { - super((byte) 1, maxDoc, docCount, sumDocFreq, sumTotalTermFreq, isSearchable, isAggregatable, null, null); - } - - public Double(long maxDoc, boolean isSearchable, boolean isAggregatable) { - super((byte) 1, maxDoc, isSearchable, isAggregatable); - } - @Override public int compare(java.lang.Double o1, java.lang.Double o2) { return o1.compareTo(o2); @@ -391,12 +384,12 @@ public abstract class FieldStats implements Writeable, ToXContent { @Override public String getMinValueAsString() { - return minValue != null ? java.lang.Double.toString(minValue) : null; + return java.lang.Double.toString(minValue); } @Override public String getMaxValueAsString() { - return maxValue != null ? java.lang.Double.toString(maxValue) : null; + return java.lang.Double.toString(maxValue); } } @@ -412,20 +405,6 @@ public abstract class FieldStats implements Writeable, ToXContent { this.formatter = formatter; } - public Date(long maxDoc, long docCount, long sumDocFreq, long sumTotalTermFreq, - boolean isSearchable, boolean isAggregatable, - FormatDateTimeFormatter formatter) { - super((byte) 2, maxDoc, docCount, sumDocFreq, sumTotalTermFreq, isSearchable, isAggregatable, - null, null); - this.formatter = formatter; - } - - public Date(long maxDoc, boolean isSearchable, boolean isAggregatable, - FormatDateTimeFormatter formatter) { - super((byte) 2, maxDoc, isSearchable, isAggregatable); - this.formatter = formatter; - } - @Override public int compare(java.lang.Long o1, java.lang.Long o2) { return o1.compareTo(o2); @@ -449,12 +428,29 @@ public abstract class FieldStats implements Writeable, ToXContent { @Override public String getMinValueAsString() { - return minValue != null ? formatter.printer().print(minValue) : null; + return formatter.printer().print(minValue); } @Override public String getMaxValueAsString() { - return maxValue != null ? formatter.printer().print(maxValue) : null; + return formatter.printer().print(maxValue); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + if (!super.equals(o)) return false; + + Date that = (Date) o; + return Objects.equals(formatter.format(), that.formatter.format()); + } + + @Override + public int hashCode() { + int result = super.hashCode(); + result = 31 * result + formatter.format().hashCode(); + return result; } } @@ -467,10 +463,6 @@ public abstract class FieldStats implements Writeable, ToXContent { minValue, maxValue); } - public Text(long maxDoc, boolean isSearchable, boolean isAggregatable) { - super((byte) 3, maxDoc, isSearchable, isAggregatable); - } - @Override public int compare(BytesRef o1, BytesRef o2) { return o1.compareTo(o2); @@ -492,12 +484,12 @@ public abstract class FieldStats implements Writeable, ToXContent { @Override public String getMinValueAsString() { - return minValue != null ? minValue.utf8ToString() : null; + return minValue.utf8ToString(); } @Override public String getMaxValueAsString() { - return maxValue != null ? maxValue.utf8ToString() : null; + return maxValue.utf8ToString(); } @Override @@ -516,10 +508,6 @@ public abstract class FieldStats implements Writeable, ToXContent { minValue, maxValue); } - public Ip(long maxDoc, boolean isSearchable, boolean isAggregatable) { - super((byte) 4, maxDoc, isSearchable, isAggregatable); - } - @Override public int compare(InetAddress o1, InetAddress o2) { byte[] b1 = InetAddressPoint.encode(o1); @@ -544,12 +532,12 @@ public abstract class FieldStats implements Writeable, ToXContent { @Override public String getMinValueAsString() { - return minValue != null ? NetworkAddress.format(minValue) : null; + return NetworkAddress.format(minValue); } @Override public String getMaxValueAsString() { - return maxValue != null ? NetworkAddress.format(maxValue) : null; + return NetworkAddress.format(maxValue); } } @@ -561,53 +549,35 @@ public abstract class FieldStats implements Writeable, ToXContent { long sumTotalTermFreq = in.readLong(); boolean isSearchable = in.readBoolean(); boolean isAggregatable = in.readBoolean(); - boolean hasMinMax = in.readBoolean(); switch (type) { case 0: - if (hasMinMax) { - return new Long(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable, in.readLong(), in.readLong()); - } return new Long(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable); + isSearchable, isAggregatable, in.readLong(), in.readLong()); case 1: - if (hasMinMax) { - return new Double(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable, in.readDouble(), in.readDouble()); - } return new Double(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable); + isSearchable, isAggregatable, in.readDouble(), in.readDouble()); case 2: FormatDateTimeFormatter formatter = Joda.forPattern(in.readString()); - if (hasMinMax) { - return new Date(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable, formatter, in.readLong(), in.readLong()); - } return new Date(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable, formatter); + isSearchable, isAggregatable, formatter, in.readLong(), in.readLong()); + case 3: - if (hasMinMax) { - return new Text(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable, in.readBytesRef(), in.readBytesRef()); - } return new Text(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, - isSearchable, isAggregatable, null, null); + isSearchable, isAggregatable, in.readBytesRef(), in.readBytesRef()); case 4: - InetAddress min = null; - InetAddress max = null; - if (hasMinMax) { - int l1 = in.readByte(); - byte[] b1 = new byte[l1]; - int l2 = in.readByte(); - byte[] b2 = new byte[l2]; - min = InetAddressPoint.decode(b1); - max = InetAddressPoint.decode(b2); - } + int l1 = in.readByte(); + byte[] b1 = new byte[l1]; + in.readBytes(b1, 0, l1); + int l2 = in.readByte(); + byte[] b2 = new byte[l2]; + in.readBytes(b2, 0, l2); + InetAddress min = InetAddressPoint.decode(b1); + InetAddress max = InetAddressPoint.decode(b2); return new Ip(maxDoc, docCount, sumDocFreq, sumTotalTermFreq, isSearchable, isAggregatable, min, max); diff --git a/core/src/test/java/org/elasticsearch/fieldstats/FieldStatsTests.java b/core/src/test/java/org/elasticsearch/fieldstats/FieldStatsTests.java index 4a5f79a12a8..8cd1b479416 100644 --- a/core/src/test/java/org/elasticsearch/fieldstats/FieldStatsTests.java +++ b/core/src/test/java/org/elasticsearch/fieldstats/FieldStatsTests.java @@ -23,13 +23,19 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.action.fieldstats.FieldStats; import org.elasticsearch.action.fieldstats.FieldStatsResponse; import org.elasticsearch.action.fieldstats.IndexConstraint; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.joda.Joda; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.test.ESSingleNodeTestCase; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; +import java.io.IOException; +import java.net.InetAddress; +import java.net.UnknownHostException; import java.util.ArrayList; +import java.util.Date; import java.util.List; import java.util.Locale; @@ -513,4 +519,52 @@ public class FieldStatsTests extends ESSingleNodeTestCase { assertThat(response.getAllFieldStats().get("_type").isSearchable(), equalTo(true)); assertThat(response.getAllFieldStats().get("_type").isAggregatable(), equalTo(true)); } + + public void testSerialization() throws IOException { + for (int i = 0; i < 20; i++) { + assertSerialization(randomFieldStats()); + } + } + + /** + * creates a random field stats which does not guarantee that {@link FieldStats#maxValue} is greater than {@link FieldStats#minValue} + **/ + private FieldStats randomFieldStats() throws UnknownHostException { + int type = randomInt(5); + switch (type) { + case 0: + return new FieldStats.Long(randomPositiveLong(), randomPositiveLong(), randomPositiveLong(), + randomPositiveLong(), randomBoolean(), randomBoolean(), randomLong(), randomLong()); + case 1: + return new FieldStats.Double(randomPositiveLong(), randomPositiveLong(), randomPositiveLong(), + randomPositiveLong(), randomBoolean(), randomBoolean(), randomDouble(), randomDouble()); + case 2: + return new FieldStats.Date(randomPositiveLong(), randomPositiveLong(), randomPositiveLong(), + randomPositiveLong(), randomBoolean(), randomBoolean(), Joda.forPattern("basicDate"), + new Date().getTime(), new Date().getTime()); + case 3: + return new FieldStats.Text(randomPositiveLong(), randomPositiveLong(), randomPositiveLong(), + randomPositiveLong(), randomBoolean(), randomBoolean(), + new BytesRef(randomAsciiOfLength(10)), new BytesRef(randomAsciiOfLength(20))); + case 4: + return new FieldStats.Ip(randomPositiveLong(), randomPositiveLong(), randomPositiveLong(), + randomPositiveLong(), randomBoolean(), randomBoolean(), + InetAddress.getByName("::1"), InetAddress.getByName("::1")); + case 5: + return new FieldStats.Ip(randomPositiveLong(), randomPositiveLong(), randomPositiveLong(), + randomPositiveLong(), randomBoolean(), randomBoolean(), + InetAddress.getByName("1.2.3.4"), InetAddress.getByName("1.2.3.4")); + default: + throw new IllegalArgumentException("Invalid type"); + } + } + + private void assertSerialization(FieldStats stats) throws IOException { + BytesStreamOutput output = new BytesStreamOutput(); + stats.writeTo(output); + output.flush(); + FieldStats deserializedStats = FieldStats.readFrom(output.bytes().streamInput()); + assertThat(stats, equalTo(deserializedStats)); + assertThat(stats.hashCode(), equalTo(deserializedStats.hashCode())); + } }