Skip to content

fix: return error instead of panic when decoding ParquetScan/AvroScan without features - #24198

Merged
kumarUjjawal merged 1 commit into
apache:mainfrom
nam2ee:fix-proto-feature-panic
Aug 9, 2026
Merged

fix: return error instead of panic when decoding ParquetScan/AvroScan without features#24198
kumarUjjawal merged 1 commit into
apache:mainfrom
nam2ee:fix-proto-feature-panic

Conversation

@nam2ee

@nam2ee nam2ee commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Explained in #24197.

What changes are included in this PR?

I just replaced the three feature-gate panic!s in datafusion/proto/src/physical_plan/mod.rs with not_impl_err!, following the existing ParquetSink convention in the same file. Also fixes "a Avro" to "an Avro" in the message.

Are these changes tested?

Checked the same feature combinations as the datafusion-proto features CI job and ran cargo test -p datafusion-proto. Manually verified with a no-default-features build that decoding panicked before and returns NotImplemented after. No regression test added because the error path only exists in feature-disabled builds, which CI only runs cargo check on.

Are there any user-facing changes?

No API changes.

@github-actions github-actions Bot added the proto Related to proto crate label Aug 9, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.04%. Comparing base (abc5ce7) to head (18423b4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24198      +/-   ##
==========================================
- Coverage   81.05%   81.04%   -0.01%     
==========================================
  Files        1106     1106              
  Lines      382398   382398              
  Branches   382398   382398              
==========================================
- Hits       309938   309914      -24     
- Misses      54149    54168      +19     
- Partials    18311    18316       +5     

☔ 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.

@kumarUjjawal kumarUjjawal 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.

Thank you @nam2ee

LGTM!

@getChan getChan 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.

Reviewed the changes. Replacing the feature-disabled panic paths with not_impl_err! is appropriate and matches the existing convention. No blocking issues found.

@kumarUjjawal

Copy link
Copy Markdown
Contributor

Thank you @nam2ee and @getChan

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

Labels

proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decoding a ParquetScan/AvroScan physical plan panics when the feature is disabled, instead of returning an error

4 participants