Skip to content

Restore the From / TryFrom proto conversions dropped since 54.1.0, and retire FromProto / TryFromProto #24019

Description

@adriangb

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

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'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 Proto: add DataSink serialization hook #23752 / Proto: migrate file sink serialization #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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions