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

Fixed compilation errors #1

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

Fixed compilation errors #1

wants to merge 2 commits into from

Conversation

pteridin
Copy link

@pteridin pteridin commented Jun 4, 2024

Fixed some compilation error. Build works, but polars:::export_df_to_arrow_stream has been removed, thus still unusable. Unable to find equivalent for quick fix.

Copy link
Member

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

This repository was left alone because it seems to have no real users.
What is your use case?

polars:::export_df_to_arrow_stream has been removed, thus still unusable. Unable to find equivalent for quick fix.

Currently the polars package can exchange Arrow Streams via a string representing a pointer to the Arrow Stream, but it is actually possible to directly reference an external pointer to bring it into the Rust side. I think we need to rewrite the export_stream() and import_stream() methods to do so.
I don't have time this week and will be able to work on that next week, but if you are interested, please refer to the following sections to work on it.
https://github.com/yutannihilation/datafusion-r/blob/d908c42c4703d581bbcc0e5522d0ad6f98f2c783/src/rust/src/lib.rs#L42-L49

Comment on lines -13 to -14
polars = { version = "*", default_features = false, features = ["dtype-struct"]}
polars-core = { version = "*", default_features = false }
Copy link
Member

Choose a reason for hiding this comment

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

These changes are needed?

Copy link
Author

Choose a reason for hiding this comment

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

cargo warned me that this parameter is not available for polars & polars_core. To be honest: I am unsure about the implications of these changes and just wanted to remove compiler warnings along the way.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change is ideal here because the default functionality includes extra stuff.

@pteridin
Copy link
Author

pteridin commented Jun 5, 2024

it is actually possible to directly reference an external pointer to bring it into the Rust side

Interesting. This looks like a much cleaner and performant solution.

Let me know if I can assist you in any way. There is no need to hurry to implement it from my side, but it would open up some interesting opportunities for some projects that require extensive number crunching. I try to make room to have a look at the datafusion-r implementation.

@eitsupi
Copy link
Member

eitsupi commented Jun 9, 2024

Currently the polars package can exchange Arrow Streams via a string representing a pointer to the Arrow Stream, but it is actually possible to directly reference an external pointer to bring it into the Rust side. I think we need to rewrite the export_stream() and import_stream() methods to do so.

I have tried this a bit and it seems impossible because there is no API in extendr to cast an external pointer to something else.

@eitsupi
Copy link
Member

eitsupi commented Jun 9, 2024

There is no need to hurry to implement it from my side, but it would open up some interesting opportunities for some projects that require extensive number crunching.

Honestly, I doubt that using Polars from outside of Python (i.e. Rust, R, etc.) is worth the maintenance cost, and I would recommend considering alternatives such as DuckDB or DataFusion.

@pteridin
Copy link
Author

pteridin commented Jun 9, 2024

Honestly, I doubt that using Polars from outside of Python (i.e. Rust, R, etc.) is worth the maintenance cost, and I would recommend considering alternatives such as DuckDB or DataFusion.

Absolutely fair. It would actually be interesting if you found a way to actually extract the forementioned pointer string, just out of curiosity. But do not put effort into that.

@eitsupi
Copy link
Member

eitsupi commented Jun 9, 2024

I opened pola-rs/r-polars#1138.
It appears that there is a segmentation error on CI.

I have looked at this for a few hours and have not been able to resolve it. Feel free to edit that.

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