Repository navigation
Deprecate CsvSource::newlines_in_values and preserve proto coverage - #26179
Merged
Merged
Conversation
…de guide - Getter deprecated since 56.0.0; accurate migration note. - Encoder uses options directly. - Proto coverage: true/false/unset; existing assertions retained. - Upgrade guide updated.
…checks once, retain all three positive roundtrips. - terminator checks now run only once. - all three positive roundtrips are retained.
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
1 of 5 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26179 +/- ##
==========================================
- Coverage 82.77% 82.77% -0.01%
==========================================
Files 1148 1148
Lines 451075 451075
Branches 451075 451075
==========================================
- Hits 373397 373387 -10
- Misses 54930 54940 +10
Partials 22748 22748 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
marked this pull request as ready for review
October 10, 2026 10:58
Contributor
Author
|
🚀 |
Omega359
pushed a commit
to Omega359/arrow-datafusion
that referenced
this pull request
Oct 11, 2026
…pache#26179) ## Which issue does this PR close? - Part of apache#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
datasourcecrates #24612Rationale 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 originalCsvOptionswhen 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?
CsvSource::newlines_in_values()as deprecated starting in 56.0.0, with an accurate migration note.self.options.newlines_in_values.unwrap_or(false), preserving the existing boolean mapping.roundtrip_csv_scan_preserves_format_optionsto coverSome(true),Some(false), andNone.CsvScan.newlines_in_valuesfield after round-tripping the physical plan, while retaining all existing CSV format-option assertions.No protobuf wire-format changes or new public APIs are introduced.
Are these changes tested?
Extended
roundtrip_csv_scan_preserves_format_optionsto verify:Some(true)serializes astrue.Some(false)serializes asfalse.Noneserializes asfalse.The patch includes these regression tests, but does not establish whether they have been executed successfully.
Recommended verification commands:
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
CsvOptionsused to configure the source and readoptions.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.