[parquet-mr] branch master updated: PARQUET-1417: BINARY_AS_SIGNED_INTEGER_COMPARATOR fails with IOBE for the same arrays with the different length (#522)

[gabor]

This is an automated email from the ASF dual-hosted git repository.

gabor pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/parquet-mr.git


The following commit(s) were added to refs/heads/master by this push:
     new 93767ca  PARQUET-1417: BINARY_AS_SIGNED_INTEGER_COMPARATOR fails with 
IOBE for the same arrays with the different length (#522)
93767ca is described below

commit 93767ca512524cbf51548430782f061773600971
Author: Volodymyr Vysotskyi <vvo...@gmail.com>
AuthorDate: Thu Sep 20 12:13:18 2018 +0300

    PARQUET-1417: BINARY_AS_SIGNED_INTEGER_COMPARATOR fails with IOBE for the 
same arrays with the different length (#522)
---
 .../apache/parquet/schema/PrimitiveComparator.java    |  6 +++---
 .../parquet/schema/TestPrimitiveComparator.java       | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git 
a/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java
 
b/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java
index 5e9adbc..d343b0e 100644
--- 
a/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java
+++ 
b/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java
@@ -238,8 +238,8 @@ public abstract class PrimitiveComparator<T> implements 
Comparator<T>, Serializa
       int p1 = b1.position();
       int p2 = b2.position();
 
-      boolean isNegative1 = l1 > 0 ? b1.get(p1) < 0 : false;
-      boolean isNegative2 = l2 > 0 ? b2.get(p2) < 0 : false;
+      boolean isNegative1 = l1 > 0 && b1.get(p1) < 0;
+      boolean isNegative2 = l2 > 0 && b2.get(p2) < 0;
       if (isNegative1 != isNegative2) {
         return isNegative1 ? -1 : 1;
       }
@@ -259,7 +259,7 @@ public abstract class PrimitiveComparator<T> implements 
Comparator<T>, Serializa
 
       // The beginning of the longer buffer equals to the padding or the 
lengths are equal
       if (result == 0) {
-        result = compare(l1, b1, p1, b2, p2);
+        result = compare(Math.min(l1, l2), b1, p1, b2, p2);
       }
       return result;
     }
diff --git 
a/parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java
 
b/parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java
index 3f9d643..0bf3599 100644
--- 
a/parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java
+++ 
b/parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java
@@ -23,6 +23,8 @@ import org.junit.Test;
 
 import java.math.BigInteger;
 import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
 
 import static org.apache.parquet.schema.PrimitiveComparator.BOOLEAN_COMPARATOR;
 import static org.apache.parquet.schema.PrimitiveComparator.DOUBLE_COMPARATOR;
@@ -249,6 +251,23 @@ public class TestPrimitiveComparator {
             ByteBuffer.wrap(new 
BigInteger("9999999999999999999999999999999999999999").toByteArray())));
   }
 
+  @Test
+  public void testBinaryAsSignedIntegerComparatorWithEquals() {
+    List<Binary> valuesToCompare = new ArrayList<>();
+    valuesToCompare.add(Binary.fromConstantByteBuffer(ByteBuffer.wrap(new 
byte[] { 0, 0, -108 })));
+    valuesToCompare.add(Binary.fromConstantByteBuffer(ByteBuffer.wrap(new 
byte[] { 0, 0, 0, 0, 0, -108 })));
+    valuesToCompare.add(Binary.fromConstantByteBuffer(ByteBuffer.wrap(new 
byte[] { 0, 0, 0, -108 })));
+    valuesToCompare.add(Binary.fromConstantByteBuffer(ByteBuffer.wrap(new 
byte[] { 0, 0, 0, 0, -108 })));
+    valuesToCompare.add(Binary.fromConstantByteBuffer(ByteBuffer.wrap(new 
byte[] { 0, -108 })));
+
+    for (Binary v1 : valuesToCompare) {
+      for (Binary v2 : valuesToCompare) {
+        assertEquals(String.format("Wrong result of comparison %s and %s", v1, 
v2),
+            0, BINARY_AS_SIGNED_INTEGER_COMPARATOR.compare(v1, v2));
+      }
+    }
+  }
+
   private <T> void testObjectComparator(PrimitiveComparator<T> comparator, 
T... valuesInAscendingOrder) {
     for (int i = 0; i < valuesInAscendingOrder.length; ++i) {
       for (int j = 0; j < valuesInAscendingOrder.length; ++j) {