kecookier opened a new pull request, #12984:
URL: https://github.com/apache/gluten/pull/12984
<!--
Thank you for submitting a pull request! Here are some tips:
1. For first-time contributors, please read our contributing guide:
https://github.com/apache/gluten/blob/main/CONTRIBUTING.md
2. If necessary, create a GitHub issue for discussion beforehand to avoid
duplicate work.
3. If the PR is specific to a single backend, include [VL] or [CH] in the PR
title to indicate the
Velox or ClickHouse backend, respectively.
4. If the PR is not ready for review, please mark it as a draft.
-->
## What changes are proposed in this pull request?
### Background
Two bugs were discovered in our internal production environment: shuffle
read tasks hit an infinite loop or deserialization failures with Celeborn
(rss_sort + hash partitioning).
### What
1. EOS mid-page infinite loop. GlutenByteInputStream::readBytes drives
next(true) in a for(;;) loop, but VeloxInputStream::next() ignored the
throwIfPastEnd argument and silently returned on EOS — a truncated partition
stream made the loop spin forever (task stuck in RUNNABLE). The Read() result
was also stored into an unsigned offset_ without a signed guard, so a negative
return corrupted setRange.
Fix: next() honors throwIfPastEnd (fails fast on EOS when bytes are still
expected), guards the signed Read() result, and the constructor / hasNext()
probe with next(false) so a clean EOS on an empty stream stays graceful.
2. Multi-window page corruption. VeloxInputStream is a single-window refill
stream — each next() overwrites the sole ByteRange — but the Presto serde
assumes stable multi-range data (tellp → pre-scan → seekp back for nested
types, nextView over the payload for checksum verification). When a page spans
multiple read windows, the serde's backward seek read window data already
overwritten by a refill, corrupting the stream (e.g. Invalid serialized string
size).
Fix: VeloxInputStream overrides nextView (refills the window instead of
reporting EOS at a range boundary) and tellp/seekp (window-bounded, fail fast).
The deserializer gained readPage(): fast path deserializes in-situ when the
whole page fits in the current window; slow path reassembles cross-window pages
(copied header stitched with the in-situ payload, or a fully copied payload)
into a contiguous BufferInputStream so serde seeks never touch
refill-overwritten data.
The Presto page wire header is parsed manually (21 bytes, frozen wire
format) to avoid depending on velox-internal serde detail headers.
Regression tests in VeloxShuffleReaderTest cover both fixes, driven through
the public VeloxRssSortShuffleReaderDeserializer API.
## How was this patch tested?
<!--
Describe how the changes were tested, if applicable.
Include new tests to validate the functionality, if necessary.
For UI-related changes, attach screenshots to demonstrate the updates.
-->
## Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this
patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling
Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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]
