adriangb opened a new issue, #24019:
URL: https://github.com/apache/datafusion/issues/24019
## Summary
`datafusion-proto` 54.1.0 (the current release on crates.io) exposes 39
`From` / `TryFrom` impls converting between DataFusion types and their protobuf
messages. On `main` all of them have been replaced by impls of the crate-local
`FromProto` / `TryFromProto` traits introduced in #21929, so the standard-trait
conversions are gone from the public API.
This was almost certainly unintentional: the `TryFromProto` workaround
exists to satisfy the orphan rule, not to change the API downstream users call.
This issue tracks restoring them and then retiring the workaround traits, which
have never shipped in a release.
## Evidence
Released API (`git show 54.1.0:datafusion/proto/src/...`), all `pub`:
| file | impls in 54.1.0 | on `main` |
|---|---|---|
| `physical_plan/from_proto.rs` | 8 `From`/`TryFrom` | 0 (7 `TryFromProto`) |
| `physical_plan/to_proto.rs` | 7 `TryFrom` | 0 (7 `TryFromProto`) |
| `logical_plan/from_proto.rs` | 11 `From`/`TryFrom` | 0 (11
`FromProto`/`TryFromProto`) |
| `logical_plan/to_proto.rs` | 11 `From`/`TryFrom` | 0 (11
`FromProto`/`TryFromProto`) |
| `logical_plan/file_formats.rs` | 2 `From` | 0 (4 `FromProto`) |
So code written against 54.1.0 such as
```rust
let proto = protobuf::PartitionedFile::try_from(&file)?;
let file = PartitionedFile::try_from(&proto)?;
let frame = WindowFrame::try_from(proto_frame)?;
```
no longer compiles against `main`.
Two reasons this went unnoticed:
- The `breaking_changes_detector` workflow baselines `cargo-semver-checks`
against `apache/main`, not against the last release, so a change made on `main`
after the release is never compared to what shipped.
- Even against a release baseline, `cargo-semver-checks` has no lint for a
removed hand-written trait impl (it has `derive_trait_impl_removed` and
`auto_trait_impl_removed` only). Running it manually with `--baseline-version
54.1.0` reports the other known deltas (proto types relocated to
`datafusion-proto-models`, `PhysicalExtensionCodec::try_{encode,decode}[_expr]`
parameter counts) and says nothing about these.
Note that a trait impl cannot be deprecated — there is no `#[deprecated]`
for an `impl` — so the [API health
policy](https://datafusion.apache.org/contributor-guide/api-health.html)'s
usual "deprecate rather than remove" path is not available here. The options
are to restore the impls or to accept the removal and document it in the
Upgrade Guide.
## Proposed fix
Restore each conversion as a standard `From` / `TryFrom` impl, in a crate
that owns one side of it. #24006 already did this for `PartitionedFile`,
`FileRange` and `FileGroup`, which is the reference.
Where each impl can live follows from the crate graph (`datafusion-common`
<- `datafusion-proto-common` <- `datafusion-proto-models`) and from `&T` being
`#[fundamental]`, which makes `&LocalType` count as local so that *both*
directions are expressible:
- **The type's crate can depend on `datafusion-proto-models`** (a sibling or
above it: `datafusion-expr`, `datafusion-datasource*`, `datafusion-physical-*`)
— put the impl next to the type, behind a `proto` feature.
- **It cannot** (it is *below* `proto-models`, i.e. anything in
`datafusion-common`, or it is a third-party type) — put the impl in
`datafusion-proto-models`, implementing on its own local proto type.
`datafusion-proto-common` already hosts the `ScalarValue` / `Statistics`
conversions this way.
- **Neither side is a nameable local type** — not expressible. The only case
is `impl TryFrom<&[PartitionedFile]> for protobuf::FileGroup`: a slice is not a
local type in any crate that could host it, and `proto-models` cannot depend on
`datafusion-datasource`. A free function, or routing callers through
`&FileGroup`, is the replacement.
## Work items
Independent of one another; any order.
- [ ] `FileSinkConfig` -> `datafusion-datasource`; `JsonSink` / `CsvSink` /
`ParquetSink` -> their format crates (8 impls). In flight as part of #23752 /
#23781.
- [ ] Delete the now-redundant `TryFromProto` shims for `PartitionedFile` /
`FileRange` / `FileGroup` (6 impls); replace the `&[PartitionedFile]` one with
a free function.
- [ ] `WindowFrame`, `WindowFrameBound`, `WindowFrameUnits`,
`MergeIntoClauseKind`, `NullTreatment` -> `datafusion-expr` behind a new
`proto` feature (10 impls).
- [ ] `UnnestOptions`, `TableReference`, `StringifiedPlan`, `JoinType`,
`JoinConstraint`, `NullEquality`, `CsvOptions`, `JsonOptions` ->
`datafusion-proto-models` (14 impls).
- [ ] `CsvFormatFactory` / `JsonFormatFactory` ->
`datafusion-datasource-csv` / `-json` (2 impls).
- [ ] Once no impls remain: delete `datafusion/proto/src/convert.rs`
(`FromProto`, `TryFromProto`) and rewrite `convert_required_proto!` in
`datafusion/proto/src/common.rs` on top of `TryFrom`.
The last item is not a breaking change: `convert.rs` does not exist in the
54.0.0 or 54.1.0 tags, and the identifier `TryFromProto` appears nowhere in
54.1.0, so neither trait has been released.
## Related
- #21835 / #21929 introduced the traits.
- #23494 is the `ExecutionPlan` serialization epic that motivated moving the
conversions out of `datafusion-proto` in the first place.
- #24006 is the reference for the restored shape.
--
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]
