Copilot commented on code in PR #3476:
URL: https://github.com/apache/parquet-java/pull/3476#discussion_r3105443147
##########
parquet-plugins/parquet-encoding-vector/src/main/java/org/apache/parquet/column/values/bitpacking/ByteBitPacking512VectorLE.java:
##########
@@ -3175,13 +3171,30 @@ public final void unpackValuesUsingVector(final byte[]
in, final int inPos, fina
public final void unpackValuesUsingVector(
final ByteBuffer in, final int inPos, final int[] out, final int
outPos) {
- ByteVector byteVector = ByteVector.fromByteBuffer(B512, in, inPos,
in.order(), inp_mask);
+ ByteVector byteVector = fromByteBuffer(B512, in, inPos, inp_mask);
IntVector tempRes1 =
byteVector.rearrange(perm_mask0).reinterpretAsInts();
tempRes1.intoArray(out, outPos, out_mask);
}
}
+ private static ByteVector fromByteBuffer(VectorSpecies<Byte> species,
ByteBuffer input, int inPos) {
+ return ByteVector.fromArray(species, readInputBytes(input, inPos,
species.length()), 0);
+ }
+
+ private static ByteVector fromByteBuffer(
+ VectorSpecies<Byte> species, ByteBuffer input, int inPos,
VectorMask<Byte> mask) {
+ return ByteVector.fromArray(species, readInputBytes(input, inPos,
mask.trueCount()), 0, mask);
+ }
Review Comment:
In the masked overload, `readInputBytes(..., mask.trueCount())` creates a
backing array smaller than `species.length()`. `ByteVector.fromArray(species,
array, 0, mask)` may still perform bounds checks assuming `array.length >=
species.length()` (and also breaks immediately if a mask ever has a true lane
at an index >= array.length). To keep masked loads safe across JDK
implementations, allocate an array of `species.length()` and only fill the
needed prefix bytes (e.g., `mask.trueCount()`), leaving the rest as zero.
##########
parquet-plugins/parquet-encoding-vector/src/main/java/org/apache/parquet/column/values/bitpacking/ByteBitPacking512VectorLE.java:
##########
@@ -3175,13 +3171,30 @@ public final void unpackValuesUsingVector(final byte[]
in, final int inPos, fina
public final void unpackValuesUsingVector(
final ByteBuffer in, final int inPos, final int[] out, final int
outPos) {
- ByteVector byteVector = ByteVector.fromByteBuffer(B512, in, inPos,
in.order(), inp_mask);
+ ByteVector byteVector = fromByteBuffer(B512, in, inPos, inp_mask);
IntVector tempRes1 =
byteVector.rearrange(perm_mask0).reinterpretAsInts();
tempRes1.intoArray(out, outPos, out_mask);
}
}
+ private static ByteVector fromByteBuffer(VectorSpecies<Byte> species,
ByteBuffer input, int inPos) {
+ return ByteVector.fromArray(species, readInputBytes(input, inPos,
species.length()), 0);
+ }
+
+ private static ByteVector fromByteBuffer(
+ VectorSpecies<Byte> species, ByteBuffer input, int inPos,
VectorMask<Byte> mask) {
+ return ByteVector.fromArray(species, readInputBytes(input, inPos,
mask.trueCount()), 0, mask);
+ }
+
+ private static byte[] readInputBytes(ByteBuffer input, int inPos, int
byteCount) {
+ byte[] bytes = new byte[byteCount];
+ ByteBuffer source = input.duplicate();
+ source.position(inPos);
+ source.get(bytes);
Review Comment:
`readInputBytes` allocates a new `byte[]` and creates a new `ByteBuffer` via
`duplicate()` on every vector unpack from a `ByteBuffer`. This is a significant
per-call allocation/copy regression compared to the previous
`ByteVector.fromByteBuffer(...)` path and can materially impact the performance
benefits of the vector plugin. Consider reducing allocations by reusing a
scratch buffer (e.g., via `ThreadLocal<byte[]>` sized to the max species
length) and/or fast-pathing heap buffers (`input.hasArray()`) to load directly
from the backing array when possible.
```suggestion
private static final ThreadLocal<byte[]> INPUT_SCRATCH =
ThreadLocal.withInitial(() -> new byte[B512.length()]);
private static ByteVector fromByteBuffer(VectorSpecies<Byte> species,
ByteBuffer input, int inPos) {
if (input.hasArray()) {
return ByteVector.fromArray(species, input.array(),
input.arrayOffset() + inPos);
}
return ByteVector.fromArray(species, readInputBytes(input, inPos,
species.length()), 0);
}
private static ByteVector fromByteBuffer(
VectorSpecies<Byte> species, ByteBuffer input, int inPos,
VectorMask<Byte> mask) {
if (input.hasArray()) {
return ByteVector.fromArray(species, input.array(),
input.arrayOffset() + inPos, mask);
}
return ByteVector.fromArray(species, readInputBytes(input, inPos,
mask.trueCount()), 0, mask);
}
private static byte[] readInputBytes(ByteBuffer input, int inPos, int
byteCount) {
byte[] bytes = INPUT_SCRATCH.get();
for (int i = 0; i < byteCount; i++) {
bytes[i] = input.get(inPos + i);
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
