From 7be0c76c7217e1c25c5e9efdc98410353cf0c7d6 Mon Sep 17 00:00:00 2001 From: Stanislav Poryadnyi <37914083+aP0StAl@users.noreply.github.com> Date: Sat, 28 Mar 2020 08:26:31 +0300 Subject: [PATCH 1/2] fix MAX_INTERMEDIATE_SIZE for DoubleMeanHolder (#9568) * fix MAX_INTERMEDIATE_SIZE for DoubleMeanHolder * byte[] type handling in deserialize and finalizeComputation for DoubleMeanAggregatorFactory * DoubleMeanAggregatorFactory tests: Max Intermediate Size, Deserialize, finalizeComputation * moved byte[] check to first position Co-authored-by: Stanislav --- .../mean/DoubleMeanAggregatorFactory.java | 8 ++- .../aggregation/mean/DoubleMeanHolder.java | 12 ++-- .../mean/DoubleMeanAggregatorFactoryTest.java | 66 +++++++++++++++++++ 3 files changed, 78 insertions(+), 8 deletions(-) create mode 100644 processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactory.java index 5ed87bee66c2..d1da4d3189f0 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactory.java @@ -148,7 +148,9 @@ public List getRequiredColumns() @Override public Object deserialize(Object object) { - if (object instanceof String) { + if (object instanceof byte[]) { + return DoubleMeanHolder.fromBytes((byte[]) object); + } else if (object instanceof String) { return DoubleMeanHolder.fromBytes(StringUtils.decodeBase64(StringUtils.toUtf8((String) object))); } else if (object instanceof DoubleMeanHolder) { return object; @@ -161,7 +163,9 @@ public Object deserialize(Object object) @Override public Object finalizeComputation(@Nullable Object object) { - if (object instanceof DoubleMeanHolder) { + if (object instanceof byte[]) { + return DoubleMeanHolder.fromBytes((byte[]) object).mean(); + } else if (object instanceof DoubleMeanHolder) { return ((DoubleMeanHolder) object).mean(); } else if (object == null) { return null; diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java b/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java index f42c993b68aa..06aac1a9bb4a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java @@ -30,7 +30,7 @@ public class DoubleMeanHolder { - public static final int MAX_INTERMEDIATE_SIZE = Long.SIZE + Double.SIZE; + public static final int MAX_INTERMEDIATE_SIZE = Long.BYTES + Double.BYTES; public static final Comparator COMPARATOR = (o1, o2) -> Doubles.compare(o1.mean(), o2.mean()); private double sum; @@ -62,16 +62,16 @@ public double mean() public byte[] toBytes() { - ByteBuffer buf = ByteBuffer.allocate(Double.SIZE + Long.SIZE); + ByteBuffer buf = ByteBuffer.allocate(Double.BYTES + Long.BYTES); buf.putDouble(0, sum); - buf.putLong(Double.SIZE, count); + buf.putLong(Double.BYTES, count); return buf.array(); } public static DoubleMeanHolder fromBytes(byte[] data) { ByteBuffer buf = ByteBuffer.wrap(data); - return new DoubleMeanHolder(buf.getDouble(0), buf.getLong(Double.SIZE)); + return new DoubleMeanHolder(buf.getDouble(0), buf.getLong(Double.BYTES)); } public static void init(ByteBuffer buf, int position) @@ -109,12 +109,12 @@ private static double getSum(ByteBuffer buf, int position) private static void writeCount(ByteBuffer buf, int position, long count) { - buf.putLong(position + Double.SIZE, count); + buf.putLong(position + Double.BYTES, count); } private static long getCount(ByteBuffer buf, int position) { - return buf.getLong(position + Double.SIZE); + return buf.getLong(position + Double.BYTES); } public static class Serializer extends JsonSerializer diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java new file mode 100644 index 000000000000..0e49363f16eb --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.aggregation.mean; + +import org.apache.commons.lang3.builder.EqualsBuilder; +import org.junit.Assert; +import org.junit.Test; + +public class DoubleMeanAggregatorFactoryTest +{ + @Test + public void testMaxIntermediateSize() + { + DoubleMeanAggregatorFactory factory = new DoubleMeanAggregatorFactory("name", "fieldName"); + Assert.assertEquals(Double.BYTES + Long.BYTES, factory.getMaxIntermediateSize()); + Assert.assertEquals(Double.BYTES + Long.BYTES, factory.getMaxIntermediateSizeWithNulls()); + } + + @Test + public void testDeserialyze() + { + DoubleMeanAggregatorFactory factory = new DoubleMeanAggregatorFactory("name", "fieldName"); + DoubleMeanHolder expectedHolder = new DoubleMeanHolder(50.0, 10L); + + DoubleMeanHolder actualHolder = (DoubleMeanHolder) factory.deserialize(expectedHolder); + Assert.assertTrue(EqualsBuilder.reflectionEquals(expectedHolder, actualHolder)); + + actualHolder = (DoubleMeanHolder) factory.deserialize(expectedHolder.toBytes()); + Assert.assertTrue(EqualsBuilder.reflectionEquals(expectedHolder, actualHolder)); + } + + @Test + public void testFinalizeComputation() + { + DoubleMeanAggregatorFactory factory = new DoubleMeanAggregatorFactory("name", "fieldName"); + double sum = 50.0; + long count = 10L; + double expecterMean = sum / count; + DoubleMeanHolder holder = new DoubleMeanHolder(sum, count); + + double actualMean = (Double) factory.finalizeComputation(holder); + Assert.assertEquals("", expecterMean, actualMean, 1e-6); + + actualMean = (Double) factory.finalizeComputation(holder.toBytes()); + Assert.assertEquals("", expecterMean, actualMean, 1e-6); + + Assert.assertNull(factory.finalizeComputation(null)); + } +} From 481ce75ca6a84f8de49df14bad5f60137c19d8cd Mon Sep 17 00:00:00 2001 From: Himanshu Gupta Date: Sat, 28 Mar 2020 01:01:26 -0700 Subject: [PATCH 2/2] remove commons-lang3 usage from DoubleMeanAggregatorFactoryTest --- .../aggregation/mean/DoubleMeanHolder.java | 21 +++++++++++++++++++ .../mean/DoubleMeanAggregatorFactoryTest.java | 5 ++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java b/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java index 06aac1a9bb4a..f04150fcede8 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanHolder.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.util.Comparator; +import java.util.Objects; public class DoubleMeanHolder { @@ -68,6 +69,26 @@ public byte[] toBytes() return buf.array(); } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + DoubleMeanHolder that = (DoubleMeanHolder) o; + return Double.compare(that.sum, sum) == 0 && + count == that.count; + } + + @Override + public int hashCode() + { + return Objects.hash(sum, count); + } + public static DoubleMeanHolder fromBytes(byte[] data) { ByteBuffer buf = ByteBuffer.wrap(data); diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java index 0e49363f16eb..01dabaaca7fa 100644 --- a/processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java +++ b/processing/src/test/java/org/apache/druid/query/aggregation/mean/DoubleMeanAggregatorFactoryTest.java @@ -19,7 +19,6 @@ package org.apache.druid.query.aggregation.mean; -import org.apache.commons.lang3.builder.EqualsBuilder; import org.junit.Assert; import org.junit.Test; @@ -40,10 +39,10 @@ public void testDeserialyze() DoubleMeanHolder expectedHolder = new DoubleMeanHolder(50.0, 10L); DoubleMeanHolder actualHolder = (DoubleMeanHolder) factory.deserialize(expectedHolder); - Assert.assertTrue(EqualsBuilder.reflectionEquals(expectedHolder, actualHolder)); + Assert.assertEquals(expectedHolder, actualHolder); actualHolder = (DoubleMeanHolder) factory.deserialize(expectedHolder.toBytes()); - Assert.assertTrue(EqualsBuilder.reflectionEquals(expectedHolder, actualHolder)); + Assert.assertEquals(expectedHolder, actualHolder); } @Test