Re: [PR] chore: Removing copying data from dictionary values into CometDictionary [datafusion-comet]

[via GitHub]

viirya commented on code in PR #490:
URL: https://github.com/apache/datafusion-comet/pull/490#discussion_r1618088185


##########
common/src/main/java/org/apache/comet/vector/CometDictionary.java:
##########
@@ -59,121 +46,56 @@ public ValueVector getValueVector() {
   }
 
   public boolean decodeToBoolean(int index) {
-    return booleans[index];
+    return values.getBoolean(index);
   }
 
   public byte decodeToByte(int index) {
-    return bytes[index];
+    return values.getByte(index);
   }
 
   public short decodeToShort(int index) {
-    return shorts[index];
+    return values.getShort(index);
   }
 
   public int decodeToInt(int index) {
-    return ints[index];
+    return values.getInt(index);
   }
 
   public long decodeToLong(int index) {
-    return longs[index];
+    return values.getLong(index);
   }
 
   public float decodeToFloat(int index) {
-    return floats[index];
+    return values.getFloat(index);
   }
 
   public double decodeToDouble(int index) {
-    return doubles[index];
+    return values.getDouble(index);
   }
 
   public byte[] decodeToBinary(int index) {
-    return binaries[index].bytes;
+    switch (values.getValueVector().getMinorType()) {
+      case BIT:
+        return values.getBinary(index);
+      case DECIMAL:
+        byte[] bytes = new byte[DECIMAL_BYTE_WIDTH];
+        bytes = values.copyBinaryDecimal(index, bytes);
+        return bytes;
+      default:
+        throw new IllegalArgumentException(
+                "Invalid Arrow minor type: " + 
values.getValueVector().getMinorType());
+    }
   }
 
   public UTF8String decodeToUTF8String(int index) {
-    return strings[index];
+    return values.getUTF8String(index);
   }
 
   @Override
   public void close() {
     values.close();
   }
 
-  private void initialize() {
-    switch (values.getValueVector().getMinorType()) {
-      case BIT:
-        booleans = new boolean[numValues];
-        for (int i = 0; i < numValues; i++) {
-          booleans[i] = values.getBoolean(i);
-        }

Review Comment:
   The recent performance regression hits that this copying from dictionary to 
`CometDictionary` is slow. It seems we don't need to copy dictionary values.



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

To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org