Repository navigation
EPIC: destructure proto serde hooks in the datasource crates #24612
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::fetchwas dropped this way (fixed in fix(proto): preserve HashJoinExec fetch across serialization #24165).- CsvSource::with_terminator is silently dropped when a physical plan is serialized #24609 --
CsvSourceterminator -- 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
- Destructure proto hooks for the Parquet source and sink #24620 Destructure proto hooks for the Parquet source and sink -- partial fix in fix: preserve Parquet sort pushdown across proto roundtrips #24930 (merged,
reverse_row_groups/sort_order_for_reorderonly); the destructure itself is still open - Destructure proto hooks for the JSON source and sink #24621 Destructure proto hooks for the JSON source and sink -- fix(proto): destructure JSON source and sink serde hooks #24945 (open)
- Destructure proto hooks for the Arrow and Avro sources #24622 Destructure proto hooks for the Arrow and Avro sources -- refactor(proto): destructure Arrow and Avro source serde hooks #24696 (merged)
- Destructure proto hooks for the CSV sink #24623 Destructure proto hooks for the CSV sink
- Destructure proto hooks for FileScanConfig and MemorySourceConfig #24624 Destructure proto hooks for FileScanConfig and MemorySourceConfig -- refactor(proto): destructure FileScanConfig and MemorySourceConfig serde hooks #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.
Activity
adriangb commentedon Aug 24, 2026
cc @buraksenn, @kosiew, @kumarUjjawal, @Phoenix500526 since you were instrumental in previous work on this topic
kosiew commentedon Oct 10, 2026
@adriangb
All five sub-issues here are closed now (#24620, #24621, #24622, #24623, #24624), and the three confirmed field drops are fixed on
main. One part hasn't landed: the "Public API to consider deprecating" section. None of the three "safe to deprecate" accessors is deprecated yet:CsvSource::newlines_in_values: the plan was to deprecate it as part of the CsvSource::with_terminator is silently dropped when a physical plan is serialized #24609 fix, but CsvSource::with_terminator is silently dropped when a physical plan is serialized #24609 closed without it.AnalyzeExec::metric_categoriesExplainExec::verboseI re-ran the downstream search (GitHub code search, plus grepping fresh clones of the 8 repos from your survey and greptimedb, lance and datafusion-federation):
CsvSource::newlines_in_values: no callers.AnalyzeExec::metric_categories: no callers. Every hit was a vendored copy of DataFusion or an unrelated identifier.ExplainExec::verbose: three callers, all copies of DataFusion's old in-tree proto serializer (ExplainExecNode { verbose: exec.verbose(), .. }) in projects that haven't been updated in a long time:hetudb/hetu(last push 2024-10),suibianwanwank/hexa(2024-05) andep-infosec/33_apache_arrow-ballista(2022-12).I opened #26179 to deprecate
CsvSource::newlines_in_values. It would also switch the CSV encoder to read the field directly and add the 56.0.0 upgrade-guide entry. Before I touch the other two, could you confirm:ExplainExec::verbose: deprecate or keep? It has a few (stale) downstream callers, and your note argues read accessors should stay available to external codecs, so my inclination is to keep it.AnalyzeExec::metric_categoriesandExplainExec::verboseas outside this EPIC. Should they go in the new PR, or be left alone?datafusion/proto/tests/cases/plans/misc.rsusesmetric_categories()andExplainExec::verbose()to check that those fields survive a round-trip. If we deprecate them, would you rather have#[expect(deprecated)]in the tests, or assertions that compareDebug/fmt_asoutput instead?adriangb commentedon Oct 10, 2026
I don't think the deprecations is super important. Those cases all sound like they could be useful. Maybe we just leave them? Either way I'll close this issue, we can track those minor things in followups if we want.
Great work everyone!
Deprecate CsvSource::newlines_in_values and preserve proto coverage (a…
Deprecate CsvSource::newlines_in_values and preserve proto coverage (a…