adriangb opened a new pull request, #24006:
URL: https://github.com/apache/datafusion/pull/24006
## Which issue does this PR close?
- Part of #23494. Precursor for #23497 / #23683 (`DataSource` / `FileSource`
proto hooks) and for #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. #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 #23516–#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::try_to_proto` / `try_from_proto`
- `datafusion-proto`'s `TryFromProto` impls for those types become one-line
shims delegating to the new methods, so every existing caller keeps working
and the two sides cannot disagree.
This is the shape the rest of the migration wants: once an impl lives in the
crate that owns one side of the conversion, the `TryFromProto` workaround is
no
longer needed for it. Doing the same for the remaining ~40 impls (and then
retiring `FromProto`/`TryFromProto` entirely) is worth its own follow-up;
this PR just takes the file-scan types, which are the ones blocking the
data-source work.
## 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.
- Full workspace run: 10078 passed / 0 failed
(`cargo test --profile ci --workspace --lib --tests --features
avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`),
plus `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 public `try_to_proto` / `try_from_proto` on `FileRange`,
`PartitionedFile` and `FileGroup` under that feature.
Note for reviewers: while writing the round-trip test I found that
`PartitionedFile` statistics do not round-trip cleanly on `main` — filed as
#23998. This PR preserves that behavior exactly rather than changing decode
semantics in a refactor; the test documents it.
--
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]
