fix: return execution error instead of capacity overflow panic in array_resize#23306
fix: return execution error instead of capacity overflow panic in array_resize#23306buraksenn wants to merge 2 commits into
Conversation
| /// Largest element count whose eager value buffer stays within `isize::MAX` | ||
| /// bytes, so `array_resize` rejects oversized results instead of panicking. | ||
| /// Only primitive and `FixedSizeBinary` leaves are byte-exact. | ||
| fn max_resize_values(value_type: &DataType) -> usize { |
There was a problem hiding this comment.
the compiler prob materialize it, but would it be better to prematerialize it manually through
static LazyLock<HashMap<DataType, usize>> ?
There was a problem hiding this comment.
Since there are parameterized types I would need to keep match right? I'm not sure it would be better but can't say I've a strong opinion. If you say go for it I can change it that way
|
benchmark run array_resize |
|
run benchmark array_resize |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-array-resize-overflow-panic (26928b9) to e4aa41d (merge-base) diff using: array_resize File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_resize — base (merge-base)
array_resize — branch
File an issue against this benchmark runner |
| fn max_resize_values(value_type: &DataType) -> usize { | ||
| let element_width = match value_type { | ||
| DataType::FixedSizeBinary(size) if *size > 0 => { | ||
| (*size as usize).saturating_mul(u8::BITS as usize) |
There was a problem hiding this comment.
do we need .saturating_mul(u8::BITS as usize)?
There was a problem hiding this comment.
we don't actually I actually mixed size of bytes and bits there. It just narrowed the limit 1/8 but size is already byte and we should return it there. changing this and rerunning tests now
There was a problem hiding this comment.
thanks for the heads up 🙏
Which issue does this PR close?
Rationale for this change
Please check #22227
What changes are included in this PR?
Instead of panicking we return execution error buy checking sizes in array_resize.
Outcome is:
in issue it was:

Are these changes tested?
yes. added new unit and slt tests
Are there any user-facing changes?
instead of panic users will see execution error