fix(proto): check plan integer conversions across usize boundaries - #24483
Conversation
github-actions
Bot
added
logical-expr
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24483 +/- ##
==========================================
- Coverage 81.45% 81.45% -0.01%
==========================================
Files 1119 1120 +1
Lines 400411 401470 +1059
Branches 400411 401470 +1059
==========================================
+ Hits 326144 327006 +862
- Misses 55195 55308 +113
- Partials 19072 19156 +84 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
# Conflicts: # datafusion/physical-plan/src/joins/hash_join/exec.rs
|
@adriangb can you review when you find time? |
There was a problem hiding this comment.
Pull request overview
Adds checked usize/protobuf integer conversions across logical and physical plan serialization to prevent truncation on 32-bit targets.
Changes:
- Introduces shared checked wire-conversion helpers.
- Applies them to limits, sizes, capacities, partition counts, indexes, and statistics.
- Adds boundary, malformed-input, sentinel, and zero-batch-size tests.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
datafusion/common/src/utils/mod.rs |
Adds shared checked conversion helpers. |
datafusion/expr/src/logical_plan/plan.rs |
Checks SQL limit and offset conversions. |
datafusion/functions-table/src/generate_series.rs |
Rejects zero batch sizes. |
datafusion/proto/src/logical_plan/mod.rs |
Checks logical-plan integer conversions and limit sentinels. |
datafusion/proto/src/physical_plan/mod.rs |
Checks generate-series batch sizes. |
datafusion/proto-common/src/from_proto/mod.rs |
Treats oversized statistics as unknown. |
datafusion/physical-expr/src/partitioning.rs |
Checks partition-count conversions. |
datafusion/physical-expr/src/scalar_subquery.rs |
Checks scalar-subquery index encoding. |
datafusion/physical-plan/src/aggregates/mod.rs |
Checks aggregate limits. |
datafusion/physical-plan/src/buffer.rs |
Checks buffer capacity decoding. |
datafusion/physical-plan/src/coalesce_batches.rs |
Checks sizes and rejects zero batches. |
datafusion/physical-plan/src/coalesce_partitions.rs |
Checks fetch conversions. |
datafusion/physical-plan/src/empty.rs |
Checks partition-count encoding. |
datafusion/physical-plan/src/filter.rs |
Checks sizes and validates batch sizes. |
datafusion/physical-plan/src/joins/hash_join/exec.rs |
Uses shared checked fetch decoding. |
datafusion/physical-plan/src/limit.rs |
Checks global and local limits. |
datafusion/physical-plan/src/placeholder_row.rs |
Checks partition-count encoding. |
datafusion/physical-plan/src/sorts/sort.rs |
Checks sort fetch conversions. |
datafusion/physical-plan/src/sorts/sort_preserving_merge.rs |
Checks merge fetch conversions. |
datafusion/physical-plan/src/unnest.rs |
Checks struct-column indexes. |
datafusion/physical-plan/src/windows/window_agg_exec.rs |
Checks window-column indexes. |
datafusion/datasource/src/file_scan_config/proto.rs |
Checks scan limits and batch sizes. |
datafusion/datasource/src/memory.rs |
Checks memory-scan fetch values. |
datafusion/proto/tests/cases/plans/leaves.rs |
Tests oversized partition counts. |
datafusion/proto/tests/cases/plans/limits.rs |
Tests limit boundaries and zero batches. |
datafusion/proto/tests/cases/plans/misc.rs |
Updates partitioning error assertions. |
datafusion/proto/tests/cases/roundtrip_logical_plan.rs |
Tests logical limits and malformed values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
Other than the public API question this seems like a good change! Without having counted I'm going to say that if making these pub(crate) would require duplicating in 3 or more places (not counting tests) then we should keep it pub as is. If it's 2 or less places lets just duplicate it. I hope that's a fair unbiased opinion 😄
| pub fn usize_from_wire<T>(value: T, context: &str, field: &str) -> Result<usize> | ||
| where | ||
| T: TryInto<usize> + std::fmt::Display + Copy, | ||
| { | ||
| value.try_into().map_err(|_| { | ||
| _plan_datafusion_err!( | ||
| "{context}: {field} wire value {value} is out of range for usize" | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| /// Converts a `usize` to a wire integer, rejecting out-of-range values. | ||
| pub fn usize_to_wire<T: TryFrom<usize>>( |
There was a problem hiding this comment.
Do these need to be public in this crate, or is there a version where they're private and close the the callers? I'd almost prefer to duplicate a small helper like this in 2 crates than expose it to the public API.
There was a problem hiding this comment.
I've checked and counted these helpers are used in 4 crates in this PR. Moreover, I've a followup work on this issue to wrap it up and will use helpers there as well. Thus, keeping them public as you've advised
Which issue does this PR close?
usizeon 32-bit targets #24170.Rationale for this change
Several logical and physical plan protobuf paths convert wire integers to
usizewith unchecked casts. On 32-bit targets, values aboveu32::MAXsilently truncate. For row limits, a fetch of1 << 32becomes0, causing a plan that should return rows to return an empty result.The reverse conversion can also truncate when a 64-bit
usizedoes not fit the protobuf field.What changes are included in this PR?
usize, with errors identifying the plan node and field.-1sentinel for limits without a fetch.FilterExecpayloads.usizestatistics 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
Valuesnodes, 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.