iemejia commented on code in PR #55932:
URL: https://github.com/apache/spark/pull/55932#discussion_r3875891698
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaLengthByteArrayReader.java:
##########
@@ -113,11 +119,12 @@ public ByteBuffer getBytes(int rowId) {
@Override
public void skipBinary(int total) {
+ long totalSkip = 0;
for (int i = 0; i < total; i++) {
- int remaining = lengthsVector.getInt(currentRow + i);
- while (remaining > 0) {
- remaining -= in.skip(remaining);
- }
+ totalSkip += lengthsVector.getInt(currentRow + i);
Review Comment:
Good catch - addressed. Both `readBinary` and `getBytes` now validate each
decoded length before it reaches `in.slice`, via a shared `checkLength` helper
that throws `ParquetDecodingException("Encountered negative length: ...")`. I
applied the same guard to `readGeoData` (`in.readNBytes`), which had the
identical flaw, so the four length-consuming paths are consistent. As you
noted, it throws rather than clamping to 0. Covered by the new test below.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaLengthByteArrayReader.java:
##########
@@ -113,11 +119,12 @@ public ByteBuffer getBytes(int rowId) {
@Override
public void skipBinary(int total) {
+ long totalSkip = 0;
Review Comment:
Done, following your construction: the new test `readBinary, getBytes and
skipBinary reject a negative decoded length` writes the length header directly
with `DeltaBinaryPackingValuesWriterForInteger` (lengths `[3, -6, 3]`) and
appends a data region, since Spark's writer never emits a negative length. It
asserts all three methods throw. For `skipBinary` the check stays per value
inside the accumulation loop, so `[3, -6, 3]` throws before any skip rather
than summing and advancing `currentRow` past an unmoved stream - I kept the
accumulation-loop placement precisely for that reason.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaLengthByteArrayReader.java:
##########
@@ -54,17 +54,26 @@ public void initFromPage(int valueCount,
ByteBufferInputStream in) throws IOExce
@Override
public void readBinary(int total, WritableColumnVector c, int rowId) {
- ByteBuffer buffer;
- ByteBufferOutputWriter outputWriter =
ByteBufferOutputWriter::writeArrayByteBuffer;
- int length;
+ // Compute total data length across all values so we can read everything
in a single
Review Comment:
Thanks - you're right. Updated the PR description: the bold speedups in the
table are from the integer-millisecond Best Time column and round high, so I
noted that the unrounded `Rate(M/s)` column gives 1.93x-2.28x across the four
payload sizes (and that rounding is why JDK 17 and JDK 21 both read 2.33x
despite different baseline rates). The mechanism argument stands: post-change
`skipBinary` lands within 0.6% across the three files while same-group
`readBinary` stays flat.
--
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]
