kumarUjjawal commented on code in PR #25058:
URL: https://github.com/apache/datafusion/pull/25058#discussion_r4022609965
##########
datafusion/proto-common/src/to_proto/mod.rs:
##########
@@ -872,10 +873,46 @@ impl TryFrom<&CsvWriterOptions> for
protobuf::CsvWriterOptions {
type Error = DataFusionError;
fn try_from(opts: &CsvWriterOptions) -> datafusion_common::Result<Self,
Self::Error> {
- Ok(csv_writer_options_to_proto(
- &opts.writer_options,
- &opts.compression,
- ))
+ let CsvWriterOptions {
+ writer_options,
+ compression,
+ compression_level,
+ } = opts;
+ let compression: protobuf::CompressionTypeVariant = compression.into();
+ let quote_style: protobuf::CsvQuoteStyle =
writer_options.quote_style().into();
+ let terminator = match writer_options.line_terminator() {
+ Terminator::CRLF => b"\r\n".to_vec(),
+ Terminator::Any(byte) => vec![*byte],
+ };
+
+ Ok(protobuf::CsvWriterOptions {
+ compression: compression.into(),
+ delimiter: (writer_options.delimiter() as char).to_string(),
+ has_header: writer_options.header(),
+ date_format:
writer_options.date_format().unwrap_or_default().to_owned(),
Review Comment:
`unwrap_or_default()` encodes both None and `Some("")` as the same protobuf
value, while the decoder now interprets that value as `None`. An explicit empty
date/time format is valid and makes Arrow write an empty value; after a plan
round trip it reverts to the default format and changes CSV output. Please use
presence-aware fields, such as optional string, for all five optional formats
and test both None and `Some("")`.
##########
datafusion/proto-common/proto/datafusion_common.proto:
##########
@@ -471,6 +471,12 @@ message CsvWriterOptions {
bool ignore_leading_whitespace = 13;
// Whether to ignore trailing whitespace in string values
bool ignore_trailing_whitespace = 14;
+ // Optional compression level
+ optional uint32 compression_level = 15;
Review Comment:
These additions are wire-compatible, but they break exhaustive
CsvWriterOptions struct literals in both datafusion-proto-common and
datafusion-proto-models.
Please a DataFusion 56.0 upgrade-guide entry with before/after construction
examples, matching the adjacent JsonWriterOptions precedent from PR #24945.
--
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]
