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 rust-polars to 0.30.0 #289

Merged
merged 31 commits into from
Jul 4, 2023
Merged

feat!: bump rust-polars to 0.30.0 #289

merged 31 commits into from
Jul 4, 2023

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Jul 2, 2023

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jul 2, 2023

The errors in Rust have been fixed but have not yet passed the R tests.
Feel free to edit this branch as I may not have enough time to work on this.

@etiennebacher
Copy link
Collaborator

I think this should close #230

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jul 2, 2023

I fixed as many of the test failures as I could.

The remaining failures are a problem where date_range_lazy now returns a list, so when converted to R, it becomes a list instead of a vector. (pola-rs/polars#8513)

@eitsupi eitsupi linked an issue Jul 2, 2023 that may be closed by this pull request
@sorhawell sorhawell self-assigned this Jul 2, 2023
@sorhawell
Copy link
Collaborator

Thanks. I will take a look at it.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jul 2, 2023

I was just looking at this myself and Python's date_range behaves quite strangely.
I mean, when eager=True it returns an un-nested Series (r-polars' current behavior, lazy=TRUE/FALSE), but when eager=False (default now) it returns a list type, which I think is more appropriate here given the meaning of range, but anyway it returns something completely different for True and Flase.
Some tests must be removed if this is to be mimicked.

@sorhawell
Copy link
Collaborator

@eitsupi

I found polars did a behavior change on date_range such that it produces Eager = False a list of DateRanges.
It is being discussed here

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jul 3, 2023

I found polars did a behavior change on date_range such that it produces Eager = False a list of DateRanges.
It is being discussed here (pola-rs/polars#9019)

Oh, thank you for finding that.

The change definitely makes sense, but we cannot incorporate that change here because it has not yet been made in the main branch.
Perhaps we can change the lazy=FALSE behavior in this repository using implode? (This is different from the behavior of py-polars, but since it will certainly be changed in py-polars in the future, it does not seem to be a problem to change it ahead of time.)

@sorhawell
Copy link
Collaborator

sorhawell commented Jul 3, 2023

One timezone error remaining and the we have restored previous behavior.

We can update the date_range(s) behavior in a new PR after this has merged

@sorhawell
Copy link
Collaborator

tiny detail for reference "fix subtle typo bug" is an incorrect commit msg. There was a unrelated subtle bug (high/low) which did not show, and then I disabled a test on timezone until a future refactor.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jul 4, 2023

All tests now pass; I would like to defer the renaming from list to arr on the R side to a follow-up fix as it would be an additional major task.

@eitsupi eitsupi marked this pull request as ready for review July 4, 2023 01:49
@eitsupi eitsupi added this to the 1st CRAN Release milestone Jul 4, 2023
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.

Looks great, thanks for updating this. It's fine for me to do all the renaming from arr to list in a separate PR, are you also ok with this @sorhawell ?

NEWS.md Outdated Show resolved Hide resolved
@@ -706,20 +700,20 @@ ExprStr_slice = function(offset, length = NULL) {
unwrap("in str$slice:")
}

#' explode
#' @name ExprStr_explode
#' str_explode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be better to have a proper title here (not just for this function)

Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@eitsupi
Copy link
Collaborator Author

eitsupi commented Jul 4, 2023

@etiennebacher Thanks for your review.

Since this PR contains updates to the toolchain and everything needs to be recompiled every time we switch branches, I think it's better to merge it in sooner rather than later, so I'm merging it.
Additional modifications can be made later.......

@eitsupi eitsupi merged commit d155ea4 into main Jul 4, 2023
11 checks passed
@eitsupi eitsupi deleted the rust-polars-0.30 branch July 4, 2023 07:06
vincentarelbundock pushed a commit to vincentarelbundock/r-polars that referenced this pull request Jul 4, 2023
Co-authored-by: sorhawell <sorhawell@gmail.com>
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
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