refactor(proto): migrate ArrowSource serde - #24189
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24189 +/- ##
==========================================
- Coverage 81.06% 81.04% -0.02%
==========================================
Files 1106 1106
Lines 381891 381896 +5
Branches 381891 381896 +5
==========================================
- Hits 309578 309511 -67
- Misses 54034 54100 +66
- Partials 18279 18285 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adriangb
left a comment
There was a problem hiding this comment.
This review was AI-assisted (Claude Code); I ran the checks below myself and read the diff.
LGTM — this is a faithful port and matches the pattern set by #24177 / #24178 / #24169. The deprecated try_into_arrow_scan_physical_plan is reduced to a delegating shim rather than a duplicated body, which is what the epic asks for, and deleting the central encode arm in the same PR is what proves the new hook is actually reached.
Verified locally
On a worktree at the PR head:
| check | result |
|---|---|
clippy -p datafusion-proto --all-targets -- -D warnings (default features) |
clean |
same, --features avro |
clean |
same, --all-features |
clean |
clippy -p datafusion-datasource-arrow --all-targets with and without proto |
clean |
cargo test -p datafusion-proto --test proto_integration |
217 passed |
Cargo.lock drift after building |
none |
The proto-off build of datafusion-datasource-arrow matters here and it's clean, so the new optional dep is gated correctly.
One thing to watch when this and #24190 are stacked
The two PRs make opposite edits to the same import block. This PR gates FileScanConfig, parse_protobuf_file_scan_config, parse_table_schema_from_proto and serialize_file_scan_config behind #[cfg(feature = "avro")] — correct while the Avro arm is the last remaining user. #24190 removes that arm, and its follow-up commit un-gates the same four. Once both land all four are dead and every one of those #[cfg]s should be deleted, not reconciled. It's a textual conflict so the rebase will surface it; just flagging so it doesn't get merged back into existence.
Pleasant side effect once both are in: try_from_data_source_exec is down to MemorySourceConfig alone.
Non-blocking, filed separately
ArrowSource::format has no wire representation, so a stream-format scan round-trips into a file-format scan — the encode side emits ArrowScan for both variants and decode always rebuilds with new_file_source. Pre-existing, unchanged by this PR, and your try_from_proto doc comment says so explicitly; I filed #24196 with a reproduction. Worth noting that roundtrip_test does not catch it (I checked — the before/after Debug strings compare equal), so it needs a downcast assertion.
Only nit on this PR: the decode doc comment records the lossiness but try_to_proto's doesn't. A one-line mirror there would help the next reader. Not worth another round trip if you'd rather fold it into #24196.
56c0574 to
2de2dcb
Compare
2de2dcb to
893e045
Compare
893e045 to
4ae3ceb
Compare
Which issue does this PR close?
Rationale for this change
Part of epic #23494. Moves
ArrowSourceprotobuf serialization from the central dispatch into the source implementation.What changes are included in this PR?
Add protobuf serialization and deserialization to
ArrowSourceand add the requiredprotofeature wiring todatafusion-datasource-arrow.Repoint the live decode arm to
ArrowSource::try_from_protoand remove the old central encode arm. Keeptry_into_arrow_scan_physical_planas a deprecated compatibility wrapper that delegates to the new implementation.The protobuf wire format remains unchanged.
Are these changes tested?
Yes. The existing
roundtrip_arrow_scancoverage passes through the new hooks.Are there any user-facing changes?
The existing
PhysicalPlanNodeExtmethod remains available as a deprecated compatibility wrapper. There is no immediate API removal or wire-format change.