fix(proto): preserve CSV/JSON scan options on roundtrip - #24233
fix(proto): preserve CSV/JSON scan options on roundtrip#24233buraksenn wants to merge 8 commits into
Conversation
github-actions
Bot
added
proto
…json-scan-options # Conflicts: # datafusion/proto/tests/cases/roundtrip_physical_plan.rs
|
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24233 +/- ##
==========================================
- Coverage 81.35% 81.34% -0.01%
==========================================
Files 1117 1117
Lines 397457 397554 +97
Branches 397457 397554 +97
==========================================
+ Hits 323337 323385 +48
- Misses 55208 55253 +45
- Partials 18912 18916 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…json-scan-options
…json-scan-options
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The overall approach looks good, and preserving these scan options through physical-plan protobuf serialization makes sense.
I found one issue with the CSV terminator representation that I think needs to be addressed before merging. I also left one non-blocking suggestion to strengthen the roundtrip coverage.
| bool newlines_in_values = 7; | ||
| bool truncate_rows = 8; | ||
| // Custom line terminator. Absent means the default newline terminator. | ||
| optional string terminator = 9; |
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks for the review and suggestion @kosiew. I've applied it with the additional test case below
| } | ||
|
|
||
| #[test] | ||
| fn roundtrip_csv_scan_preserves_format_options() -> Result<()> { |
There was a problem hiding this 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.
There was a problem hiding this comment.
I've added the test thanks
02ede80 to
f67bca2
Compare
There was a problem hiding this comment.
Thanks for the follow-up. The issues from the previous review look addressed.
The CSV terminator is now represented as bytes, so arbitrary one-byte values such as 0xFF can roundtrip correctly. The decoder also rejects zero-byte and multi-byte terminators, and the new tests cover both the valid and invalid cases.
The end-to-end compressed scan suggestion is addressed as well. The new gzip JSON-array test serializes and deserializes the physical plan, then executes it successfully, which verifies that the compression and JSON format options make it through to the reader path.
I did not find any new correctness issues in the follow-up changes.
Thanks for addressing the feedback. Looks good to me.
3 participants
Which issue does this PR close?
Rationale for this change
Physical-plan protobuf serialization does not preserve several CSV and JSON scan options. Custom CSV terminators, JSON newline-delimited mode, and file compression therefore revert to their defaults after a roundtrip, which can cause the
decoded plan to read the file incorrectly.
What changes are included in this PR?
terminatortoCsvScanExecNode.newline_delimitedtoJsonScanExecNode.file_compression_typeto the sharedFileScanExecConf.Are these changes tested?
Yes. Extended the CSV and JSON physical-plan roundtrip tests to cover custom terminators, non-newline-delimited JSON, compression, and backward-compatible defaults.
Are there any user-facing changes?
CSV and JSON scans now preserve their format and compression options across protobuf roundtrips. The protobuf changes are additive and backward compatible.
JsonSourcealso gains anis_newline_delimitedgetter.