[GitHub] [arrow] kiszk commented on a change in pull request #8056: ARROW-9861: [Java] Support big-endian in DecimalVector

[GitBox <gi...@apache.org>]

kiszk commented on a change in pull request #8056:
URL: https://github.com/apache/arrow/pull/8056#discussion_r515768974



##########
File path: java/vector/src/main/java/org/apache/arrow/vector/util/DecimalUtility.java
##########
@@ -131,9 +135,14 @@ public static void writeBigDecimalToArrowBuf(BigDecimal value, ArrowBuf bytebuf,
    */
   public static void writeLongToArrowBuf(long value, ArrowBuf bytebuf, int index) {
     final long addressOfValue = bytebuf.memoryAddress() + (long) index * DECIMAL_BYTE_LENGTH;
-    PlatformDependent.putLong(addressOfValue, value);
     final long padValue = Long.signum(value) == -1 ? -1L : 0L;
-    PlatformDependent.putLong(addressOfValue + Long.BYTES, padValue);
+    if (LITTLE_ENDIAN) {
+      PlatformDependent.putLong(addressOfValue, value);
+      PlatformDependent.putLong(addressOfValue + Long.BYTES, padValue);

Review comment:
       This is used for `DecialVector` that is a fixed-width (16-byte) vector. This routine extends the signed bit to a new long. I will write a document in the comment.

##########
File path: java/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java
##########
@@ -218,25 +220,37 @@ public void setBigEndian(int index, byte[] value) {
     valueBuffer.checkBytes((long) index * TYPE_WIDTH, (long) (index + 1) * TYPE_WIDTH);
 
     long outAddress = valueBuffer.memoryAddress() + (long) index * TYPE_WIDTH;
-    // swap bytes to convert BE to LE
-    for (int byteIdx = 0; byteIdx < length; ++byteIdx) {
-      PlatformDependent.putByte(outAddress + byteIdx, value[length - 1 - byteIdx]);
-    }
-
-    if (length == TYPE_WIDTH) {
-      return;
-    }
-
     if (length == 0) {
       PlatformDependent.setMemory(outAddress, DecimalVector.TYPE_WIDTH, (byte) 0);
-    } else if (length < TYPE_WIDTH) {
-      // sign extend
-      final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00);
-      PlatformDependent.setMemory(outAddress + length, DecimalVector.TYPE_WIDTH - length, pad);
+      return;
+    }
+    if (LITTLE_ENDIAN) {
+      // swap bytes to convert BE to LE
+      for (int byteIdx = 0; byteIdx < length; ++byteIdx) {
+        PlatformDependent.putByte(outAddress + byteIdx, value[length - 1 - byteIdx]);
+      }
+
+      if (length == TYPE_WIDTH) {
+        return;
+      }
+
+      if (length < TYPE_WIDTH) {
+        // sign extend
+        final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00);
+        PlatformDependent.setMemory(outAddress + length, DecimalVector.TYPE_WIDTH - length, pad);
+        return;
+      }
     } else {
-      throw new IllegalArgumentException(
-          "Invalid decimal value length. Valid length in [1 - 16], got " + length);
+      if (length <= TYPE_WIDTH) {

Review comment:
       If we add `if (length == TYPE_WIDTH) return` before line 244, no data is copied from `value` to `outAddress`. I will add a comment `copy data from value to outAddress` after line 244.

##########
File path: java/vector/src/test/java/org/apache/arrow/vector/util/DecimalUtilityTest.java
##########
@@ -32,6 +32,31 @@
   private static final BigInteger[] MIN_BIG_INT = new BigInteger[]{MAX_BIG_INT[0].multiply(BigInteger.valueOf(-1)),
      MAX_BIG_INT[1].multiply(BigInteger.valueOf(-1))};
 
+  @Test
+  public void testSetLongInDecimalArrowBuf() {
+    try (BufferAllocator allocator = new RootAllocator(128);
+         ArrowBuf buf = allocator.buffer(16);
+    ) {
+      int [] intValues = new int [] {Integer.MAX_VALUE, Integer.MIN_VALUE, 0};
+      for (int val : intValues) {
+        buf.clear();
+        DecimalUtility.writeLongToArrowBuf((long) val, buf, 0);
+        BigDecimal actual = DecimalUtility.getBigDecimalFromArrowBuf(buf, 0, 0);
+        BigDecimal expected = BigDecimal.valueOf(val);
+        Assert.assertEquals(expected, actual);
+      }
+
+      long [] longValues = new long[] {Long.MIN_VALUE, 0 , Long.MAX_VALUE};

Review comment:
       Sure, I will drop lines 49-57.

##########
File path: java/vector/src/main/java/org/apache/arrow/vector/util/DecimalUtility.java
##########
@@ -131,9 +135,14 @@ public static void writeBigDecimalToArrowBuf(BigDecimal value, ArrowBuf bytebuf,
    */
   public static void writeLongToArrowBuf(long value, ArrowBuf bytebuf, int index) {
     final long addressOfValue = bytebuf.memoryAddress() + (long) index * DECIMAL_BYTE_LENGTH;
-    PlatformDependent.putLong(addressOfValue, value);
     final long padValue = Long.signum(value) == -1 ? -1L : 0L;
-    PlatformDependent.putLong(addressOfValue + Long.BYTES, padValue);
+    if (LITTLE_ENDIAN) {
+      PlatformDependent.putLong(addressOfValue, value);
+      PlatformDependent.putLong(addressOfValue + Long.BYTES, padValue);

Review comment:
       In addition, this PR will support to write it to a 256-bit entry for the future.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org