szehon-ho commented on code in PR #57608:
URL: https://github.com/apache/spark/pull/57608#discussion_r3708337171
##########
docs/sql-data-sources-csv.md:
##########
@@ -207,7 +207,7 @@ Data source options of CSV can be set via:
<tr>
<td><code>mode</code></td>
<td>PERMISSIVE</td>
- <td>Allows a mode for dealing with corrupt records during parsing. It
supports the following case-insensitive modes. Note that Spark tries to parse
only required columns in CSV under column pruning. Therefore, corrupt records
can be different based on required set of fields. This behavior can be
controlled by <code>spark.sql.csv.parser.columnPruning.enabled</code> (enabled
by default).<br>
+ <td>Allows a mode for dealing with corrupt records during parsing. It
supports the following case-insensitive modes. Note that Spark tries to parse
only required columns in CSV under column pruning. Therefore, corrupt records
can be different based on required set of fields. This behavior can be
controlled by <code>spark.sql.csv.parser.columnPruning.enabled</code> (enabled
by default). In particular, when <code>multiLine</code> is disabled, a quoted
value that contains a line break is split, which increases the record count,
and some or all of the resulting records may be malformed — typically when the
token count no longer matches the schema. <code>mode</code> then applies to
each record independently, so a half whose token count happens to match the
schema is retained as a valid row even though its value was truncated,
including under <code>DROPMALFORMED</code>, which drops only the malformed
halves. An action that requires no columns (for example a bare
<code>count()</code>
) may surface none of this because of column pruning.<br>
Review Comment:
Verified the softened claim against the parser and it holds:
`UnivocityParser.convert` flags a record when `tokens.length !=
parsedSchema.length` (and on per-value conversion failures), and nothing there
fails on an unclosed quote. The `count()` caveat checks out too, since with
column pruning and an empty required schema `parse` returns
`Some(InternalRow.empty)` without ever reaching `convert`, so not even
`FAILFAST` sees the split.
Four things on this row:
**1. The new sentence contradicts the `PERMISSIVE` bullet two lines below.**
The added text says records are malformed "typically when the token count no
longer matches the schema", while the bullet still says "A record with
less/more tokens than schema is not a corrupted record to CSV." Those cannot
both be true, and the reader now hits the contradiction inside a single table
cell. The parser sides with your new sentence: a token-count mismatch throws
`BadRecordException`, which `FailureSafeParser` turns into a corrupt-record row
under `PERMISSIVE`, an empty iterator under `DROPMALFORMED`, and a raised error
under `FAILFAST`. The old bullet is simply wrong, and predates this PR. #57658
already edits that bullet but leaves this sentence untouched, so folding the
correction in there looks like the natural home. Otherwise this PR lands text
that reads as self-contradictory.
**2. "a half" and "the malformed halves" have no antecedent.** This row
introduces the split only as "is split", never as a split into two, so both
references dangle and only parse for someone who has already read the
`multiLine` row.
**3. "which increases the record count" is stated unconditionally**, and
sits ahead of the sentence explaining that `mode` is applied afterward, so it
reads as a guarantee. Under `DROPMALFORMED` with both halves malformed the
count goes down relative to the source.
**4. Punctuation nit:** the em dash is the only non-ASCII character the PR
introduces, and this file is currently pure ASCII. No need for a substitute
like `--` either (rare in `docs/sql-*.md`, and absent from the option tables)
since a comma or a sentence break reads more naturally here.
The `DROPMALFORMED` inversion you added is the sharpest part of this and
worth keeping. Something along these lines covers the same ground and addresses
all four points:
```
In particular, when <code>multiLine</code> is disabled, a quoted value
containing a line break splits one source record into two before
<code>mode</code> is applied. Each of the two is then evaluated on its own, so
a record whose token count happens to match the schema is retained as valid
even though its value was truncated. This holds under
<code>DROPMALFORMED</code> as well, since that mode drops only the records it
finds malformed. An action requiring no columns (a bare <code>count()</code>,
for example) may surface none of this because of column pruning.
```
##########
docs/sql-data-sources-csv.md:
##########
@@ -231,7 +231,8 @@ Data source options of CSV can be set via:
<tr>
<td><code>multiLine</code></td>
<td>false</td>
- <td>Allows a row to span multiple lines, by parsing line breaks within
quoted values as part of the value itself. CSV built-in functions ignore this
option.</td>
+ <td>Allows a row to span multiple lines, by parsing line breaks within
quoted values as part of the value itself. CSV built-in functions ignore this
option.<br>
+ When this option is disabled (the default), a line break inside a quoted
value terminates the record at that break: the value is truncated, and the rest
of the value begins a new record. Either or both of the resulting records may
then be malformed (for example when the token count no longer matches the
schema); how they are handled is controlled by <code>mode</code>.</td>
Review Comment:
This row reads well now, and I confirmed the mechanism behind the correction
in the source: `UnivocityParser.convert` treats a record as bad when
`tokens.length != parsedSchema.length` or when a value conversion fails, and
there is no check that fails on an unclosed quote. So "either or both of the
resulting records may be malformed (for example when the token count no longer
matches the schema)" matches the parser, and dropping the "remaining fields set
to `null`" claim was right, since that only held when the leading half came up
short on tokens.
Nothing further from me on this row.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
