PDGGK opened a new pull request, #9278:
URL: https://github.com/apache/paimon/pull/9278
### 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:
| written | read back |
|---|---|
| `Special\Characters` | `SpecialCharacters` |
| `trailing\` | `trailing` |
| `a,b\` | **null** |
| `\\double` | `double` |
Nothing is logged and nothing throws.
`CsvFormatWriter.escapeField` decides whether a field needs quoting, and
escapes the quote character inside it — but the escape character is in neither
step:
```java
// CsvFormatWriter:103-116
boolean needsQuoting =
field.indexOf(csvOptions.fieldDelimiter().charAt(0)) >= 0
|| field.indexOf(csvOptions.lineDelimiter().charAt(0)) >= 0
|| field.indexOf(csvOptions.quoteCharacter().charAt(0)) >= 0;
if (!needsQuoting) {
return field;
}
String escaped =
field.replace(
csvOptions.quoteCharacter(),
csvOptions.escapeCharacter() + csvOptions.quoteCharacter());
return csvOptions.quoteCharacter() + escaped + csvOptions.quoteCharacter();
```
`CsvParser` consumes an escape character unconditionally — it is only ever
appended to the buffer when the character after it is a quote or another escape:
```java
// CsvParser:108-118
if (c == escapeChar) {
if (inQuotes || inField) {
int nextCharacter = peekNextCharacter(line, position);
if (nextCharacter == quoteChar || nextCharacter == escapeChar) {
buffer.append(line.charAt(position + 1));
position++;
}
}
} else if (c == quoteChar) {
```
So each row above fails a little differently:
* `Special\Characters` and `trailing\` are written verbatim, because a lone
escape character does not trigger quoting. The reader then drops it.
* `\\double` is written verbatim too, and both escape characters are lost —
at the start of a field `inQuotes || inField` is 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
`testCsvEscapeCharacterWriteRead` writes exactly this value and then does
not look at it:
```java
GenericRow.of(3, BinaryString.fromString("Special\\Characters"))
...
assertThat(result.get(2).getInt(0)).isEqualTo(3);
```
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 their `int` column
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:
```java
String quote = csvOptions.quoteCharacter();
String escape = csvOptions.escapeCharacter();
boolean escapable = !escape.isEmpty();
boolean needsQuoting =
field.indexOf(csvOptions.fieldDelimiter().charAt(0)) >= 0
|| field.indexOf(csvOptions.lineDelimiter().charAt(0)) >= 0
|| field.indexOf(quote.charAt(0)) >= 0
|| (escapable && field.indexOf(escape.charAt(0)) >= 0);
if (!needsQuoting) {
return field;
}
String escaped = escapable ? field.replace(escape, escape + escape) : field;
return quote + escaped.replace(quote, escape + quote) + quote;
```
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.
`escapable` keeps an empty `csv.escape-character` behaving 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, `inQuotes` is 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 touching `CsvParser`. 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-character` set to a character that
appears often in ordinary data — the `/` that `testCsvEscapeCharacterWriteRead`
also 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
`testFieldsContainingTheEscapeCharacterRoundTrip` writes 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 and `CsvFormatWriter` reverted, **two fail** —
the new one, and `testCsvEscapeCharacterWriteRead` with its restored assertions:
```
CsvFileFormatTest.testCsvEscapeCharacterWriteRead:378
expected: "Special\Characters"
but was: "SpecialCharacters"
```
`CsvFileFormatTest` — 28 tests, and the whole of `paimon-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.
--
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]
