adriangb opened a new issue, #24612:
URL: https://github.com/apache/datafusion/issues/24612
## 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
24 un-destructured hooks in this EPIC's scope (12 encoders, 12 decoders)
across 12 files.
| group | files | sub-issue |
|---|---|---|
| Parquet source and sink | 2 | B1 |
| JSON source and sink | 2 | B2 |
| Arrow and Avro sources | 2 | B3 |
| CSV sink | 1 | B4 |
| `FileScanConfig` and `MemorySourceConfig` | 2 | B5 |
`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.
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
