Skip to content

fix(proto): check plan integer conversions across usize boundaries - #24483

Open
buraksenn wants to merge 1 commit into
apache:mainfrom
buraksenn:24170-plan-serde
Open

fix(proto): check plan integer conversions across usize boundaries#24483
buraksenn wants to merge 1 commit into
apache:mainfrom
buraksenn:24170-plan-serde

Conversation

@buraksenn

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Several logical and physical plan protobuf paths convert wire integers to usize with unchecked casts. On 32-bit targets, values above u32::MAX silently truncate. For row limits, a fetch of 1 << 32 becomes 0, causing a plan that should return rows to return an empty result.

The reverse conversion can also truncate when a 64-bit usize does not fit the protobuf field.

What changes are included in this PR?

  • Add shared helpers for checked conversions between wire integers and usize, with errors identifying the plan node and field.
  • Apply checked conversions to limits, partition counts, batch sizes, capacities, and indexes across logical and physical plan serialization.
  • Preserve the documented -1 sentinel for limits without a fetch.
  • Reject invalid zero batch sizes while preserving proto3 defaults for older FilterExec payloads.
  • Treat out-of-range usize statistics as unknown rather than truncating them.

The protobuf wire format is unchanged.

Follow-up work will make common protobuf option and constraint conversions fallible.

Are these changes tested?

Yes. Added unit and protobuf roundtrip coverage for checked conversions, architecture-dependent integer boundaries, oversized encode/decode values, limits without a fetch, malformed zero-column Values nodes, and zero batch sizes.

Are there any user-facing changes?

Out-of-range plan integers now return a clear planning error instead of silently truncating. Invalid zero batch sizes are rejected, and out-of-range statistics become unknown. There are no breaking public API or protobuf wire-format changes.

@github-actions github-actions Bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates common Related to common crate proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate physical-plan Changes to the physical-plan crate labels Aug 19, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.33962% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.24%. Comparing base (bbf739a) to head (e80c835).
⚠️ Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/expr/src/logical_plan/plan.rs 53.57% 8 Missing and 5 partials ⚠️
datafusion/proto/src/logical_plan/mod.rs 47.05% 5 Missing and 4 partials ⚠️
datafusion/physical-plan/src/filter.rs 78.37% 3 Missing and 5 partials ⚠️
...usion/physical-plan/src/windows/window_agg_exec.rs 0.00% 6 Missing ⚠️
datafusion/physical-plan/src/coalesce_batches.rs 79.16% 1 Missing and 4 partials ⚠️
datafusion/physical-plan/src/limit.rs 54.54% 0 Missing and 5 partials ⚠️
datafusion/proto/src/physical_plan/mod.rs 61.53% 3 Missing and 2 partials ⚠️
...atafusion/datasource/src/file_scan_config/proto.rs 81.25% 0 Missing and 3 partials ⚠️
datafusion/physical-expr/src/partitioning.rs 57.14% 0 Missing and 3 partials ⚠️
datafusion/datasource/src/memory.rs 81.81% 0 Missing and 2 partials ⚠️
... and 7 more
Additional details and impacted files
@@            Coverage Diff            @@
##             main   #24483     +/-   ##
=========================================
  Coverage   81.23%   81.24%             
=========================================
  Files        1111     1113      +2     
  Lines      390208   392929   +2721     
  Branches   390208   392929   +2721     
=========================================
+ Hits       316990   319219   +2229     
- Misses      54591    54917    +326     
- Partials    18627    18793    +166     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate datasource Changes to the datasource crate functions Changes to functions implementation logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants