From a14b263ed442a4a088b1b0e5578993644147a599 Mon Sep 17 00:00:00 2001 From: vkorukanti Date: Thu, 29 Mar 2018 13:39:56 -0700 Subject: [PATCH] ARROW-2368: Correctly pad negative values in DecimalVector#setBigEndian --- .../arrow/vector/NullableDecimalVector.java | 26 +++++++-- .../arrow/vector/TestDecimalVector.java | 54 +++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/java/vector/src/main/java/org/apache/arrow/vector/NullableDecimalVector.java b/java/vector/src/main/java/org/apache/arrow/vector/NullableDecimalVector.java index 0d0a7c0ec1332..69bcd33fc6eba 100644 --- a/java/vector/src/main/java/org/apache/arrow/vector/NullableDecimalVector.java +++ b/java/vector/src/main/java/org/apache/arrow/vector/NullableDecimalVector.java @@ -216,7 +216,6 @@ public void set(int index, ArrowBuf buffer) { * @param value array of bytes containing decimal in big endian byte order. */ public void setBigEndian(int index, byte[] value) { - assert value.length <= TYPE_WIDTH; BitVectorHelper.setValidityBitToOne(validityBuffer, index); final int length = value.length; int startIndex = index * TYPE_WIDTH; @@ -228,13 +227,32 @@ public void setBigEndian(int index, byte[] value) { valueBuffer.setByte(startIndex + 3, value[i-3]); startIndex += 4; } - } else { + + return; + } + + if (length == 0) { + valueBuffer.setZero(startIndex, TYPE_WIDTH); + return; + } + + if (length < 16) { for (int i = length - 1; i >= 0; i--) { valueBuffer.setByte(startIndex, value[i]); startIndex++; } - valueBuffer.setZero(startIndex, TYPE_WIDTH - length); + + final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00); + final int maxStartIndex = (index + 1) * TYPE_WIDTH; + while (startIndex < maxStartIndex) { + valueBuffer.setByte(startIndex, pad); + startIndex++; + } + + return; } + + throw new IllegalArgumentException("Invalid decimal value length. Valid length in [1 - 16], got " + length); } /** @@ -472,4 +490,4 @@ public void copyValueSafe(int fromIndex, int toIndex) { to.copyFromSafe(fromIndex, toIndex, NullableDecimalVector.this); } } -} \ No newline at end of file +} diff --git a/java/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java b/java/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java index 4d844d6d3ca0f..be21200d697d4 100644 --- a/java/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java +++ b/java/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.math.BigDecimal; import java.math.BigInteger; @@ -111,4 +112,57 @@ public void testBigDecimalDifferentScaleAndPrecision() { } } } + + /** + * Test {@link NullableDecimalVector#setBigEndian(int, byte[])} which takes BE layout input and stores in LE layout. + * Cases to cover: value given in byte array in different lengths in range [1-16] and negative values. + */ + @Test + public void decimalBE2LE() { + try (NullableDecimalVector decimalVector = TestUtils.newVector(NullableDecimalVector.class, "decimal", new ArrowType.Decimal(21, 2), allocator);) { + decimalVector.allocateNew(); + + BigInteger[] testBigInts = new BigInteger[] { + new BigInteger("0"), + new BigInteger("-1"), + new BigInteger("23"), + new BigInteger("234234"), + new BigInteger("-234234234"), + new BigInteger("234234234234"), + new BigInteger("-56345345345345"), + new BigInteger("29823462983462893462934679234653456345"), // converts to 16 byte array + new BigInteger("-3894572983475982374598324598234346536"), // converts to 16 byte array + new BigInteger("-345345"), + new BigInteger("754533") + }; + + int insertionIdx = 0; + insertionIdx++; // insert a null + for (BigInteger val : testBigInts) { + decimalVector.setBigEndian(insertionIdx++, val.toByteArray()); + } + insertionIdx++; // insert a null + // insert a zero length buffer + decimalVector.setBigEndian(insertionIdx++, new byte[0]); + + // Try inserting a buffer larger than 16bytes and expect a failure + try { + decimalVector.setBigEndian(insertionIdx, new byte[17]); + fail("above statement should have failed"); + } catch (IllegalArgumentException ex) { + assertTrue(ex.getMessage().equals("Invalid decimal value length. Valid length in [1 - 16], got 17")); + } + decimalVector.setValueCount(insertionIdx); + + // retrieve values and check if they are correct + int outputIdx = 0; + assertTrue(decimalVector.isNull(outputIdx++)); + for (BigInteger expected : testBigInts) { + final BigDecimal actual = decimalVector.getObject(outputIdx++); + assertEquals(expected, actual.unscaledValue()); + } + assertTrue(decimalVector.isNull(outputIdx++)); + assertEquals(BigInteger.valueOf(0), decimalVector.getObject(outputIdx).unscaledValue()); + } + } }