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!: bump polars to unreleased 2023-12-21 version #601

Merged
merged 35 commits into from
Dec 23, 2023

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Dec 17, 2023

It references the same commit as py-polars 0.20.0 0.20.2.

I've been busy lately so I don't know if I'll be able to finish this, but I'd like to finish this and create a new release if possible.

@etiennebacher
Copy link
Collaborator

etiennebacher commented Dec 17, 2023

Can we wait for rust-polars 0.36? It is just easier to bump when there's a changelog for Rust-polars and we always bumped Rust versions when there was a rust-polars release.

I can ask on discord when they plan to release 0.36

@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 17, 2023

It's okay to wait, but since we don't know when it will be released, I thought it might be a good idea to release it with what we have now.

@stinodego Are there any plans for the next release of Rust-polars?
I'm also curious about whether or not you intend to update the version of rust-polars to 1.0.0. (The discussion looks mostly like py-polars' stuff and I have a feeling that rust-polars won't make it to 1.0.0)

@eitsupi eitsupi added this to the 0.12 milestone Dec 17, 2023
@stinodego
Copy link
Member

stinodego commented Dec 17, 2023

1.0.0 for Rust Polars is a long way off. It will definitely not coincide with 1.0.0 for Python Polars.

I'm not sure when 0.36 comes out. The release cadence for Rust Polars is basically whenever Ritchie decides to press the button 😄

@etiennebacher
Copy link
Collaborator

Thanks @stinodego :) @ritchie46 do you already have a date in mind (or an estimation)?

R/datatype.R Outdated Show resolved Hide resolved
@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 19, 2023

I have fixed as many of the problems as I can but it is not complete yet.
Unfortunately I don't have any more time this week so it would be great if someone could finish this.

@etiennebacher
Copy link
Collaborator

etiennebacher commented Dec 19, 2023

I'll work on this on Friday, maybe before. It would be amazing if rust-polars was bumped in the meantime but I'll continue this anyway

@eitsupi eitsupi changed the title feat!: bump polars to unreleased 2023-12-16 version feat!: bump polars to unreleased 2023-12-21 version Dec 22, 2023
@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 22, 2023

I don't think the error in the describe example is directly related to this PR; describe in py-polars is written in python, which needs to be ported to R.
New issue #614

@etiennebacher
Copy link
Collaborator

We can use mtcars instead of iris in the example to circumvent this temporarily.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 22, 2023

The state of sink_csv is strange, but I don't know what's causing it.
In my local case, when I run sink_csv to a tempfile(), the R session freezes with the CSV left in an incomplete state.

library(polars)
temp_out <- tempfile()
pl$LazyFrame(mtcars)$sink_csv(temp_out)

@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 22, 2023

Strangely, it doesn't freeze when I give a string to the path argument.

library(polars)
temp_out = tempfile()
temp_out
#> [1] "/tmp/RtmpXe0CL2/file3d5d188b4813"
pl$DataFrame(mtcars)$lazy()$sink_csv("/tmp/RtmpXe0CL2/file3d5d188b4813")
pl$DataFrame(mtcars)$lazy()$sink_csv(temp_out)

Edit: Ah, sorry, now I know the cause. If I try to write to the same path a second time it freezes.
The exact same thing happens with py-polars.

@eitsupi eitsupi marked this pull request as ready for review December 22, 2023 15:00
Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Thanks a lot @eitsupi, great work! I have minor comments but also I came across some weird panics when I tried to update results of rollbench so I'd like to ensure this is upstream and not on the R side.

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
#' @return DataFrame
pl$read_csv = function(
read_csv = function( # remapped to pl$read_csv, a hack to support roxygen2 @inheritsParams
Copy link
Collaborator

Choose a reason for hiding this comment

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

cool hack, nice to get rid of all these duplicated docs

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 thinks this is related to #560 and the function name should be pl_read_csv.
This function should be add to the pl environment when loading.

Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Can't reproduce the panics in rollbench code, so fine for me

@etiennebacher
Copy link
Collaborator

Also should we bump the r-polars version in cargo.toml?

@etiennebacher
Copy link
Collaborator

I added $replace() which should be fine to do here since it was rewritten in py-polars 0.20.2 (so it fits the objective of this PR)

@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 23, 2023

Thanks for updates, @etiennebacher.

@eitsupi eitsupi merged commit 6942e63 into main Dec 23, 2023
23 checks passed
@eitsupi eitsupi deleted the bump-polars-to-py-0.20.0 branch December 23, 2023 00:54
@eitsupi
Copy link
Collaborator Author

eitsupi commented Dec 23, 2023

Also should we bump the r-polars version in cargo.toml?

The current version has never been released, so the same number is fine.

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.

None yet

3 participants