LuciferYang commented on code in PR #55932:
URL: https://github.com/apache/spark/pull/55932#discussion_r3749238446
##########
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);
+ }
+ while (totalSkip > 0) {
+ totalSkip -= in.skip(totalSkip);
Review Comment:
`skipBinary`'s `while (totalSkip > 0) { totalSkip -= in.skip(totalSkip); }`
never exits once the stream is dry. `SingleBufferInputStream.skip` returns -1
when `remaining() <= 0`, and `MultiBufferInputStream.skip` returns -1 when it
cannot advance, so every iteration adds 1 to `totalSkip`.
- Impact: a pure-CPU spin with no I/O and no interrupt check, so `killTask`
cannot stop it and the stage hangs.
- Change: the old per-value `int` loop wrapped negative after 2^31
iterations per value, so one `skipBinary(4096)` already took about 2.7 hours.
`long` removes even that bound: roughly 511 years at the rate I measured.
- Suggestion: use `in.skipFully(totalSkip)` and wrap the `IOException` in a
`ParquetDecodingException`. On its own it is not enough: for a negative
`totalSkip`, `skip(-n)` rewinds the stream and `skipFully` does not throw, so
pair it with the non-negative check.
--
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]
