LuciferYang commented on code in PR #55932:
URL: https://github.com/apache/spark/pull/55932#discussion_r3749252453
##########
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
+ // bulk slice instead of allocating one ByteBuffer per value.
+ int totalDataLen = 0;
for (int i = 0; i < total; i++) {
- length = lengthsVector.getInt(currentRow + i);
- try {
- buffer = in.slice(length);
- } catch (EOFException e) {
- throw new ParquetDecodingException("Failed to read " + length + "
bytes");
- }
- outputWriter.write(c, rowId + i, buffer, length);
+ totalDataLen += lengthsVector.getInt(currentRow + i);
Review Comment:
`totalDataLen` is an `int` summing unvalidated file-supplied lengths. 4096
values each declaring 1 MiB sum to 2^32, so `totalDataLen` becomes 0.
- Impact: `slice(0)` does not throw, `dataArray` is still the full backing
array, and `reserve` bounds only the destination, so nothing checks the source;
off heap, `Platform.copyMemory` does an unbounded 1 MiB `Unsafe` read per value.
- Bound: `reserve` overflows at value 2048 and throws `RuntimeException`, so
the batch never completes, but the diagnostic has already degraded from
`ParquetDecodingException`.
- Suggestion: accumulate into a `long` and reject sums above
`in.available()`.
##########
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:
Neither loop checks the sign while summing, so a negative length cancels
positive ones in the same batch. `[100, -100]` sums to 0 in `skipBinary`, the
`while` never runs, `in` does not move, yet `currentRow += total` advances.
- Impact: `getBytes` then slices from the stale offset and returns bytes
belonging to earlier rows, silently, and the drift propagates through
`previous` into DELTA_BYTE_ARRAY.
- Other loop: in `readBinary` a negative `length` reaches `appendBytes(-n)`,
where `elementsAppended` moves backwards and later values overwrite bytes
already committed for earlier rows.
- Suggestion: validate each length as non-negative in both loops, alongside
the upper-bound check.
##########
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:
`skipBinary`'s gain is credible not because it is large (the untouched
`Variant reads / skipBytes` swings +142% on JDK 17 and -42% on JDK 25) but
because all 12 combinations move the same way, the post-change absolute rates
nearly coincide, and there is a mechanism. `readBinary` satisfies none of the
three.
- Impact: the int overflow, the unguarded `array()`, and the whole-batch
copy all belong to the half whose gain is unmeasurable.
- Suggestion: land `skipBinary` plus the validation fixes on their own. If
`readBinary` stays, support it with an allocation measurement rather than wall
time.
--
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]
