Fix invalid bucket calculation HashPartitionMaskOperator

Math.abs returns a negative value for MIN_INT, so when this hash value is later modded to choose a bucket, the bucket is also negative. In the HashPartitionMaskOperator this means the value is a member of no buckets and the value is skipped. The HashPartitionMaskOperator is only used for the experimental local parallel aggregations optimization so this not a high impact bug.
trinodb · Dec 17, 2015 · 795d988 · 795d988
1 parent 7fa62fd
commit 795d988
Show file tree

Hide file tree

Showing 2 changed files with 6 additions and 5 deletions.
diff --git a/presto-main/src/main/java/com/facebook/presto/operator/HashPartitionMaskOperator.java b/presto-main/src/main/java/com/facebook/presto/operator/HashPartitionMaskOperator.java
@@ -29,7 +29,6 @@
 import static com.facebook.presto.spi.type.BooleanType.BOOLEAN;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
-import static java.lang.Math.abs;
 import static java.util.Objects.requireNonNull;
 
 public class HashPartitionMaskOperator
@@ -198,7 +197,8 @@ public void addInput(Page page)
         for (int position = 0; position < page.getPositionCount(); position++) {
             int rawHash = hashGenerator.hashPosition(position, page);
             // mix the bits so we don't use the same hash used to distribute between stages
-            rawHash = abs((int) XxHash64.hash(Integer.reverse(rawHash)));
+            rawHash = (int) XxHash64.hash(Integer.reverse(rawHash));
+            rawHash &= Integer.MAX_VALUE;
 
             boolean active = (rawHash % partitionCount == partition);
             BOOLEAN.writeBoolean(activePositions, active);

diff --git a/presto-main/src/test/java/com/facebook/presto/operator/TestHashPartitionMaskOperator.java b/presto-main/src/test/java/com/facebook/presto/operator/TestHashPartitionMaskOperator.java
@@ -39,7 +39,6 @@
 import static com.facebook.presto.testing.MaterializedResult.resultBuilder;
 import static com.facebook.presto.testing.TestingTaskContext.createTaskContext;
 import static io.airlift.concurrent.Threads.daemonThreadsNamed;
-import static java.lang.Math.abs;
 import static java.util.concurrent.Executors.newCachedThreadPool;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertTrue;
@@ -95,7 +94,8 @@ public void testHashPartitionMask(boolean hashEnabled)
             for (int i = 0; i < ROW_COUNT; i++) {
                 int rawHash = (int) BigintOperators.hashCode(i);
                 // mix the bits so we don't use the same hash used to distribute between stages
-                rawHash = abs((int) XxHash64.hash(Integer.reverse(rawHash)));
+                rawHash = (int) XxHash64.hash(Integer.reverse(rawHash));
+                rawHash &= Integer.MAX_VALUE;
 
                 boolean active = (rawHash % PARTITION_COUNT == partition);
                 expected.row(i, active);
@@ -137,7 +137,8 @@ public void testHashPartitionMaskWithMask(boolean hashEnabled)
             for (int i = 0; i < ROW_COUNT; i++) {
                 int rawHash = (int) BigintOperators.hashCode(i);
                 // mix the bits so we don't use the same hash used to distribute between stages
-                rawHash = abs((int) XxHash64.hash(Integer.reverse(rawHash)));
+                rawHash = (int) XxHash64.hash(Integer.reverse(rawHash));
+                rawHash &= Integer.MAX_VALUE;
 
                 boolean active = (rawHash % PARTITION_COUNT == partition);
                 boolean maskValue = i % 2 == 0;