HIVE-29785: SkippingTextInputFormat: ClassCastException on skip.header.line.count files with lone-CR (\r) line endings - #6680
Open
srbiswal wants to merge 2 commits into
Open
HIVE-29785: SkippingTextInputFormat: ClassCastException on skip.header.line.count files with lone-CR (\r) line endings#6680srbiswal wants to merge 2 commits into
srbiswal wants to merge 2 commits into
Conversation
…r.line.count files with lone-CR (\r) line endings SkippingTextInputFormat.getCachedStartIndex (header path) and getCachedEndIndex (footer path) called FSDataInputStream.readLine() and then getPos() on the same stream. On a lone '\r' not followed by '\n', DataInputStream.readLine() pushes its look-ahead byte back by replacing the stream's inner input with a non-Seekable PushbackInputStream, so the subsequent getPos() throws ClassCastException during Tez split generation. LF, CRLF and '\r'-at-EOF were unaffected. Replace readLine()+getPos() in both methods with a ByteCountingLineReader that recognizes '\n', '\r\n' and lone '\r' and derives offsets from bytes consumed, so getPos() is never called on a readLine()-mutated stream. Offset semantics are preserved exactly. As a safety net, makeSplitInternal now converts any unexpected RuntimeException from header/footer detection into a clear, file-contextual error instead of a cryptic cast. Adds tests: a lone-CR repro, exact split-offset assertions across LF/CR/CRLF proving no boundary regression, and a footer-path lone-CR case.
uros-b
approved these changes
Aug 9, 2026
deniskuzZ
reviewed
Aug 12, 2026
| return new NullRowsInputFormat.DummyInputSplit(file); | ||
| } catch (RuntimeException e) { | ||
| // Report unexpected detection failures clearly instead of a cryptic cast. | ||
| throw new RuntimeException("Failed to detect header/footer boundaries for file " |
Member
There was a problem hiding this comment.
i don't see much benefit in re-throwing same runtime exception
Author
There was a problem hiding this comment.
makes sense. removed the catch (RuntimeException)
deniskuzZ
reviewed
Aug 12, 2026
| * {@code '\r\n'} and lone {@code '\r'}; after {@link #readLine()}, | ||
| * {@link #getBytesConsumed()} is the offset just past the line's terminator. | ||
| */ | ||
| static final class ByteCountingLineReader { |
Member
There was a problem hiding this comment.
doesn't hadoop already comes with similar util:
Text headerLine = new Text();
LineReader reader = new LineReader(fis);
int bytesRead = reader.readLine(headerLine);
Author
There was a problem hiding this comment.
replaced ByteCountingLineReader with org.apache.hadoop.util.LineReader
…p redundant RuntimeException wrap Address review feedback on the lone-CR ClassCastException fix: - Replace the hand-rolled ByteCountingLineReader with Hadoop's org.apache.hadoop.util.LineReader, which counts bytes internally via readLine(Text) (no getPos()) and handles '\n', '\r\n' and lone '\r'. It is the standard utility, already used in TextRecordReader. Constructed as new LineReader(fis) so the existing textinputformat.record.delimiter index logic is preserved. - Decode the last header line as ISO-8859-1 (one char per byte) before the delimiter indexOf so the index remains a byte offset, matching the prior implementation exactly for non-ASCII header bytes. - Drop the catch (RuntimeException) re-throw in makeSplitInternal: it wrapped a RuntimeException in the same type and only added the file path, while the reader change already removes the ClassCastException it guarded against. Offset semantics unchanged; existing TestSkippingTextInputFormat and TestLineBuffer pass.
srbiswal
commented
Aug 12, 2026
srbiswal
left a comment
srbiswal
left a comment
Author
There was a problem hiding this comment.
Implemented the suggestions.
|
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
4 participants
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.
For tables with skip.header.line.count/skip.footer.line.count, SkippingTextInputFormat called readLine() then getPos() on the same FSDataInputStream (in getCachedStartIndex and getCachedEndIndex). When the data contains a lone \r (not followed by \n), DataInputStream.readLine() pushes back its look-ahead byte by wrapping the stream in a non-Seekable PushbackInputStream. The following getPos() then throws ClassCastException during Tez split generation. LF, CRLF, and \r-at-EOF were unaffected.
Replace readLine()+getPos() in both methods with a ByteCountingLineReader that handles \n, \r\n, and lone \r, computing offsets from bytes consumed, so getPos() is never called on a mutated stream. Offset behavior is unchanged. As a safety net, makeSplitInternal now turns any unexpected error from header/footer detection into a clear, file-specific message instead of a raw ClassCastException
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added unit tests in
TestSkippingTextInputFormat:testSkipFileSplitsLoneCR— reproduces theClassCastExceptionon a lone-CR file withskip.header.line.count=1(fails on master, passes with this fix).testSkipHeaderSplitOffsetsAcrossLineEndings— writes the same content with LF, lone-CR, and CRLF terminators and asserts exact splitgetStart()/getLength(), proving lone-CR matches LF and there is no boundary regression.testSkipFileSplitsLoneCRHeaderFooter— exercises the footer path (getCachedEndIndex) with lone-CR and skip header/footer, verifying the header and footer rows are skipped.