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!: update built-in Rust Polars to 2023-04-20 #183

Merged
merged 40 commits into from
May 2, 2023
Merged

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Apr 27, 2023

I have not yet been able to correct the errors by breaking changes.

If anyone wants to work on this, you can push it to this branch.

The upstream includes various breaking changes.
Two in particular that affect many areas are

TODO

  • Add more tests for new options
    • dt_replace_time_zone's use_earliest
    • str_parse_int's strict
  • Update NEWS about new features and breaking changes

@eitsupi eitsupi changed the title [WIP] chore: update polars [WIP] chore: update built-in Rust Polars Apr 27, 2023
src/rust/src/lazy/dsl.rs Outdated Show resolved Hide resolved
src/rust/src/lazy/dsl.rs Outdated Show resolved Hide resolved
@vincentarelbundock

This comment was marked as outdated.

@eitsupi

This comment was marked as outdated.

@sorhawell
Copy link
Collaborator

Sometimes in the past when bumping rust-polars, it can become quite messy with 10-20 hotfixes here and there. The breaking changes are fortunately easy to catch when compiling. Non breaking changes can be hard to notices and update the code base for. Maybe polars supports some new join strategy, but r-polars will not allow before someone files an issue.

@sorhawell sorhawell self-assigned this Apr 27, 2023
@eitsupi

This comment was marked as outdated.

@sorhawell
Copy link
Collaborator

I fixed all compiler errors. Still missing: update tests + evaluate rust-polars changes and file some issues.

@vincentarelbundock

This comment was marked as outdated.

@eitsupi

This comment was marked as outdated.

@eitsupi
Copy link
Collaborator Author

eitsupi commented May 1, 2023

Remain test failures

  • Error (test-expr_arr.R:4:3): arr$lengths
    > pl$DataFrame(list_of_strs = pl$Series(list(list(),NULL)))
    Error: in Series.new: SchemaMismatch(ErrString("invalid series dtype: expected `List`, got `f64`"))
  • Error (test-expr_arr.R:396:3): concat
    > df = pl$DataFrame(x = "a", y = "b")
    > df$select(pl$col("x")$arr$concat(pl$col("y")))
    Error: in Series.new: InvalidOperation(ErrString("new series from rtype ExternalPtr is not supported (yet)"))
  • Error (test-expr_arr.R:454:3): eval
    Seems the same error of Error (test-expr_arr.R:396:3): concat from concat_list
  • Failure (test-expr.R:1512:3): hash + reinterpret
  • Failure (test-expr.R:2176:3): extend_expr
  • Error (test-expr.R:2339:3): concat_list
    Seems the same error of Error (test-expr_arr.R:396:3): concat from concat_list

@eitsupi eitsupi marked this pull request as draft May 2, 2023 10:31
@sorhawell
Copy link
Collaborator

@eitsupi oh more edits, flag me when ready :)

@eitsupi
Copy link
Collaborator Author

eitsupi commented May 2, 2023

I would like to eventually version up to the latest commit at this time. Thoughts?

@sorhawell
Copy link
Collaborator

sorhawell commented May 2, 2023

I'm puzzled about how the "NA" / NA conversion error started. I fixed it in this PR 9d93a08-9e6137f , is not related to bumping rust-polars.
It is now also an issue in join_asof PR. It must be some new crate or rextendr triggering it. The issue is extendr has an known tricky as_str() method which will convert R NA_character into &str "NA", which is not very desirable.

I will delay with updating other PRs until this fix has been merged.

@sorhawell sorhawell self-requested a review May 2, 2023 10:41
@eitsupi
Copy link
Collaborator Author

eitsupi commented May 2, 2023

I'm puzzled about how the "NA" / NA conversion error started. I fixed it in this PR 9e6137f , is not related to bumping rust-polars.

As I commented on #190 (comment), I think this is due to the new release of waldo.

@sorhawell
Copy link
Collaborator

As I commented on #190 (comment), I think this is due to the new release of waldo.

Nice then I have had a bug in these conversion forever, and now waldo sees it.

@sorhawell
Copy link
Collaborator

sorhawell commented May 2, 2023

I would like to eventually version up to the latest commit at this time. Thoughts?

like bumping to polars 0.6.0? I aggree

oh you mean merge in main? I aggree also :)

maybe bumping polars can be delayed to after this merge

@eitsupi eitsupi changed the title [WIP] feat!: update built-in Rust Polars feat!: update built-in Rust Polars May 2, 2023
@eitsupi eitsupi marked this pull request as ready for review May 2, 2023 11:09
@eitsupi
Copy link
Collaborator Author

eitsupi commented May 2, 2023

maybe bumping polars can be delayed to after this merge

Indeed, I was hoping to upgrade to today's version since rust-polars is currently 2 weeks old in this PR, but given that there are bugs that will be fixed by this PR, it seems better to merge now.

@eitsupi eitsupi changed the title feat!: update built-in Rust Polars feat!: update built-in Rust Polars to 2023-04-20 May 2, 2023
@eitsupi
Copy link
Collaborator Author

eitsupi commented May 2, 2023

I think I have completed all the work and can merge if there are no problems with CI.

@eitsupi
Copy link
Collaborator Author

eitsupi commented May 2, 2023

Can I merge this?

@sorhawell
Copy link
Collaborator

Can I merge this?

yeah

@eitsupi eitsupi merged commit d6b2c64 into main May 2, 2023
@eitsupi eitsupi deleted the update-polars branch May 2, 2023 12:54
@eitsupi
Copy link
Collaborator Author

eitsupi commented May 2, 2023

Thanks all!

@vincentarelbundock
Copy link
Collaborator

This is very cool! I think there are some new methods in the latest version. When I find time, I'll try to make a list and see which ones I'm able to implement.

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