Bugfix/fix rss sort reader eos full - #12984
Draft
kecookier wants to merge 4 commits into
Draft
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
arrow::io::Readable::Read(int64_t) is pure virtual; without an override FakeInputStream is abstract and make_shared fails to compile. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…eserialization Two production issues in the rss_sort shuffle reader: 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, so a truncated partition stream made the loop spin forever (task stuck in RUNNABLE). Also, the Read() result was stored into an unsigned offset_ without a signed guard, so a negative return corrupted setRange. Fix: next() honors throwIfPastEnd (VELOX_FAIL on EOS when bytes are still expected), checks the signed Read() result, and the constructor / hasNext() probe with next(false) so a clean EOS on an empty stream is 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 now overrides nextView (refills the window instead of reporting EOS at a range boundary) and tellp/seekp (window bounded, fail fast instead of reading overwritten data). The deserializer gained readPage(): the fast path deserializes in-situ when the whole page fits in the current window; the 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. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Background
Related issue: #12985
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).
The regression tests were first submitted in #12983, where the failing cases confirm that both bugs reproduce on current main — hence this PR adds the fix on top.
What
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.
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?
Add unit-test.
Was this patch authored or co-authored using generative AI tooling?
Co-Authored-By: Claude noreply@anthropic.com