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

Update Polars on Arrow2 and implement Polars on Arrow-rs #726

Merged
merged 23 commits into from
Jan 21, 2025

Conversation

EricFecteau
Copy link
Collaborator

@EricFecteau EricFecteau commented Jan 11, 2025

Added the ability to send the data to a modern version of Polars from both Arrow and Arrow2, through the FFI.

Some design decisions were made along the way (described here: #720), but more than willing to change those decisions! Had to disable two tests for Arrow2 as the data was not able to be moved between arrow2 and arrow_polars and these the functionality needed was also not implemented in Arrow.

@@ -71,11 +72,12 @@ iai = "0.1"
pprof = {version = "0.5", features = ["flamegraph"]}

[features]
all = ["src_sqlite", "src_postgres", "src_mysql", "src_mssql", "src_oracle", "src_bigquery", "src_csv", "src_dummy", "src_trino", "dst_arrow", "dst_arrow2", "federation", "fed_exec"]
all = ["src_sqlite", "src_postgres", "src_mysql", "src_mssql", "src_oracle", "src_bigquery", "src_csv", "src_dummy", "src_trino", "dst_arrow", "dst_arrow2", "dst_arrow", "federation", "fed_exec", "dst_polars"]
Copy link
Contributor

Choose a reason for hiding this comment

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

a duplicate dst_arrow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Fixed!

@wangxiaoying
Copy link
Contributor

Thanks for drafting the design and submitting the PR! @EricFecteau

I will take a deeper look at this by this weekend.

@wangxiaoying
Copy link
Contributor

wangxiaoying commented Jan 17, 2025

@EricFecteau , the code looks good to me! In terms of resolving the array type issue, I left a comment in the discussion

@EricFecteau
Copy link
Collaborator Author

EricFecteau commented Jan 18, 2025

@wangxiaoying This was simpler than I anticipated (great Crate design)! I implemented arrays for arrow and reverted to using polars_old = 0.32 for arrow2. I am interested in doing another PR to remove arrow2 from the crate, but I think it deserves it’s own PR. Now all the test (plus the tests I added) pass. Please let me know what you think.

More info in the discussion: #720 (reply in thread)

@wangxiaoying
Copy link
Contributor

Thank you for the update @EricFecteau ! I did some minor changes but I think the code looks good in general. Will merge it after the CI passes.

I left some comments in the discussion for concrete questions.

@EricFecteau
Copy link
Collaborator Author

This PR also closes #657.

@wangxiaoying wangxiaoying merged commit 652d088 into sfu-db:main Jan 21, 2025
2 checks passed
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