refactor(proto): move PartitionedFile / FileGroup serde into datafusion-datasource - #24006
Merged
adriangb merged 4 commits intoJul 30, 2026
Merged
Conversation
github-actions
Bot
added
proto
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Moves protobuf (de)serialization for file-scan leaf types (FileRange, PartitionedFile, FileGroup) from datafusion-proto into datafusion-datasource, behind a new proto feature, and turns the old TryFromProto impls into delegating shims to avoid drift.
Changes:
- Added
datafusion_datasource::protomodule (feature-gated) implementingTryFrom<&T>conversions for file-scan leaf types and their protobuf messages. - Updated
datafusion-protoencoding/decoding impls to delegate to the newTryFromconversions. - Enabled
datafusion-datasource’s newprotofeature fromdatafusion-protoand added unit tests for round-trips and invalid paths.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| datafusion/proto/src/physical_plan/to_proto.rs | Replaces local serialization logic with thin shims delegating to TryFrom in datafusion-datasource. |
| datafusion/proto/src/physical_plan/from_proto.rs | Replaces local parsing logic with thin shims delegating to TryFrom in datafusion-datasource. |
| datafusion/proto/Cargo.toml | Enables datafusion-datasource’s proto feature so the shims can compile. |
| datafusion/datasource/src/proto.rs | New canonical home for file leaf-type protobuf conversions + unit tests. |
| datafusion/datasource/src/mod.rs | Exposes the new proto module behind the proto feature. |
| datafusion/datasource/Cargo.toml | Adds proto feature and optional dependency on datafusion-proto-models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+71
to
+76
| let last_modified = file.object_meta.last_modified; | ||
| let last_modified_ns = last_modified.timestamp_nanos_opt().ok_or_else(|| { | ||
| DataFusionError::Plan(format!( | ||
| "Invalid timestamp on PartitionedFile::ObjectMeta: {last_modified}" | ||
| )) | ||
| })? as u64; |
| location: Path::parse(file.path.as_str()).map_err(|e| { | ||
| internal_datafusion_err!("Invalid object_store path: {e}") | ||
| })?, | ||
| last_modified: Utc.timestamp_nanos(file.last_modified_ns as i64), |
kumarUjjawal
approved these changes
Jul 30, 2026
kumarUjjawal
left a comment
kumarUjjawal
left a comment
Contributor
There was a problem hiding this comment.
Looks good! Left one question you can decide.
| /// Protobuf conversions for [`FileRange`], [`PartitionedFile`] and | ||
| /// [`FileGroup`](crate::file_groups::FileGroup), gated on the `proto` feature. | ||
| #[cfg(feature = "proto")] | ||
| pub mod proto; |
Contributor
There was a problem hiding this comment.
do we need this a public?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24006 +/- ##
==========================================
- Coverage 80.85% 80.85% -0.01%
==========================================
Files 1098 1099 +1
Lines 374273 374338 +65
Branches 374273 374338 +65
==========================================
+ Hits 302611 302663 +52
- Misses 53621 53623 +2
- Partials 18041 18052 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adriangb
enabled auto-merge
July 30, 2026 17:04
…on-datasource The protobuf conversions for the file-scan leaf types live in `datafusion-proto` today, as `TryFromProto` impls, because that is historically the only crate that could name both sides. That forces any other crate needing them (e.g. a `FileSource` serializing its own scan config, per apache#23494) to reimplement the same wire logic, which can then drift from the central serializer. Put the single copy next to the types that own it, in a new `datafusion_datasource::proto` module behind a new `proto` feature: - `FileRange::try_to_proto` / `try_from_proto` - `PartitionedFile::try_to_proto` / `try_from_proto` - `FileGroup::try_to_proto` / `try_from_proto` `datafusion-proto`'s `TryFromProto` impls for these types become one-line shims delegating to the above, so existing callers are unaffected and the two sides cannot disagree. The wire format is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`TryFromProto` exists because `datafusion-proto` owns neither side of the
conversions it hosts: with both the DataFusion type and the prost message
foreign to it, `impl TryFrom<protobuf::X> for X` is rejected by the orphan
rule, so a local trait was needed to say the same thing.
Moving a conversion into the crate that owns the DataFusion type removes
that constraint, and `&T` is `#[fundamental]`, so both directions are
expressible with the standard trait:
impl TryFrom<&protobuf::PartitionedFile> for PartitionedFile // ok
impl TryFrom<&PartitionedFile> for protobuf::PartitionedFile // ok
Use it for the three types this PR moves, instead of inherent
`try_to_proto` / `try_from_proto` methods. The hook naming stays for
conversions that need an encode/decode context (plans, expressions, scan
configs) — the standard trait cannot carry that second argument, and the
distinction now tells a reader whether a conversion recurses.
The slice form is the one exception: `&[PartitionedFile]` is not a type
this crate owns, so `protobuf::FileGroup`'s slice conversion stays a
`TryFromProto` shim. Callers inside DataFusion go through `FileGroup`,
whose impl lives next to the type.
`datafusion-proto`'s `TryFromProto` impls remain as delegating shims, so
downstream callers are unaffected.
The module holds nothing but `TryFrom` impls, and trait impls are registered globally by coherence, so they reach every downstream crate whether or not the module that writes them is reachable by path. `pub` therefore adds no capability: it only publishes an item-less module page and a path we would then owe compatibility to. If a later conversion in this family needs a named helper that `TryFrom` cannot express (an encode/decode context, per apache#23494), exporting the module again is additive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…code apache#23999 landed in `datafusion-proto` while this branch was open, fixing the duplicate-partition-statistics decode that apache#23998 described. The moved copy in `datafusion-datasource` predates it, so replaying the move on top of current main would have silently reverted the fix. Apply it to the copy that now owns the wire logic, so the delegating shim behaves exactly as `main` does today. Upstream's regression test, `partitioned_file_statistics_roundtrip_with_partition_values`, exercises the shim and therefore pins the moved impl. The round-trip test here asserted only that statistics survived decode, because on the old base they did not survive it faithfully. That caveat is gone: it now asserts the decoded statistics equal the originals, and that they span the full table schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
adriangb
force-pushed
the
prep/partitioned-file-proto-conversions
branch
from
July 30, 2026 18:01
b72e6c0 to
53cb606
Compare
This was referenced Jul 30, 2026
adriangb
added a commit
to pydantic/datafusion
that referenced
this pull request
Aug 9, 2026
The real `TryFrom` impls for `PartitionedFile`, `FileRange`, `FileGroup` (apache#24006) and for `JsonSink` / `CsvSink` / `ParquetSink` / `FileSinkConfig` (apache#23781) now live next to the types, so the `TryFromProto` copies in `datafusion-proto` were pure delegation. `TryFrom<&[PartitionedFile]> for protobuf::FileGroup` has no `TryFrom` equivalent — a slice is not a local type in any crate that could host the impl — so it becomes `datafusion_datasource::proto::partitioned_files_to_proto`, re-exported from `datafusion_proto::physical_plan::to_proto`. The `PartitionedFile` tests move to `datafusion-datasource` alongside the logic they cover; the two that duplicated existing coverage there are dropped. Part of apache#24019. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adriangb
added a commit
to pydantic/datafusion
that referenced
this pull request
Aug 9, 2026
The real `TryFrom` impls for `PartitionedFile`, `FileRange`, `FileGroup` (apache#24006) and for `JsonSink` / `CsvSink` / `ParquetSink` / `FileSinkConfig` (apache#23781) now live next to the types, so the `TryFromProto` copies in `datafusion-proto` were pure delegation. `TryFrom<&[PartitionedFile]> for protobuf::FileGroup` goes away here and comes back in the `datafusion-proto-models` commit, which is the one crate that can express it. The `PartitionedFile` tests move to `datafusion-datasource` alongside the logic they cover; the two that duplicated existing coverage there are dropped. Part of apache#24019. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adriangb
added a commit
to pydantic/datafusion
that referenced
this pull request
Aug 11, 2026
The real `TryFrom` impls for `PartitionedFile`, `FileRange`, `FileGroup` (apache#24006) and for `JsonSink` / `CsvSink` / `ParquetSink` / `FileSinkConfig` (apache#23781) now live next to the types, so the `TryFromProto` copies in `datafusion-proto` were pure delegation. `TryFrom<&[PartitionedFile]> for protobuf::FileGroup` goes away here and comes back in the `datafusion-proto-models` commit, which is the one crate that can express it. The `PartitionedFile` tests move to `datafusion-datasource` alongside the logic they cover; the two that duplicated existing coverage there are dropped. Part of apache#24019. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alamb
added a commit
to AlvaroBalbin/datafusion
that referenced
this pull request
Aug 11, 2026
…ache#24205) ## Which issue does this PR close? - Closes apache#24019. - closes apache#23494 ## 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 apache#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, which was needed during the migration but can now be unwound to result in no breaking change across releases. ## 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 apache#24006 / apache#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 | `TryFrom<&[PartitionedFile]> for protobuf::FileGroup` needed one extra step. `datafusion-datasource` cannot host it — `&T` is `#[fundamental]` but `[T]` is not, so `&[PartitionedFile]` counts as foreign there (`error[E0117]: slices are always foreign`). But in `datafusion-proto-models` the *self* type is local, which is all the orphan rule needs, and staying generic over the element avoids naming `PartitionedFile`, which sits above that crate in the graph: ```rust impl<T> TryFrom<&[T]> for protobuf::FileGroup where for<'a> &'a T: TryInto<protobuf::PartitionedFile, Error = DataFusionError>, { ... } ``` The bound is satisfied by `TryFrom<&PartitionedFile> for protobuf::PartitionedFile` in `datafusion-datasource`, so `protobuf::FileGroup::try_from(&files[..])` resolves for callers exactly as it did in 54.1.0. 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. Finally, `convert.rs` and `convert_required_proto!` are deleted. apache#21929 introduced `FromProto` / `TryFromProto` so the `datafusion-proto-models` extraction could land without relocating ~39 conversions at the same time, and flagged them there as "a known workaround, not the end state", with dropping them listed under Future work. With every conversion moved they have no implementors and no callers. Neither trait has ever shipped in a release, so they are removed outright rather than deprecated — there is nothing for downstream users to migrate off, and doing it now keeps the workaround out of the released API entirely. ## Are these changes tested? Yes. - New `datafusion/proto/tests/cases/public_conversions.rs` coerces all 45 proto conversions in the touched crates to `fn` pointers (the 39 from 54.1.0 plus the ones added on `main`). 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`, `cargo machete`, and the extended test suite all pass at HEAD. 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, and no upgrade-guide entry for the moves. - One genuine delta remains: the parquet options conversions are `TryFrom` rather than 54.1.0's infallible `From`. That predates this PR — `main` had already made them fallible — but it is a real 54.1.0 -> 55.0.0 break and was undocumented, so it is now in the 55.0.0 upgrade guide with a migration snippet. - `FromProto` / `TryFromProto` and `convert_required_proto!` are gone. Not a breaking change: they exist only on `main` and appear nowhere in 54.0.0 or 54.1.0. - `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. Keeping the `api change` label for the parquet options fallibility. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
…on-datasource (apache#24006) ## Which issue does this PR close? - Part of apache#23494. Precursor for apache#23497 / apache#23683 (`DataSource` / `FileSource` proto hooks) and for apache#23752. ## Rationale for this change The protobuf conversions for the file-scan leaf types — `PartitionedFile`, `FileGroup`, `FileRange` — live in `datafusion-proto` as `TryFromProto` impls, because that is historically the only crate that can name both sides (the DataFusion type and the prost message are both foreign to it, hence the `TryFromProto` workaround trait in the first place). That placement means any *other* crate that needs those conversions has to reimplement them. apache#23683 hits exactly this: a `FileSource` serializing its own scan config needs to encode file groups, so the first cut of that PR grew a private second copy of the `PartitionedFile` wire logic inside `datafusion-datasource`, which can then drift from the central serializer. The same will be true of every source migrated under apache#23516–apache#23518. Nothing about these conversions needs `datafusion-proto`: they are plain data, with `ScalarValue` / `Statistics` / `Schema` going through `datafusion-proto-common`. They belong next to the types. ## What changes are included in this PR? - New `datafusion_datasource::proto` module, behind a new `proto` feature on `datafusion-datasource` (off by default; `datafusion-proto` enables it): - `FileRange::try_to_proto` / `try_from_proto` - `PartitionedFile::try_to_proto` / `try_from_proto` - `FileGroup` <-> `protobuf::FileGroup` - `datafusion-proto`'s `TryFromProto` impls for those types become one-line shims delegating to the new impls, so every existing caller keeps working and the two sides cannot disagree. ### Why these are `TryFrom` and not `try_to_proto` hooks `TryFromProto` exists because `datafusion-proto` owns neither side of the conversions it hosts: with both the DataFusion type and the prost message foreign to it, `impl TryFrom<protobuf::X> for X` is rejected by the orphan rule, so a local trait was the only way to say the same thing. Moving a conversion into the crate that owns the DataFusion type removes that constraint, and `&T` is `#[fundamental]`, so both directions are expressible with the standard trait (checked, not assumed): ```rust impl TryFrom<&protobuf::PartitionedFile> for PartitionedFile // ok impl TryFrom<&PartitionedFile> for protobuf::PartitionedFile // ok impl TryFrom<&[PartitionedFile]> for protobuf::FileGroup // E0117 ``` The last one is why `protobuf::FileGroup`'s *slice* conversion stays a `TryFromProto` shim: `&[PartitionedFile]` is not a type this crate owns, while `&FileGroup` is. Callers inside DataFusion go through `FileGroup`. So the rule this PR sets for the rest of apache#23494: **plain data uses `TryFrom`; anything needing an encode/decode context keeps the `try_to_proto(ctx)` / `try_from_proto(node, ctx)` hooks**, because the standard trait cannot carry that second argument. Usefully, none of the ~40 `TryFromProto`/`FromProto` impls needs a context, and nothing that needs one was ever a `TryFromProto` impl — the two categories are already disjoint, so the shape now tells a reader whether a conversion recurses. ### Why now `FromProto` / `TryFromProto` were added in apache#21929, *after* the 54.0.0 release, and 54.1.0 was cut before any of this landed — so they have never shipped in a release. Replacing them with the standard traits, and eventually deleting them, is a no-op for semver **today** and a major breaking change the moment 55.0.0 goes out. The same applies to the six inherent `try_to_proto` / `try_from_proto` methods this PR would otherwise have added: they are new, unreleased API, so choosing their final shape costs nothing right now. The other reason to settle it here rather than in a follow-up: this is the PR that establishes the pattern for the data-source family (apache#23516-apache#23519 and apache#23752 / apache#23781 are all queued behind it). Whichever shape merges first is the one they will copy. Retiring the remaining ~34 impls is still its own follow-up. Two notes for whoever picks it up: the sink and format-option conversions can move next to their types the same way, but the ones for `datafusion-common`-owned types (`JoinType`, `NullEquality`, `TableReference`, `UnnestOptions`, ...) cannot — `datafusion-common` cannot depend on `datafusion-proto-models` (it is underneath it via `datafusion-proto-common`). Their legal home is `proto-models` itself, implementing on the local proto type, which is already how `proto-common` hosts the `ScalarValue` / `Statistics` conversions. ## Are these changes tested? Yes. - New unit tests in `datafusion_datasource::proto` covering the `PartitionedFile` round trip (path, size, mtime, partition values, range, arrow schema, statistics), the `FileGroup` round trip, and the invalid-path error. - The existing `datafusion-proto` tests now exercise the delegating shims, so they also pin the shims themselves. - `datafusion-proto`, all features: 227 passed / 0 failed. - `datafusion-datasource` with `proto`: 180 passed / 0 failed. - Full workspace run: 10347 passed (`cargo test --profile ci --workspace --lib --tests --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`). The only failures are 8 backtrace-symbolization tests in `datafusion-common`, a crate this PR does not touch and which sits below every crate it does; they fail the same way on the base commit on macOS. - `cargo fmt` and `ci/scripts/rust_clippy.sh` clean. ## Are there any user-facing changes? The protobuf wire format is unchanged, and no existing API changes shape. Additive: - New `proto` feature on `datafusion-datasource` (off by default). - New `TryFrom` impls in both directions between `FileRange`, `PartitionedFile`, `FileGroup` and their protobuf messages, under that feature. No new names are added to the crate's API surface: the trait is `core::convert::TryFrom`. Note for reviewers: while writing the round-trip test I found that `PartitionedFile` statistics do not round-trip cleanly on `main` — filed as apache#23998. This PR preserves that behavior exactly rather than changing decode semantics in a refactor; the test documents it. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
…ache#24205) ## Which issue does this PR close? - Closes apache#24019. - closes apache#23494 ## 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 apache#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, which was needed during the migration but can now be unwound to result in no breaking change across releases. ## 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 apache#24006 / apache#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 | `TryFrom<&[PartitionedFile]> for protobuf::FileGroup` needed one extra step. `datafusion-datasource` cannot host it — `&T` is `#[fundamental]` but `[T]` is not, so `&[PartitionedFile]` counts as foreign there (`error[E0117]: slices are always foreign`). But in `datafusion-proto-models` the *self* type is local, which is all the orphan rule needs, and staying generic over the element avoids naming `PartitionedFile`, which sits above that crate in the graph: ```rust impl<T> TryFrom<&[T]> for protobuf::FileGroup where for<'a> &'a T: TryInto<protobuf::PartitionedFile, Error = DataFusionError>, { ... } ``` The bound is satisfied by `TryFrom<&PartitionedFile> for protobuf::PartitionedFile` in `datafusion-datasource`, so `protobuf::FileGroup::try_from(&files[..])` resolves for callers exactly as it did in 54.1.0. 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. Finally, `convert.rs` and `convert_required_proto!` are deleted. apache#21929 introduced `FromProto` / `TryFromProto` so the `datafusion-proto-models` extraction could land without relocating ~39 conversions at the same time, and flagged them there as "a known workaround, not the end state", with dropping them listed under Future work. With every conversion moved they have no implementors and no callers. Neither trait has ever shipped in a release, so they are removed outright rather than deprecated — there is nothing for downstream users to migrate off, and doing it now keeps the workaround out of the released API entirely. ## Are these changes tested? Yes. - New `datafusion/proto/tests/cases/public_conversions.rs` coerces all 45 proto conversions in the touched crates to `fn` pointers (the 39 from 54.1.0 plus the ones added on `main`). 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`, `cargo machete`, and the extended test suite all pass at HEAD. 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, and no upgrade-guide entry for the moves. - One genuine delta remains: the parquet options conversions are `TryFrom` rather than 54.1.0's infallible `From`. That predates this PR — `main` had already made them fallible — but it is a real 54.1.0 -> 55.0.0 break and was undocumented, so it is now in the 55.0.0 upgrade guide with a migration snippet. - `FromProto` / `TryFromProto` and `convert_required_proto!` are gone. Not a breaking change: they exist only on `main` and appear nowhere in 54.0.0 or 54.1.0. - `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. Keeping the `api change` label for the parquet options fallibility. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
DataSource/FileSourceproto hooks) and for Proto: add DataSink serialization hook #23752.Rationale for this change
The protobuf conversions for the file-scan leaf types —
PartitionedFile,FileGroup,FileRange— live indatafusion-protoasTryFromProtoimpls,because that is historically the only crate that can name both sides (the
DataFusion type and the prost message are both foreign to it, hence the
TryFromProtoworkaround trait in the first place).That placement means any other crate that needs those conversions has to
reimplement them. #23683 hits exactly this: a
FileSourceserializing its ownscan config needs to encode file groups, so the first cut of that PR grew a
private second copy of the
PartitionedFilewire logic insidedatafusion-datasource, which can then drift from the central serializer.The same will be true of every source migrated under #23516–#23518.
Nothing about these conversions needs
datafusion-proto: they are plain data,with
ScalarValue/Statistics/Schemagoing throughdatafusion-proto-common. They belong next to the types.What changes are included in this PR?
datafusion_datasource::protomodule, behind a newprotofeature ondatafusion-datasource(off by default;datafusion-protoenables it):FileRange::try_to_proto/try_from_protoPartitionedFile::try_to_proto/try_from_protoFileGroup<->protobuf::FileGroupdatafusion-proto'sTryFromProtoimpls for those types become one-lineshims delegating to the new impls, so every existing caller keeps working
and the two sides cannot disagree.
Why these are
TryFromand nottry_to_protohooksTryFromProtoexists becausedatafusion-protoowns neither side of theconversions it hosts: with both the DataFusion type and the prost message
foreign to it,
impl TryFrom<protobuf::X> for Xis rejected by the orphanrule, so a local trait was the only way to say the same thing.
Moving a conversion into the crate that owns the DataFusion type removes that
constraint, and
&Tis#[fundamental], so both directions are expressiblewith the standard trait (checked, not assumed):
The last one is why
protobuf::FileGroup's slice conversion stays aTryFromProtoshim:&[PartitionedFile]is not a type this crate owns, while&FileGroupis. Callers inside DataFusion go throughFileGroup.So the rule this PR sets for the rest of #23494: plain data uses
TryFrom;anything needing an encode/decode context keeps the
try_to_proto(ctx)/try_from_proto(node, ctx)hooks, because the standard trait cannot carrythat second argument. Usefully, none of the ~40
TryFromProto/FromProtoimpls needs a context, and nothing that needs one was ever a
TryFromProtoimpl — the two categories are already disjoint, so the shape now tells a
reader whether a conversion recurses.
Why now
FromProto/TryFromProtowere added in #21929, after the 54.0.0 release,and 54.1.0 was cut before any of this landed — so they have never shipped in a
release. Replacing them with the standard traits, and eventually deleting them,
is a no-op for semver today and a major breaking change the moment 55.0.0
goes out. The same applies to the six inherent
try_to_proto/try_from_protomethods this PR would otherwise have added: they are new, unreleased API, so
choosing their final shape costs nothing right now.
The other reason to settle it here rather than in a follow-up: this is the PR
that establishes the pattern for the data-source family (#23516-#23519 and
#23752 / #23781 are all queued behind it). Whichever shape merges first is the
one they will copy.
Retiring the remaining ~34 impls is still its own follow-up. Two notes for
whoever picks it up: the sink and format-option conversions can move next to
their types the same way, but the ones for
datafusion-common-owned types(
JoinType,NullEquality,TableReference,UnnestOptions, ...) cannot —datafusion-commoncannot depend ondatafusion-proto-models(it isunderneath it via
datafusion-proto-common). Their legal home isproto-modelsitself, implementing on the local proto type, which is alreadyhow
proto-commonhosts theScalarValue/Statisticsconversions.Are these changes tested?
Yes.
datafusion_datasource::protocovering thePartitionedFileround trip (path, size, mtime, partition values, range,arrow schema, statistics), the
FileGroupround trip, and the invalid-patherror.
datafusion-prototests now exercise the delegating shims, sothey also pin the shims themselves.
datafusion-proto, all features: 227 passed / 0 failed.datafusion-datasourcewithproto: 180 passed / 0 failed.(
cargo test --profile ci --workspace --lib --tests --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption).The only failures are 8 backtrace-symbolization tests in
datafusion-common,a crate this PR does not touch and which sits below every crate it does; they
fail the same way on the base commit on macOS.
cargo fmtandci/scripts/rust_clippy.shclean.Are there any user-facing changes?
The protobuf wire format is unchanged, and no existing API changes shape.
Additive:
protofeature ondatafusion-datasource(off by default).TryFromimpls in both directions betweenFileRange,PartitionedFile,FileGroupand their protobuf messages, under thatfeature. No new names are added to the crate's API surface: the trait is
core::convert::TryFrom.Note for reviewers: while writing the round-trip test I found that
PartitionedFilestatistics do not round-trip cleanly onmain— filed as#23998. This PR preserves that behavior exactly rather than changing decode
semantics in a refactor; the test documents it.