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

Implement FromPyObject and IntoPy Traits for AnyValue #37

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

JabobKrauskopf
Copy link

I added a new PyAnyValue wrapper around AnyValue and implemented the FromPyObject, IntoPy, From and AsRef traits for the new wrapper

Any feedback is welcome!

See issues: #10

I hope we can get this merged! :D

@JabobKrauskopf
Copy link
Author

I saw that both unwrap and expect were used in the rest of the library. Let me know if you have any preferences and if I should improve the error handling for PyAnyValue

@FloLimebit
Copy link

FloLimebit commented Nov 20, 2023

This feature would be super helpful - is there any update on the PR already? :)

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.

Sorry for the late reply. Left some comments.

@@ -145,6 +162,73 @@ impl<'a> FromPyObject<'a> for PyDataFrame {
}
}

impl<'a> FromPyObject<'a> for PyAnyValue<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

}
);

impl IntoPy<PyObject> for PyAnyValue<'_> {
Copy link
Member

Choose a reason for hiding this comment

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

idem, I think this should copy from py-polars.

@JabobKrauskopf
Copy link
Author

@ritchie46 Thanks for the review and yeah it makes total sense to copy the implementation from py-polars.
I tried to copy everything that is needed but need help in some parts that I'll mention in other comments.
I'm still very new to rust so I still find it a bit difficult finding my way around in a big project like polars, so any help is appreciated :)

P.S.: Thank you for your amazing work on polars. It's been a huge factor in wanting to learn rust

// TODO: need help here
AnyValue::Array(_v, _) | AnyValue::List(_v) => {
todo!();
// PySeries(v).to_list()
Copy link
Author

Choose a reason for hiding this comment

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

Help needed :)

Implementing the to_list function for the PySeries would add a lot more code because all of this would need to be copied: https://github.com/pola-rs/polars/blob/main/py-polars/src/series/export.rs#L16 including the implementation and all the traits for the DataType.

Am i correct in assuming this?
I'm feeling confident that I could "implement" all of this but I didn't want to get started before knowing if I'm actually correct

}
ref av @ AnyValue::Struct(_, _, flds) => struct_dict(py, av._iter_struct_av(), flds),
AnyValue::StructOwned(payload) => struct_dict(py, payload.0.into_iter(), &payload.1),
// TODO: Also need help here
Copy link
Author

Choose a reason for hiding this comment

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

Help needed :)

Same as with the DataType struct, implementing the ObjectValue struct would lead to a lot of new code that would need to be copied.

Maybe we could just not support this feature? Open to ideas :)

unsafe impl<T: Send + Sync> Sync for GILOnceCell<T> {}
unsafe impl<T: Send> Send for GILOnceCell<T> {}

impl<T> GILOnceCell<T> {
Copy link
Author

@JabobKrauskopf JabobKrauskopf Mar 21, 2024

Choose a reason for hiding this comment

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

This implementation of the GILOnceCell can only be used if the users of this library also include a function that is called "on startup", that initializes the value in the Cell, similar to https://github.com/pola-rs/polars/blob/97eff077f37209386836361e9ab4da582bc5b18e/py-polars/src/on_startup.rs#L98

I don't think that this requirement would be a good idea. We could check in the with_gil function if the value in the cell has been initialized, and if not initialize it there

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.

3 participants