iemejia commented on code in PR #3476:
URL: https://github.com/apache/parquet-java/pull/3476#discussion_r3401893936
##########
.gitignore:
##########
@@ -20,4 +21,3 @@ target/
mvn_install.log
.vscode/*
.DS_Store
-
Review Comment:
Done, restored in b5f6cb9fb.
##########
parquet-plugins/parquet-encoding-vector/src/main/java/org/apache/parquet/column/values/bitpacking/ByteBitPacking512VectorLE.java:
##########
@@ -3175,13 +3171,56 @@ 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);
}
}
+ // TODO Replace these helpers with ByteVector.fromMemorySegment(...) once
the project's minimum
+ // supported JDK is >= 22 (where java.lang.foreign.MemorySegment became a
permanent API per
+ // JEP 454). fromMemorySegment is the direct successor to
ByteVector.fromByteBuffer (which was
+ // removed after JDK 21) and is intrinsifiable by HotSpot for both heap and
direct buffers via
+ // jdk.internal.misc.ScopedMemoryAccess, eliminating the byte[] copy that
this fallback uses
+ // for direct ByteBuffers. Until then, the implementations below are
constrained to APIs
+ // available under --release 17. A multi-release JAR overlay
(src/main/java22) would let JDK
+ // 22+ runtimes pick up the fromMemorySegment path automatically; see
GH-3475 discussion.
+ private static ByteVector fromByteBuffer(VectorSpecies<Byte> species,
ByteBuffer input, int inPos) {
+ // Heap buffers are loaded directly from their backing array: no
ByteBuffer.duplicate(),
+ // no intermediate byte[] allocation, no extra copy. ByteVector.fromArray
is intrinsified by
+ // HotSpot and lowered to a single AVX-512 load.
+ if (input.hasArray()) {
+ return ByteVector.fromArray(species, input.array(), input.arrayOffset()
+ inPos);
+ }
+ // Off-heap (direct) buffers: fall back to copying species.length() bytes
into a scratch
+ // array. The original JDK 17 fast path used
ScopedMemoryAccess.loadFromByteBuffer to avoid
+ // the copy, but that intrinsic is not exposed; the alternative
ByteVector.fromMemorySegment
+ // is only available on JDK 19+ and cannot be called from --release 17
sources.
+ return ByteVector.fromArray(species, readInputBytes(input, inPos,
species.length()), 0);
+ }
+
+ private static ByteVector fromByteBuffer(
+ VectorSpecies<Byte> species, ByteBuffer input, int inPos,
VectorMask<Byte> mask) {
+ // Mirror the fast path of OpenJDK 17's ByteVector.fromByteBuffer(species,
bb, offset, bo, m):
+ // when the full vector window fits in the buffer, read the entire window
unconditionally and
+ // let fromArray(species, array, 0, mask) apply the mask. This matches the
JDK semantics for
+ // arbitrary mask shapes (not just contiguous-prefix masks) and keeps
array.length equal to
+ // species.length(), which satisfies the bounds-check precondition of
fromArray.
+ if (input.hasArray()) {
+ return ByteVector.fromArray(species, input.array(), input.arrayOffset()
+ inPos, mask);
+ }
+ return ByteVector.fromArray(species, readInputBytes(input, inPos,
species.length()), 0, mask);
+ }
Review Comment:
Good suggestion. Added `unpackValuesUsingVectorDirectByteBuffer` and
`unpackValuesUsingVectorReadOnlyByteBuffer` tests in b5f6cb9fb. They exercise
`ByteBuffer.allocateDirect()` and `.asReadOnlyBuffer()` respectively, covering
all 32 bit widths.
##########
.github/workflows/vector-plugins.yml:
##########
@@ -46,12 +46,19 @@ jobs:
run: |
EXTRA_JAVA_TEST_ARGS=$(./mvnw help:evaluate
-Dexpression=extraJavaTestArgs -q -DforceStdout)
export MAVEN_OPTS="$MAVEN_OPTS $EXTRA_JAVA_TEST_ARGS"
- ./mvnw install --batch-mode -Pvector-plugins -DskipTests=true
-Dmaven.javadoc.skip=true -Dsource.skip=true -Dmaven.buildNumber.skip=true
-Djava.version=${{ matrix.java }} -pl
parquet-plugins/parquet-encoding-vector,parquet-plugins/parquet-plugins-benchmarks
-am
+ ./mvnw install --batch-mode -Pvector-plugins -DskipTests=true
-Dmaven.javadoc.skip=true -Dsource.skip=true -Dmaven.buildNumber.skip=true
-Dspotless.check.skip=true -Djava.version=${{ matrix.java }} -pl
parquet-plugins/parquet-encoding-vector,parquet-plugins/parquet-plugins-benchmarks
-am
Review Comment:
Good idea — refactored in b5f6cb9fb. The install step now computes
`SPOTLESS_ARGS` the same way as the verify step (skip on non-17 JDKs, run on
17).
##########
parquet-plugins/parquet-encoding-vector/src/main/java/org/apache/parquet/column/values/bitpacking/ByteBitPacking512VectorLE.java:
##########
@@ -3175,13 +3171,56 @@ 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);
}
}
+ // TODO Replace these helpers with ByteVector.fromMemorySegment(...) once
the project's minimum
+ // supported JDK is >= 22 (where java.lang.foreign.MemorySegment became a
permanent API per
+ // JEP 454). fromMemorySegment is the direct successor to
ByteVector.fromByteBuffer (which was
+ // removed after JDK 21) and is intrinsifiable by HotSpot for both heap and
direct buffers via
+ // jdk.internal.misc.ScopedMemoryAccess, eliminating the byte[] copy that
this fallback uses
+ // for direct ByteBuffers. Until then, the implementations below are
constrained to APIs
+ // available under --release 17. A multi-release JAR overlay
(src/main/java22) would let JDK
+ // 22+ runtimes pick up the fromMemorySegment path automatically; see
GH-3475 discussion.
+ private static ByteVector fromByteBuffer(VectorSpecies<Byte> species,
ByteBuffer input, int inPos) {
+ // Heap buffers are loaded directly from their backing array: no
ByteBuffer.duplicate(),
+ // no intermediate byte[] allocation, no extra copy. ByteVector.fromArray
is intrinsified by
+ // HotSpot and lowered to a single AVX-512 load.
+ if (input.hasArray()) {
+ return ByteVector.fromArray(species, input.array(), input.arrayOffset()
+ inPos);
+ }
+ // Off-heap (direct) buffers: fall back to copying species.length() bytes
into a scratch
+ // array. The original JDK 17 fast path used
ScopedMemoryAccess.loadFromByteBuffer to avoid
+ // the copy, but that intrinsic is not exposed; the alternative
ByteVector.fromMemorySegment
+ // is only available on JDK 19+ and cannot be called from --release 17
sources.
+ return ByteVector.fromArray(species, readInputBytes(input, inPos,
species.length()), 0);
+ }
+
+ private static ByteVector fromByteBuffer(
+ VectorSpecies<Byte> species, ByteBuffer input, int inPos,
VectorMask<Byte> mask) {
Review Comment:
Valid concern. Fixed in b5f6cb9fb: the masked direct-buffer path now
allocates a `species.length()`-sized array (satisfying `fromArray`'s bounds
precondition) but only copies `mask.trueCount()` bytes — preserving JDK 17
semantics where only masked-on lanes must be in bounds. This avoids the
`BufferUnderflowException` for packers like Packer3 whose mask covers fewer
bytes than the vector 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.
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]
