Skip to content

[format] Reject csv values containing a row separator - #9938

Open
jackylee-ch wants to merge 1 commit into
apache:masterfrom
jackylee-ch:csv-reject-line-break-in-value
Open

jackylee-ch wants to merge 1 commit into
apache:masterfrom
jackylee-ch:csv-reject-line-break-in-value

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

Purpose

CsvFormatWriter quoted a value containing the row separator, but neither line reader tracks quotes: StandardLineReader splits on CR/LF and CustomLineReader is a leftmost-match KMP over the delimiter bytes. CsvParser.parse takes one line at a time with no continuation state, so (1, "hello\nworld") came back as (1, null) and (null, null)COUNT(*) changed. Quoting cannot fix it without giving up splittability: a split boundary may fall inside the value.

The writer now refuses such a value, naming the column. CR/LF are rejected only when they are the separator — under a custom csv.line-delimiter they are ordinary bytes, the documented way to carry a line break inside a value, and that still round-trips.

needsQuoting still quotes a value that merely begins a delimiter match, since leftmost-match means the appended delimiter would complete a match started by the value's trailing bytes. escapable is gone: since #9904 the escape character is exactly one character.

Set csv.line-delimiter to a sequence absent from the data, or strip the separator upstream.

Tests

CsvFileFormatTest#testValueContainingRowSeparatorIsRejected. paimon-format: 658 run, 0 failures.

Written with Claude Code; verification is mine.

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction is right: the value silently corrupting row counts is worse than a write-time error, and quoting can't fix it without giving up splittability, so rejecting with the column named is the honest option. The escape hatch via a custom csv.line-delimiter (under which CR/LF stay ordinary bytes) is well covered by the tests, and the leftmost-match quoting subtlety (x| must still be quoted under delimiter |||) is a nice catch.

Two things before this can go in:

  1. CI: Java / E2E / Flink 1 and Spark interoperability failed after 25 seconds (the same job takes ~20 min on other PRs, and Java / Core and integrations passes with this change), so it looks like an infra flake. Could you rerun it?

  2. Docs: the write-side behavior changed — a value containing the row separator is now rejected — but docs/docs/concepts/spec/fileformat.md (the CSV options table) doesn't mention it. A sentence on the csv.line-delimiter row like "values containing the line delimiter are rejected at write time; set a delimiter absent from the data to carry line breaks inside values" would save users the surprise of a previously-working write starting to fail.

Also worth a line in the PR/commit message for release notes: this is intentionally a breaking change for pipelines that already write such values (they were producing corrupt files), so the failure at write time is the migration signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants