adriangb opened a new pull request, #24205:
URL: https://github.com/apache/datafusion/pull/24205
## Which issue does this PR close?
- Part of #24019. Covers every work item except the last one (deleting
`convert.rs`), which @Phoenix500526 asked to pick up once the conversions were
migrated — this PR is that prerequisite: both traits now have zero implementors.
## Rationale for this change
`datafusion-proto` 54.1.0 publishes 39 `From` / `TryFrom` impls converting
between DataFusion types and their protobuf messages. On `main` all of them
were replaced by the crate-local `FromProto` / `TryFromProto` traits introduced
in #21929, so code written against the released version stops compiling:
```rust
let proto = protobuf::PartitionedFile::try_from(&file)?; // no longer
resolves on main
let frame = WindowFrame::try_from(proto_frame)?; // no longer
resolves on main
```
That was collateral damage from the orphan-rule workaround, not an intended
API change. `cargo-semver-checks` has no lint for a removed hand-written trait
impl, and the `breaking_changes_detector` workflow baselines against
`apache/main` rather than the last release, so nothing flagged it.
## What changes are included in this PR?
Each conversion moves to a crate that owns one side of it, and goes back to
being a plain `From` / `TryFrom` — the shape 54.1.0 published. Error types are
unchanged (`FromProtoError` decoding, `ToProtoError` encoding,
`DataFusionError` for the datasource types).
| Types | New home |
|---|---|
| `PartitionedFile`, `FileRange`, `FileGroup`, `JsonSink`, `CsvSink`,
`ParquetSink`, `FileSinkConfig` | already moved by #24006 / #23781 — this PR
just deletes the `TryFromProto` shims that delegated to them |
| `WindowFrame`, `WindowFrameBound`, `WindowFrameUnits`,
`MergeIntoClauseKind`, `NullTreatment` | `datafusion-expr`, behind a new
`proto` feature (optional `datafusion-proto-common` / `datafusion-proto-models`
deps, mirroring `datafusion-datasource`) |
| `UnnestOptions`, `TableReference`, `StringifiedPlan`, `JoinType`,
`JoinConstraint`, `NullEquality`, `CsvOptions`, `JsonOptions`, and the parquet
options types | `datafusion-proto-models`, on the local proto type — their
DataFusion side sits *below* that crate in the graph, the same arrangement
`datafusion-proto-common` already uses for `ScalarValue` / `Statistics` |
| `CsvFormatFactory`, `JsonFormatFactory`, `ParquetFormatFactory` |
`datafusion-datasource-{csv,json,parquet}`, behind each crate's existing
`proto` feature |
| `Column` <-> `protobuf::PhysicalColumn` | `datafusion-physical-expr`;
`Column::try_to_proto` / `try_from_proto` now go through it instead of building
the message inline |
One conversion could not come back as a trait impl:
`TryFrom<&[PartitionedFile]> for protobuf::FileGroup`. A slice is not a local
type in any crate that could host it, so it becomes
`datafusion_datasource::proto::partitioned_files_to_proto`, re-exported from
`datafusion_proto::physical_plan::to_proto` where the impl used to live. This
is the one behaviour-preserving break, and it is in the 55.0.0 upgrade guide.
Two items beyond the issue's checklist, both needed to reach zero
implementors:
- the parquet options conversions (`ParquetOptions`, `TableParquetOptions`,
`ParquetColumnOptions`, `ParquetCdcOptions`) — the issue's table undercounts
`file_formats.rs` because they live in a private module, but trait impls are
global, so they were public API too. They return as `TryFrom`; `main` had
already made them fallible, so an exact restore of 54.1.0's infallible `From`
isn't available.
- `From<&protobuf::PhysicalColumn> for Column`, which the issue's evidence
table counts but no work item names.
Not restored, and worth calling out: `From<protobuf::dml_node::Type> for
WriteOp` and its reverse. `main` replaced them with `parse_write_op` /
`serialize_write_op` because `MergeInto` carries a payload a `From` impl cannot
express. That is a separate, deliberate change.
`convert.rs` and `convert_required_proto!` are left in place with a doc note
— deleting them is the last work item and is spoken for.
## Are these changes tested?
Yes.
- New `datafusion/proto/tests/cases/public_conversions.rs` coerces all 39
conversions to `fn` pointers. This is the regression guard the issue asks for:
it fails to compile when an impl is removed, and stays quiet when one merely
moves between crates, which is exactly the case `cargo-semver-checks` cannot
see.
- New round-trip tests next to the moved impls in `datafusion-expr` and
`datafusion-proto-models` (window frames, table references, join enums, unnest
options, stringified plans).
- The `PartitionedFile` tests move from `datafusion-proto` to
`datafusion-datasource`, alongside the logic they cover; two that duplicated
existing coverage there are dropped.
- Existing round-trip suites (`roundtrip_logical_plan`,
`roundtrip_physical_plan`) pass unchanged, which is the real wire-format check.
- Every moved impl body was diffed against `main`: 22 are byte-identical
modulo the trait rename, and the other 9 differ only by `Self::` shorthand,
error-type aliasing, and rustfmt reflow. No serialization logic changed.
- `./dev/rust_lint.sh` and the extended test suite both pass, as does each
commit in this PR individually. Also checked: `datafusion-proto` without
`parquet`, `datafusion-expr` with `proto` off and `--no-default-features`, the
format crates without `proto`, and `json` on both proto crates.
## Are there any user-facing changes?
Yes, and they restore rather than break the released API.
- The 39 conversions removed since 54.1.0 compile again. Trait impls are
global, so `X::try_from(&proto)` / `proto.try_into()` resolve regardless of
which crate now hosts the impl — no import changes needed.
- `TryFrom<&[PartitionedFile]> for protobuf::FileGroup` is now the function
`partitioned_files_to_proto`. Documented in the 55.0.0 upgrade guide with a
migration snippet.
- `datafusion-expr` gains an off-by-default `proto` feature. Additive.
- `datafusion-proto-models` gains a direct `datafusion-common` dependency
(already present transitively) and two new public modules.
Adding the `api change` label since the slice conversion changes 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]
