[format] Escape the escape character when writing CSV - #9278
Open
PDGGK wants to merge 1 commit into
Open
Conversation
CsvFormatWriter neither quotes a field because it contains the escape character nor doubles it, but CsvParser consumes an escape character unless it is followed by a quote or another escape. A value carrying one therefore does not survive a round trip through the CSV format: with the defaults, Special\Characters reads back as SpecialCharacters and a,b\ reads back as null. Quote such a field and escape the escape before the quotes -- doing it the other way would double the escapes just inserted for them. testCsvEscapeCharacterWriteRead already wrote one of these values but asserted only its int column; its string assertions are added here.
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
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.
Purpose
The CSV writer and the CSV reader disagree about the escape character, so a value containing one does not survive a round trip through Paimon's own CSV format. With default options (
csv.escape-character = \) — written, then read back:Special\CharactersSpecialCharacterstrailing\trailinga,b\\\doubledoubleNothing is logged and nothing throws.
CsvFormatWriter.escapeFielddecides whether a field needs quoting, and escapes the quote character inside it — but the escape character is in neither step:CsvParserconsumes an escape character unconditionally — it is only ever appended to the buffer when the character after it is a quote or another escape:So each row above fails a little differently:
Special\Charactersandtrailing\are written verbatim, because a lone escape character does not trigger quoting. The reader then drops it.\\doubleis written verbatim too, and both escape characters are lost — at the start of a fieldinQuotes || inFieldis still false, so the reader does not even take the "escaped escape" branch.a,b\does get quoted, for the comma. It is written"a,b\", the trailing\is not doubled, and the reader takes the closing quote to be an escaped literal. The field never terminates and the row comes back with a null.Why the existing test is green
testCsvEscapeCharacterWriteReadwrites exactly this value and then does not look at it:Three rows are written, and only the middle one —
"Normal Value", which contains nothing that needs escaping — has its string asserted. Rows 1 and 3, the two carrying quotes and a backslash, are checked on theirintcolumn alone. This PR adds the two missing assertions; with the writer unchanged, the existing test then fails.What changes
Writer only. Quote a field that contains the escape character, and escape the escape character before the quotes:
The order matters: escaping the quotes first would then double the escape characters that step had just inserted, and the reader would decode
\\"as a literal backslash followed by an unescaped quote.escapablekeeps an emptycsv.escape-characterbehaving as it does today —field.replace("", ...)inserts between every character, so the substitution has to be skipped rather than run with an empty needle.The reader is left alone. Once a field containing the escape character is quoted,
inQuotesis true by the time the reader reaches it, so it takes the branch that already works; the five cases in the new test all round trip without touchingCsvParser. Reading a lone escape character in a CSV file that Paimon did not write still drops it, which is the same behaviour as before this change.Blast radius
Only fields that contain the escape character are written differently; every other field is byte-identical. A
csv.escape-characterset to a character that appears often in ordinary data — the/thattestCsvEscapeCharacterWriteReadalso exercises, say — will now see those fields quoted and the character doubled. That is the point: today they come back with the character missing.Worth stating plainly: CSV files already written still contain the unescaped form and still read back short. This change stops new files from being written that way; it cannot repair existing ones.
Test evidence
testFieldsContainingTheEscapeCharacterRoundTripwrites five values —Special\Characters,trailing\,a,b\,\\double,\"quoteAfterEscape— and asserts each comes back equal to what went in, and non-null.Mutation control, on a forced clean rebuild of
paimon-format(rm -rf target/classes target/test-classes) so this is not an incremental-build artefact: with the tests kept andCsvFormatWriterreverted, two fail — the new one, andtestCsvEscapeCharacterWriteReadwith its restored assertions:CsvFileFormatTest— 28 tests, and the whole ofpaimon-format— 551 tests, 0 failures.API and Format
No change to any option or public signature. The on-disk bytes change only for fields containing the escape character, which are the fields that currently do not survive being read back.