fix: exact int64 boundary handling in numeric finalization (#34) - #120
Merged
Conversation
_finalize_numeric guarded int64 conversion with float(nonnull.abs().max()) < float(2**63 - 1024), which is wrong on both edges of the range: * magnitude-only: -2**63 is a legitimate int64 but its magnitude is 2**63, so the column was demoted to float64; * strict-below-threshold: 2**63 - 1024 (the largest float64 value below 2**63) round-trips through int64 exactly but was rejected. The guard now compares min()/max() in Python-int space against the exact int64 bounds. Python ints are arbitrary precision, so there is no rounding at the boundary; inf raises OverflowError and is treated as not fitting. Integrality is already established by the % 1 == 0 check before the bounds check runs. Note: issue #34 proposed float(2**63 - 1) with <=, but that constant rounds UP to 2**63 in float64 and would admit a value that wraps around in .astype("int64") — the integer-space comparison avoids the whole class of float-threshold bugs. Closes #34
Contributor
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesInt64 finalization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|---|---|---|---|---|---|---|---|---|---|---|
| crm | 10,200 | 40 | 1.298 | 1.317 | 14.0 | 100.0 | 0.0 | 100.0 | 93.84 | ✅ | 100.0 |
| event_log | 10,000 | 25 | 0.475 | 0.487 | 5.1 | 100.0 | 0.0 | 100.0 | 99.677 | ✅ | 100.0 |
| finance | 10,200 | 60 | 1.650 | 1.657 | 18.3 | 100.0 | 0.0 | 100.0 | 99.499 | ✅ | 100.0 |
| gold | 10,200 | 7 | 0.195 | 0.199 | 3.7 | 100.0 | 0.0 | 100.0 | 98.3 | ✅ | 100.0 |
| provenance | 10,000 | 18 | 0.406 | 0.407 | 7.3 | 100.0 | 0.0 | 100.0 | 99.765 | ✅ | 100.0 |
| wide_schema | 10,000 | 100 | 2.998 | 3.033 | 13.4 | 100.0 | 0.0 | 100.0 | 96.042 | ✅ | 100.0 |
Authored-code reduction (Metric 6)
- FreshData: 3 lines
- pandas baseline: 26 lines (88.46% reduction)
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #34.
_finalize_numericusedfloat(nonnull.abs().max()) < float(2**63 - 1024)to decide whether an integral column fits int64. Two real defects:-2**63is a valid int64, but its magnitude (2**63) fails the check → needless float64 demotion of legitimate data.2**63 - 1024— the largest float64 below2**63— converts to int64 exactly, but strict<rejected it → float64 demotion.Why the issue's proposed fix is unsafe
The issue suggests
_INT64_SAFE = float(2**63 - 1)with<=. That constant rounds up to2**63in float64 (float(2**63 - 1) == float(2**63)isTrue), so the proposed guard would admitfloat(2**63)and let.astype("int64")wrap around — it would introduce the very overflow the issue worries about. Notably, the current code does not actually overflow (float rounding aligns at the 1024-ulp boundary); its failure mode is wrong dtype decisions, not corruption. The PR description of record for #34 should reflect that.Fix
Compare
min()/max()in Python-int space against exact int64 bounds (_fits_int64). Python ints are arbitrary-precision → no rounding at the boundary;int()is exact because integrality (% 1 == 0) is checked first;infraisesOverflowError→ treated as not fitting.Behavior/compat
Values in
{-2**63} ∪ [2**63 - 1024](and int64-extreme integers arriving via object/int paths) now finalize as int64/Int64 instead of float64 — numerically identical, dtype now correct. Everything else unchanged.Tests
test_finalize_numeric_int64_boundaries_no_overflow_no_demotionandtest_finalize_numeric_int64_min_not_rejected_by_abs_asymmetry— both fail on main (float64 demotion), pass with the fix; also pin thatfloat(2**63)stays float64 and the nullable Int64 path is exact.test_huge_integer_strings_still_stay_float_end_to_end— pipeline-level control (2**64 stays float64).Verification
Summary by CodeRabbit