Skip to content

Commit 75ddead

Browse files
authored
Deprecate CsvSource::newlines_in_values and preserve proto coverage (#26179)
## Which issue does this PR close? - Part of #24612 ## Rationale for this change Deprecate `CsvSource::newlines_in_values()` as part of the CSV API cleanup while preserving compatibility and existing CSV serialization behavior. The getter simply returns `CsvOptions.newlines_in_values.unwrap_or(false)`. There is no replacement getter, so callers should retain their original `CsvOptions` when they need to inspect this setting. The public getter remains callable during the deprecation window, and CSV reading and repartitioning behavior are unchanged. ## What changes are included in this PR? - Mark `CsvSource::newlines_in_values()` as deprecated starting in 56.0.0, with an accurate migration note. - Replace the internal encoder's getter call with `self.options.newlines_in_values.unwrap_or(false)`, preserving the existing boolean mapping. - Extend `roundtrip_csv_scan_preserves_format_options` to cover `Some(true)`, `Some(false)`, and `None`. - Assert the protobuf `CsvScan.newlines_in_values` field after round-tripping the physical plan, while retaining all existing CSV format-option assertions. - Preserve malformed terminator validation for empty and two-byte terminators. - Document the deprecation and migration guidance in the 56.0.0 upgrade guide. No protobuf wire-format changes or new public APIs are introduced. ## Are these changes tested? Extended `roundtrip_csv_scan_preserves_format_options` to verify: - `Some(true)` serializes as `true`. - `Some(false)` serializes as `false`. - `None` serializes as `false`. - Existing CSV format options, including delimiter, quote, escape, comment, terminator, truncated rows, header, and GZIP compression, survive the round-trip. - Invalid zero-byte and two-byte terminators continue to produce decoding errors. The patch includes these regression tests, but does not establish whether they have been executed successfully. Recommended verification commands: ```bash cargo test -p datafusion-proto --test proto_integration roundtrip_csv_scan_preserves_format_options cargo test -p datafusion-datasource-csv cargo fmt --all cargo clippy --all-targets --all-features -- -D warnings ./ci/scripts/doc_prettier_check.sh --write --allow-dirty ``` ## Are there any user-facing changes? `CsvSource::newlines_in_values()` is deprecated starting in 56.0.0 but remains callable with unchanged behavior. Existing callers may receive deprecation warnings, potentially affecting downstream builds that treat warnings as errors. There is no replacement getter. Callers should retain the `CsvOptions` used to configure the source and read `options.newlines_in_values.unwrap_or(false)`. The 56.0.0 upgrade guide documents this migration. CSV parsing, repartitioning, and protobuf serialization behavior are unchanged. ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.
1 parent a4b8198 commit 75ddead

3 files changed

Lines changed: 97 additions & 66 deletions

File tree

‎datafusion/datasource-csv/src/source.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,10 @@ impl CsvSource {
174174
}
175175

176176
/// Whether values may contain newline characters
177+
#[deprecated(
178+
since = "56.0.0",
179+
note = "No replacement getter exists. Retain CsvOptions and read its newlines_in_values field with unwrap_or(false)."
180+
)]
177181
pub fn newlines_in_values(&self) -> bool {
178182
self.options.newlines_in_values.unwrap_or(false)
179183
}
@@ -354,7 +358,7 @@ impl FileSource for CsvSource {
354358
)
355359
})
356360
.transpose()?,
357-
newlines_in_values: self.newlines_in_values(),
361+
newlines_in_values: self.options.newlines_in_values.unwrap_or(false),
358362
truncate_rows: self.truncate_rows(),
359363
terminator: self.terminator().map(|terminator| vec![terminator]),
360364
};

‎datafusion/proto/tests/cases/plans/sources.rs‎

Lines changed: 78 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -535,76 +535,89 @@ fn roundtrip_csv_scan_preserves_format_options() -> Result<()> {
535535

536536
let file_schema =
537537
Arc::new(Schema::new(vec![Field::new("col", DataType::Utf8, false)]));
538-
let table_schema = TableSchema::from(&file_schema);
539-
let file_source =
540-
Arc::new(CsvSource::new(table_schema).with_csv_options(CsvOptions {
541-
has_header: Some(false),
542-
delimiter: b'|',
543-
quote: b'\'',
544-
escape: Some(b'\\'),
545-
comment: Some(b'#'),
546-
terminator: Some(0xff),
547-
newlines_in_values: Some(true),
548-
truncated_rows: Some(true),
549-
..Default::default()
550-
}));
551-
552-
let scan_config =
553-
FileScanConfigBuilder::new(ObjectStoreUrl::local_filesystem(), file_source)
554-
.with_file_groups(vec![FileGroup::new(vec![PartitionedFile::new(
555-
"/path/to/file.csv.gz".to_string(),
556-
1024,
557-
)])])
558-
.with_file_compression_type(FileCompressionType::GZIP)
559-
.build();
560-
561-
let ctx = SessionContext::new();
562-
let codec = DefaultPhysicalExtensionCodec {};
563-
let plan: Arc<dyn ExecutionPlan> = DataSourceExec::from_data_source(scan_config);
564-
let roundtripped = roundtrip_test_and_return(
565-
Arc::clone(&plan),
566-
&ctx,
567-
&codec,
568-
&DefaultPhysicalProtoConverter {},
569-
)?;
570-
let data_source = roundtripped
571-
.downcast_ref::<DataSourceExec>()
572-
.ok_or_else(|| internal_datafusion_err!("Expected DataSourceExec"))?;
573-
let file_scan = data_source
574-
.data_source()
575-
.downcast_ref::<FileScanConfig>()
576-
.ok_or_else(|| internal_datafusion_err!("Expected FileScanConfig"))?;
577-
let csv_source = file_scan
578-
.file_source()
579-
.downcast_ref::<CsvSource>()
580-
.ok_or_else(|| internal_datafusion_err!("Expected CsvSource"))?;
581-
582-
assert!(!csv_source.has_header());
583-
assert_eq!(csv_source.delimiter(), b'|');
584-
assert_eq!(csv_source.quote(), b'\'');
585-
assert_eq!(csv_source.escape(), Some(b'\\'));
586-
assert_eq!(csv_source.comment(), Some(b'#'));
587-
assert_eq!(csv_source.terminator(), Some(0xff));
588-
assert!(csv_source.newlines_in_values());
589-
assert!(csv_source.truncate_rows());
590-
assert_eq!(file_scan.file_compression_type, FileCompressionType::GZIP);
538+
for newlines_in_values in [Some(true), Some(false), None] {
539+
let table_schema = TableSchema::from(&file_schema);
540+
let file_source =
541+
Arc::new(CsvSource::new(table_schema).with_csv_options(CsvOptions {
542+
has_header: Some(false),
543+
delimiter: b'|',
544+
quote: b'\'',
545+
escape: Some(b'\\'),
546+
comment: Some(b'#'),
547+
terminator: Some(0xff),
548+
newlines_in_values,
549+
truncated_rows: Some(true),
550+
..Default::default()
551+
}));
552+
553+
let scan_config =
554+
FileScanConfigBuilder::new(ObjectStoreUrl::local_filesystem(), file_source)
555+
.with_file_groups(vec![FileGroup::new(vec![PartitionedFile::new(
556+
"/path/to/file.csv.gz".to_string(),
557+
1024,
558+
)])])
559+
.with_file_compression_type(FileCompressionType::GZIP)
560+
.build();
591561

592-
for invalid_terminator in [vec![], vec![b'\r', b'\n']] {
593-
let mut node =
594-
PhysicalPlanNode::try_from_physical_plan(Arc::clone(&plan), &codec)?;
595-
match node.physical_plan_type.as_mut() {
562+
let ctx = SessionContext::new();
563+
let codec = DefaultPhysicalExtensionCodec {};
564+
let plan: Arc<dyn ExecutionPlan> = DataSourceExec::from_data_source(scan_config);
565+
let roundtripped = roundtrip_test_and_return(
566+
Arc::clone(&plan),
567+
&ctx,
568+
&codec,
569+
&DefaultPhysicalProtoConverter {},
570+
)?;
571+
let data_source = roundtripped
572+
.downcast_ref::<DataSourceExec>()
573+
.ok_or_else(|| internal_datafusion_err!("Expected DataSourceExec"))?;
574+
let file_scan = data_source
575+
.data_source()
576+
.downcast_ref::<FileScanConfig>()
577+
.ok_or_else(|| internal_datafusion_err!("Expected FileScanConfig"))?;
578+
let csv_source = file_scan
579+
.file_source()
580+
.downcast_ref::<CsvSource>()
581+
.ok_or_else(|| internal_datafusion_err!("Expected CsvSource"))?;
582+
583+
assert!(!csv_source.has_header());
584+
assert_eq!(csv_source.delimiter(), b'|');
585+
assert_eq!(csv_source.quote(), b'\'');
586+
assert_eq!(csv_source.escape(), Some(b'\\'));
587+
assert_eq!(csv_source.comment(), Some(b'#'));
588+
assert_eq!(csv_source.terminator(), Some(0xff));
589+
let node =
590+
PhysicalPlanNode::try_from_physical_plan(Arc::clone(&roundtripped), &codec)?;
591+
match node.physical_plan_type {
596592
Some(protobuf::physical_plan_node::PhysicalPlanType::CsvScan(scan)) => {
597-
scan.terminator = Some(invalid_terminator);
593+
assert_eq!(scan.newlines_in_values, newlines_in_values.unwrap_or(false));
598594
}
599595
other => return internal_err!("Expected CsvScan node, got {other:?}"),
600596
}
601-
let err = node
602-
.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)
603-
.expect_err("invalid terminator length must fail");
604-
assert!(
605-
err.to_string().contains("expected exactly one byte"),
606-
"unexpected error: {err}"
607-
);
597+
assert!(csv_source.truncate_rows());
598+
assert_eq!(file_scan.file_compression_type, FileCompressionType::GZIP);
599+
600+
// Terminator validation is independent of the newline setting.
601+
if newlines_in_values != Some(true) {
602+
continue;
603+
}
604+
for invalid_terminator in [vec![], vec![b'\r', b'\n']] {
605+
let mut node =
606+
PhysicalPlanNode::try_from_physical_plan(Arc::clone(&plan), &codec)?;
607+
match node.physical_plan_type.as_mut() {
608+
Some(protobuf::physical_plan_node::PhysicalPlanType::CsvScan(scan)) => {
609+
scan.terminator = Some(invalid_terminator);
610+
}
611+
other => return internal_err!("Expected CsvScan node, got {other:?}"),
612+
}
613+
let err = node
614+
.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)
615+
.expect_err("invalid terminator length must fail");
616+
assert!(
617+
err.to_string().contains("expected exactly one byte"),
618+
"unexpected error: {err}"
619+
);
620+
}
608621
}
609622
Ok(())
610623
}

‎docs/source/library-user-guide/upgrading/56.0.0.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,20 @@ change. Use `FieldMetadata::to_hashmap` where a `HashMap` is still required.
115115

116116
[`metadata`]: https://docs.rs/arrow/60.0.0/arrow/datatypes/struct.Metadata.html
117117

118+
### `CsvSource::newlines_in_values` is deprecated
119+
120+
`CsvSource::newlines_in_values()` is deprecated starting in 56.0.0 but remains
121+
callable with unchanged behavior. There is no replacement getter, and
122+
`CsvSource` does not expose a `csv_options()` method.
123+
124+
**Migration guide:**
125+
126+
Retain the `CsvOptions` used to configure the source before passing them to
127+
`CsvSource::with_csv_options`. Read `options.newlines_in_values.unwrap_or(false)`
128+
to obtain the same boolean value: `None` and `Some(false)` mean false, while
129+
`Some(true)` means true. CSV reading, repartitioning, and serialization behavior
130+
are unchanged.
131+
118132
### `ParquetAccessPlan::into_overall_row_selection` is deprecated
119133

120134
`ParquetAccessPlan::into_overall_row_selection` is deprecated starting in 56.0.0.

0 commit comments

Comments
 (0)