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

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

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



##########
File path: java/vector/src/main/java/org/apache/arrow/vector/util/DecimalUtility.java
##########
@@ -128,12 +131,26 @@ public static void writeBigDecimalToArrowBuf(BigDecimal value, ArrowBuf bytebuf,
 
   /**
    * Write the given long to the ArrowBuf at the given value index.
+   * This routine extends the original sign bit to a new upper area in 128-bit or 256-bit.
    */
-  public static void writeLongToArrowBuf(long value, ArrowBuf bytebuf, int index) {
-    final long addressOfValue = bytebuf.memoryAddress() + (long) index * DECIMAL_BYTE_LENGTH;
-    PlatformDependent.putLong(addressOfValue, value);
+  public static void writeLongToArrowBuf(long value, ArrowBuf bytebuf, int index, int byteWidth) {
+    if (byteWidth != 16 && byteWidth != 32) {
+      throw new UnsupportedOperationException("DeciimalUtility.writeLongToArrowBuf() currently supports " +

Review comment:
       typo: `DeciimalUtility` -> `DecimalUtility`

##########
File path: java/vector/src/test/java/org/apache/arrow/vector/complex/impl/TestComplexCopier.java
##########
@@ -526,7 +526,7 @@ public void testCopyFixedSizedListOfDecimalsVector() {
       to.addOrGetVector(FieldType.nullable(new ArrowType.Decimal(3, 0, 128)));
 
       DecimalHolder holder = new DecimalHolder();
-      holder.buffer = allocator.buffer(DecimalUtility.DECIMAL_BYTE_LENGTH);
+      holder.buffer = allocator.buffer(16);

Review comment:
       Could you use `DecimalVector.TYPE_WIDTH` here?

##########
File path: java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
##########
@@ -310,7 +310,7 @@ public void listDecimalType() {
       listVector.allocateNew();
       UnionListWriter listWriter = new UnionListWriter(listVector);
       DecimalHolder holder = new DecimalHolder();
-      holder.buffer = allocator.buffer(DecimalUtility.DECIMAL_BYTE_LENGTH);
+      holder.buffer = allocator.buffer(16);

Review comment:
       Same here

##########
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:
       ok, I see above it is already set during the swap. I guess there is no harm in calling `PlatformDependent.setMemory(outAddress, DecimalVector.TYPE_WIDTH - length, pad)` if `length == TYPE_WIDTH`




----------------------------------------------------------------
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