kosiew commented on code in PR #24233:
URL: https://github.com/apache/datafusion/pull/24233#discussion_r3821188325
##########
datafusion/proto-models/proto/datafusion.proto:
##########
@@ -1291,10 +1294,14 @@ message CsvScanExecNode {
}
bool newlines_in_values = 7;
bool truncate_rows = 8;
+ // Custom line terminator. Absent means the default newline terminator.
+ optional string terminator = 9;
Review Comment:
`CsvOptions::terminator` is an unrestricted `Option<u8>`, but using a
protobuf `string` here only works for bytes that can be represented as
single-byte UTF-8 values. For example, a valid terminator such as `0xFF` will
fail in `CsvSource::try_to_proto` when `proto_byte_to_string` tries to convert
it to a string.
Could we use an optional `bytes` field instead, validate that it contains
exactly one byte when decoding, and add a roundtrip test with a non-UTF-8
terminator such as `0xFF`?
Since this is a new field, changing its type should not affect existing
payloads. Both protobuf `string` and `bytes` are length-delimited on the wire.
##########
datafusion/proto/tests/cases/plans/sources.rs:
##########
Review Comment:
The roundtrip tests do a good job of checking that the reconstructed
configuration is preserved. It might also be useful to add a small end-to-end
compressed CSV or JSON scan after serialization and deserialization. That would
verify that the preserved compression and format options actually make it
through to the reader path.
This is just a suggestion and not blocking.
--
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]
