adriangb opened a new issue, #24180:
URL: https://github.com/apache/datafusion/issues/24180
### Describe the bug
The physical plan protobuf encoding for CSV and JSON scans silently drops
several
scan options. They are not represented in `CsvScanExecNode` /
`JsonScanExecNode`
(or in the shared `FileScanExecConf`), so encoding succeeds without error
and the
decoded plan reads the data differently from the original:
| Option | Encoded? | Decodes as |
| --- | --- | --- |
| `JsonSource::newline_delimited` | no | always `true` (NDJSON) |
| `CsvSource::terminator` | no | `None` (default `\n`) |
| `FileScanConfig::file_compression_type` (CSV, JSON) | no | `UNCOMPRESSED` |
For `newline_delimited` and `file_compression_type` this is a correctness
problem,
not just a lost setting: a JSON-array file decoded as NDJSON, or a gzipped
file
decoded as uncompressed, produces wrong results or a parse error on the
remote
node rather than a serialization failure at the sender.
This is pre-existing behavior, not a regression. It was surfaced while
migrating
these sources onto the per-plan proto hooks (#23494), where it is now
recorded in
doc comments on `CsvSource::try_from_proto` and `JsonSource::try_from_proto`
(#24177, #24178).
### To Reproduce
Round-trip a `DataSourceExec` through `PhysicalPlanNode` encode/decode with
any of:
```rust
JsonSource::new(schema).with_newline_delimited(false)
CsvSource::new(schema).with_terminator(Some(b'#'))
FileScanConfigBuilder::new(url,
source).with_file_compression_type(FileCompressionType::GZIP)
```
Each round-trips back to the default value.
### Expected behavior
Either of:
1. Add the missing fields to the proto messages (`optional bool
newline_delimited`
on `JsonScanExecNode`, `optional string terminator` on `CsvScanExecNode`,
and a
compression field for CSV/JSON) and encode/decode them. Making them
optional
keeps the wire format backward compatible: an absent field decodes to
today's
default.
2. At minimum, return an error from `try_to_proto` when a non-default value
is set,
so the loss is loud instead of silent.
(1) is preferable — the options are cheap to encode and (2) turns a working
local
plan into a hard failure the moment it is serialized.
### Additional context
Related: #23494 (proto hook migration EPIC), #23516, #24177, #24178.
--
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]
