EPIC: destructure proto serde hooks in the `datasource` crates
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
## Background
#24164 established a convention for the per-type proto hooks introduced by EPIC #23494: `try_to_proto` starts with an exhaustive `let Self { .. }` (no `..`), and `try_from_proto` destructures the prost-generated node the same way. Adding a field to the struct then becomes a compile error in the encoder, and adding a field to `datafusion.proto` becomes a compile error in every decoder.
That PR applied the convention to the join plans, and the rest of the `physical-plan` plan nodes follow it today. **The `datasource*` crates were never converted**: every `FileSource` and `DataSink` hook still reads `self.field` or a getter.
## Why this matters
A hook that reads state field-by-field makes an added field invisible to serialization. Nothing breaks at compile time, the field simply stops round-tripping, and `Debug`-comparing round-trip tests do not see it. This is not hypothetical:
- `HashJoinExec::fetch` was dropped this way (fixed in #24165).
- #24609 -- `CsvSource` terminator -- was found by applying the convention to exactly one file in this scope.
- Checking each remaining un-destructured encoder against its struct's fields turned up two more, listed below.
## Inventory
18 un-destructured hooks in this EPIC's scope (9 encoders, 9 decoders) across 9 files.
| group | files | sub-issue |
|---|---|---|
| Parquet source and sink | 2 | #24620 |
| JSON source and sink | 2 | #24621 |
| Arrow and Avro sources | 2 | #24622 |
| CSV sink | 1 | #24623 |
| `FileScanConfig` and `MemorySourceConfig` | 2 | #24624 |
`FileScanConfig` (B5) is the highest-leverage one: it is the shared base message every file source embeds, so a field dropped there is dropped for all five formats at once.
## Sub-issues
- [ ] #24620 Destructure proto hooks for the Parquet source and sink -- partial fix in #24930 (merged, `reverse_row_groups` / `sort_order_for_reorder` only); the destructure itself is still open
- [ ] #24621 Destructure proto hooks for the JSON source and sink -- #24945 (open)
- [x] #24622 Destructure proto hooks for the Arrow and Avro sources -- #24696 (merged)
- [ ] #24623 Destructure proto hooks for the CSV sink
- [ ] #24624 Destructure proto hooks for FileScanConfig and MemorySourceConfig -- #24813 (open)
## Field drops already confirmed
| field | set via | effect after a round-trip |
|---|---|---|
| `CsvSource` terminator | `with_terminator` | tracked separately as #24609; fix in flight |
| `JsonSource::newline_delimited` | `with_newline_delimited(false)` | no wire field; decode defaults to NDJSON, so JSON-array input is read the wrong way |
| `ParquetSource::metadata_size_hint` | `with_metadata_size_hint` | absent from `ParquetScanExecNode` and from `TableParquetOptions` |
## Public API to consider deprecating alongside this work
Converting these hooks to destructuring orphans almost nothing by itself -- the accessors involved have other callers. The real deprecation candidates are accessors that already went dead when EPIC #23494 moved serialization in-crate, and were simply left behind.
I checked each one for downstream users two ways: GitHub code search (filtering out vendored copies of DataFusion by fetching each hit and confirming a real call site), and grepping shallow clones of datafusion-distributed, datafusion-ballista, datafusion-comet, sail, delta-rs, iceberg-rust, sedona-db and openobserve.
**Safe to deprecate -- no callers found anywhere:**
| accessor | notes |
|---|---|
| `CsvSource::newlines_in_values` | being deprecated as part of the #24609 fix |
| `AnalyzeExec::metric_categories` | plan node, outside this EPIC |
| `ExplainExec::verbose` | plan node, outside this EPIC |
**Do not deprecate -- downstream users found:**
| accessor | used by |
|---|---|
| `ParquetSource::parquet_file_reader_factory` | sedona-db, LakeSoul, OpenSearch analytics plugin, signaldb, jsonfusion, strake, datafusion-bloom, deltalake-datafusion |
| `MemorySourceConfig::show_sizes`, `sort_information` | sail, from its own physical-plan proto codec |
| `AnalyzeExec::verbose` | datafusion-distributed, openobserve |
| `AnalyzeExec::show_statistics` | openobserve |
| `ExplainExec::stringified_plans` | datafusion-ballista scheduler |
| `CoalesceBatchesExec::target_batch_size` | peacockdb |
| `HashJoinExec::join_schema` | IcefallDB |
| `UnnestExec::list_column_indices`, `struct_column_indices` | goldsky/streamling, which rebuilds an unnest operator from a DataFusion `UnnestExec` |
The Sail row is the interesting one: it maintains its own physical-plan proto codec, so "DataFusion serializes this itself now" does not mean the getter is unused -- downstream codecs still need public read access. That argues for treating getters and setters differently: `SortExec`/`AggregateExec`/`HashJoinExec::with_dynamic_filter_expr` had no downstream users at all and are deprecated in #24610, while these read accessors should stay.
Contributor guide
Research direction
Start with the open sub-issues, especially FileScanConfig and MemorySourceConfig, and inspect the try_to_proto and try_from_proto hooks in the datasource crates. Compare the FileSource and DataSink structs with their prost-generated nodes. Done means the 18 hooks across the nine listed files use exhaustive destructuring so all fields are covered by compilation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data-engineering
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100