Skip to content
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

Merged
merged 9 commits into from
Dec 15, 2024

Conversation

balbok0
Copy link
Contributor

@balbok0 balbok0 commented Dec 12, 2024

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.

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Dec 12, 2024
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 95.87629% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.60%. Comparing base (bcca075) to head (873ca9a).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/polars-python/src/expr/binary.rs 78.57% 3 Missing ⚠️
crates/polars-plan/src/dsl/function_expr/binary.rs 91.66% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ritchie46 ritchie46 left a 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.

py-polars/polars/expr/binary.py Outdated Show resolved Hide resolved
crates/polars-python/src/expr/binary.rs Show resolved Hide resolved
to,
is_little_endian,
)))
}
Copy link
Member

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"),
Copy link
Member

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?

Copy link
Contributor Author

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.

@balbok0 balbok0 force-pushed the add-binary-as-numerical-updated branch from 801ad75 to 417f6dc Compare December 14, 2024 21:52
@ritchie46 ritchie46 changed the title feat: Add binary as numerical (updated) feat: Add bin.reinterpret Dec 15, 2024
@ritchie46 ritchie46 merged commit da51207 into pola-rs:main Dec 15, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants