Repository navigation
fix: raise a validation error for out-of-bounds integer fill values - #4492
barlowa124 wants to merge 9 commits into
Conversation
from_json_scalar cast the parsed integer into the native dtype without a range check, so fill_value 300 for int8 surfaced a numpy OverflowError instead of a validation error. Bounds-check against np.iinfo before casting; int, intish float, and intish string forms are covered. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4492 +/- ##
==========================================
+ Coverage 94.69% 94.71% +0.01%
==========================================
Files 94 94
Lines 13619 13666 +47
==========================================
+ Hits 12897 12944 +47
Misses 722 722
🚀 New features to boost your workflow:
|
|
|
||
| info = np.iinfo(self.to_native_dtype()) | ||
| if value < info.min or value > info.max: | ||
| raise TypeError( |
There was a problem hiding this comment.
use ValueError. An out-of-bounds int is still an int, so it's not a TypeError
we should use the same parsing path for both, so users see consistent errors |
d-v-b
left a comment
There was a problem hiding this comment.
Requested changes:
- Raise
ValueErrorfor an out-of-range value, notTypeError, and drop "Invalid type" from the message. Use{data!r}so a string fill like"300"shows as a string. - Update the
Raisessection offrom_json_scalar's docstring to cover the out-of-range case. - Split
test_out_of_bounds_integer_from_json_scalarinto two parametrized tests, one for valid values and one for out-of-range values, so one failing case doesn't hide the rest. - Use single backticks in
changes/4492.bugfix.md, like the other fragments, and say the fix covers reading stored metadata only.
Optional: share the bounds check with cast_scalar, so create_array(dtype="i1", fill_value=300) raises the same error instead of numpy's OverflowError.
…shared bounds check
- Raise ValueError (not TypeError) for out-of-range values and drop
"Invalid type" from the message; format input with {data!r}
- Document the ValueError in the Raises sections of from_json_scalar
and cast_scalar
- Split the regression test into parametrized in-bounds and
out-of-bounds tests
- Share the bounds check with cast_scalar so create_array with an
out-of-range fill_value raises the same error (optional item)
- Changelog: single backticks, cover both read and write paths
…arr-python into int-fill-range-4453
|
e360200 covers the review: out-of-range ints raise 9ea1426 fixes a mypy arg-type error in the new test. |
Summary
Partial fix for #4453, item 4 first bullet.
ArrayV3Metadata.from_dictwith an out-of-range integer fill value leaked NumPy's
OverflowErrorinstead of raising a validation error:
BaseInt.from_json_scalarnow bounds-checks the parsed integer againstnp.iinfobefore casting, so all three accepted JSON forms (int,integer-valued float, integer string) get the same
TypeErrorthenon-integer path already raises. In-range behavior is unchanged.
For reviewers
The parse path only:
cast_scalarstill uses NumPy cast semantics, so anout-of-range fill passed through the create API keeps its current
behavior. The remaining item-4 bullets (byte-array
bytesfills,r*raw-bits dtypes) are not addressed here.
Author attestation
TODO
docs/user-guide/*.mdchanges/