Skip to content

refactor(proto): migrate AvroSource serde - #24190

Merged
adriangb merged 1 commit into
apache:mainfrom
buraksenn:23516-proto-avro
Aug 9, 2026
Merged

refactor(proto): migrate AvroSource serde#24190
adriangb merged 1 commit into
apache:mainfrom
buraksenn:23516-proto-avro

Conversation

@buraksenn

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Part of epic #23494. Moves AvroSource protobuf serialization from the central dispatch into the source implementation.

What changes are included in this PR?

Add protobuf serialization and deserialization to AvroSource and add the required proto feature wiring to datafusion-datasource-avro.

Repoint the feature-gated live decode arm to AvroSource::try_from_proto and remove the old central encode arm. Keep try_into_avro_scan_physical_plan as a deprecated compatibility wrapper that delegates to the new implementation.

The protobuf wire format remains unchanged.

Are these changes tested?

Yes. Added roundtrip_avro_scan.

Are there any user-facing changes?

The existing PhysicalPlanNodeExt method remains available as a deprecated compatibility wrapper. There is no immediate API removal or wire-format change.

@github-actions github-actions Bot added proto Related to proto crate datasource Changes to the datasource crate labels Aug 8, 2026
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

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
     Cloning apache/main
    Building datafusion-datasource-avro v54.1.0 (current)
       Built [  46.203s] (current)
     Parsing datafusion-datasource-avro v54.1.0 (current)
      Parsed [   0.011s] (current)
    Building datafusion-datasource-avro v54.1.0 (baseline)
       Built [  40.858s] (baseline)
     Parsing datafusion-datasource-avro v54.1.0 (baseline)
      Parsed [   0.010s] (baseline)
    Checking datafusion-datasource-avro v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.075s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  88.639s] datafusion-datasource-avro
    Building datafusion-proto v54.1.0 (current)
       Built [  61.664s] (current)
     Parsing datafusion-proto v54.1.0 (current)
      Parsed [   0.019s] (current)
    Building datafusion-proto v54.1.0 (baseline)
       Built [  61.660s] (baseline)
     Parsing datafusion-proto v54.1.0 (baseline)
      Parsed [   0.019s] (baseline)
    Checking datafusion-proto v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.256s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip

--- failure trait_method_marked_deprecated: trait method #[deprecated] added ---

Description:
A trait method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
        ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/trait_method_marked_deprecated.ron

Failed in:
  method try_into_avro_scan_physical_plan in trait datafusion_proto::physical_plan::PhysicalPlanNodeExt in /home/runner/work/datafusion/datafusion/datafusion/proto/src/physical_plan/mod.rs:1034

     Summary semver requires new minor version: 0 major and 1 minor checks failed
    Finished [ 124.926s] datafusion-proto

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Aug 8, 2026
@codecov-commenter

codecov-commenter commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.82353% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.06%. Comparing base (b225ded) to head (e5fd7f1).

Files with missing lines Patch % Lines
datafusion/datasource-avro/src/source.rs 73.07% 3 Missing and 4 partials ⚠️
datafusion/proto/src/physical_plan/mod.rs 12.50% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24190      +/-   ##
==========================================
- Coverage   81.06%   81.06%   -0.01%     
==========================================
  Files        1106     1106              
  Lines      381847   381862      +15     
  Branches   381847   381862      +15     
==========================================
+ Hits       309541   309551      +10     
+ Misses      54036    54033       -3     
- Partials    18270    18278       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@adriangb adriangb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was AI-assisted (Claude Code); I ran the checks below myself and read the diff.

LGTM. Same shape as #24189 and the earlier Csv/Json/Parquet ports: the deprecated try_into_avro_scan_physical_plan becomes a delegating shim instead of a duplicated body, and the central encode arm is deleted in the same PR so the new hook is genuinely on the path. Thanks for adding roundtrip_avro_scan — Avro was the one file scan with no round-trip coverage at all, so that's a real gap closed rather than a formality.

Verified locally

On a worktree at the PR head:

check result
clippy -p datafusion-proto --all-targets -- -D warnings (default, no avro) clean
same, --features avro clean
same, --all-features clean
clippy -p datafusion-datasource-avro --all-targets with and without proto clean
cargo test -p datafusion-proto --test proto_integration 217 (no avro) / 218 (--features avro) / 220 (--all-features) passed
Cargo.lock drift after building none

The 218-vs-217 confirms roundtrip_avro_scan actually runs, and since the central encode arm is gone it can only pass through AvroSource::try_to_proto. Good that 1b43c061 handled the no-feature build of the helper imports — that combination is easy to miss and it's clean.

One thing to watch when this and #24189 are stacked

The two PRs make opposite edits to the same import block. #24189 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 survives); 1b43c061 here un-gates them (correct while the Arrow arm survives). Once both land all four are dead and the #[cfg]s should be deleted rather than reconciled. The rebase will conflict there so it'll be visible — just flagging it.

Non-blocking, filed separately

The #[cfg(not(feature = "avro"))] panic! arm is carried over verbatim and matches the ParquetScan arm from #24169, so this PR is consistent and I wouldn't hold it up. But it aborts the process on peer-supplied bytes, which is the wrong failure mode for a deserializer — not_impl_err! is already the convention a few lines away for ParquetSink. Filed as #24197 (good first issue) covering both arms plus the "a Avro" → "an Avro" wording.

Nit

datafusion/datasource-avro/Cargo.toml puts [features] before [dependencies], while the sibling datasource-arrow change in #24189 puts it at the end. Purely cosmetic, ignore if you like.

@adriangb
adriangb added this pull request to the merge queue Aug 9, 2026
Merged via the queue into apache:main with commit 918013e Aug 9, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change datasource Changes to the datasource crate proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proto: migrate the simple file scans (CsvSource, JsonSource, ArrowSource, AvroSource)

3 participants