LuciferYang commented on code in PR #55932:
URL: https://github.com/apache/spark/pull/55932#discussion_r3868431698
##########
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:
The `readBinary` half is not resolved by the revert, though. The revert
removed the `int` sum overflow; the negative length still reaches
`in.slice(length)` on line 63 and goes into the column vector from there.
On-heap that surfaces as an `IndexOutOfBoundsException` from
`System.arraycopy`, or as a `RuntimeException` saying `Cannot reserve
additional contiguous bytes in the vectorized reader (integer overflow)` when
the negative exceeds what the batch has appended so far. Off-heap,
`Platform.copyMemory` copies nothing because both branches loop `while (length
> 0)`, and `elementsAppended` then moves backward, so the batch is corrupted
with no error at all. None of those says anything about a negative length.
Pre-existing, so a follow-up is fine. Adding the same check to `readBinary`
and `getBytes` here is cheap too, one predictable branch per value on a path
that already costs about 16 ns/value, as long as it throws rather than clamping
to 0.
--
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]
