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

refactor!: rewrite <DataFrame>$get_columns() #991

Merged
merged 3 commits into from
Mar 30, 2024
Merged

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Mar 30, 2024

No description provided.

@eitsupi eitsupi marked this pull request as ready for review March 30, 2024 15:55
Comment on lines +199 to +201
let cols = self.0.get_columns().to_vec();
let vec = unsafe { std::mem::transmute::<Vec<pl::Series>, Vec<RPolarsSeries>>(cols) };
List::from_values(vec)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change, the original was readable and didn't have any unsafe

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is copied from Python.
What is the problem with unsafe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is copied from Python.

I didn't see it in get_columns() but indeed to_pyseries() has that. So I'm fine with that

What is the problem with unsafe?

I don't have a great knowledge of Rust, so I'd rather avoid unsafe since I have no idea in which context it's fine and in which it's not. Maybe there's no problem in this case, but I don't know that in advance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can completely eliminate unsafe in terms of accessing the C ABI of Python or R.
(As evidenced by the fact that we sometimes crash R sessions.)

@eitsupi eitsupi merged commit acc29eb into main Mar 30, 2024
25 checks passed
@eitsupi eitsupi deleted the rewrite-get_columns branch March 30, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants