-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add bin.reinterpret
#20263
feat: Add bin.reinterpret
#20263
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20263 +/- ##
==========================================
- Coverage 79.60% 79.60% -0.01%
==========================================
Files 1565 1567 +2
Lines 218396 218413 +17
Branches 2477 2465 -12
==========================================
Hits 173860 173860
- Misses 43970 43984 +14
- Partials 566 569 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I've left a few comments.
to, | ||
is_little_endian, | ||
))) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line char
(pl.Int32, 4, "i"), | ||
(pl.UInt32, 4, "I"), | ||
(pl.Int64, 8, "q"), | ||
(pl.UInt64, 8, "Q"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test pl.Int128
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
Although, I should note that struct or numpy doesn't support ints. I created a second test function meant specifically for exotic ints (since it seems that UInt128 is WIP).
Reason to keep the old one, and not switch to this approach is the fact that float
doesn't have to_bytes
function.
801ad75
to
417f6dc
Compare
bin.reinterpret
Related to #18991 Follow-up to #19542
Addressed comments from previous MR, although please let me know if the placement of code is still not where it is expected.
Implements from_buffer method that casts from Binary to Numerical types. It is not the most efficient (i.e. size + reinterpret cast would probably better), but provides a consistent code for both LE and BE encodings. Name is inspired by numpy equivalent, but with polars style _ separator between words.
I also added match arm in cast that goes from Binary -> numerical type. It assumes LE encoding, as it is a vastly more popular from the two. Happy to remove that part of code, since it does make an unspecified assumption. Lastly, I noticed that there is missing Numerical -> Binary functionality. Out-of-scope for this MR IMO, but something I can do later on.