feat: Add support for unsigned 128-bit integers#24346
Conversation
dsprenkels
changed the title
github-actions
Bot
added
enhancement
dsprenkels
added
do not merge
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24346 +/- ##
==========================================
- Coverage 81.80% 81.78% -0.02%
==========================================
Files 1684 1684
Lines 228518 228740 +222
Branches 2929 2929
==========================================
+ Hits 186931 187075 +144
- Misses 40850 40928 +78
Partials 737 737 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dsprenkels
requested review from
MarcoGorelli,
alexander-beedie,
c-peters,
coastalwhite,
orlp,
reswqa and
ritchie46
as code owners
| // One side is signed, the other is unsigned. We just need to upcast the | ||
| // unsigned side to a signed integer with the next-largest bit width. | ||
| match (l, r) { | ||
| (UInt128, _) | (_, UInt128) => Some(Int128), |
There was a problem hiding this comment.
Do things fail if this returns None? Because this function claims the cast must be lossless, which this isn't.
There was a problem hiding this comment.
Good catch. I updated this
| assert_frame_equal(result, expected) | ||
|
|
||
|
|
||
| def test_from_dicts_schema_columns_do_not_match() -> None: |
There was a problem hiding this comment.
Not sure what happened here. I re-added the test
| max_size=10, | ||
| excluded_dtypes=[ | ||
| pl.Int128, | ||
| pl.UInt128, |
There was a problem hiding this comment.
Why were both Int128 and UInt128 added to the excluded_dtypes? If i128 was already there I could maybe understand it but since it used to work before it should still work now, no?
There was a problem hiding this comment.
I128 is not tested in parametric tests on main. In this PR I have added both U128 and I128 to the parametric testing types. However, I128 does not work for this test, because we serialize I128 to a custom _pli128 column (which is not supported by PyArrow).
| pl.UInt32, | ||
| pl.UInt64, | ||
| pl.Int128, | ||
| # TODO: pl.UInt128 literals are too large for Polars |
There was a problem hiding this comment.
Perhaps could you still test UInt128 and just add an exception for dtype.max() only for UInt128?
orlp
left a comment
orlp
left a comment
There was a problem hiding this comment.
Some small questions, but other than that great work!
3 participants
This PR is still a draft. I'll undraft it after double-checking some things tomorrow.This PR is ready