adriangb opened a new issue, #24609:
URL: https://github.com/apache/datafusion/issues/24609
### Describe the bug
`CsvScanExecNode` has no field for the CSV line terminator, so the
`terminator` set via `CsvSource::with_terminator` (or `CsvOptions::terminator`)
is silently dropped when a physical plan is serialized with `datafusion-proto`.
The decoded `CsvSource` comes back with `terminator: None` and parses with the
default line terminator.
This is the same failure mode as `HashJoinExec::fetch` in #24165:
`CsvSource::try_to_proto` reads plan state through getters, so a field nobody
wired into the encoder is simply never written and nothing fails at compile
time. `Debug`-comparing round-trip tests do not catch it either.
Impact: a CSV source that needs a non-default terminator — e.g. `OPTIONS
('format.terminator' E'\r')`, which DataFusion supports and tests in
`datafusion/core/src/datasource/physical_plan/csv.rs` — is read with the wrong
terminator once the plan has crossed a serialization boundary.
The other scan-relevant CSV options all round-trip correctly.
`CsvSource::builder` consumes `has_header`, `delimiter`, `quote`, `terminator`,
`escape`, `comment` and `truncated_rows` (plus `newlines_in_values` for
repartitioning); `CsvScanExecNode` carries every one of them except
`terminator`.
### To Reproduce
Add a terminator to the existing round-trip test
(`datafusion/proto/tests/cases/plans/sources.rs`,
`roundtrip_csv_scan_preserves_format_options`):
```rust
Arc::new(CsvSource::new(table_schema).with_csv_options(CsvOptions {
// ... existing options ...
terminator: Some(b';'),
..Default::default()
}));
// after the round trip:
assert_eq!(csv_source.terminator(), Some(b';'));
```
On `main` this fails with `assertion `left == right` failed: left: None,
right: Some(59)`.
### Expected behavior
`terminator` survives serialization like every other CSV scan option.
### Additional context
Found while applying the destructuring convention from #24164 to
`CsvSource::try_to_proto`: an exhaustive `let Self { .. }` / `let CsvOptions {
.. }` in the encoder makes an unserialized field impossible to miss.
I have a fix ready (adds `oneof optional_terminator` as field 9 of
`CsvScanExecNode`, wires both sides, and extends the round-trip test); PR to
follow.
--
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]
