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.33 #417

Merged
merged 58 commits into from
Oct 15, 2023
Merged

feat!: bump rust-polars to 0.33 #417

merged 58 commits into from
Oct 15, 2023

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Oct 8, 2023

@eitsupi

This comment was marked as outdated.

@eitsupi eitsupi changed the title feat!: bump rust-polars to 0.33 [skip ci] feat!: bump rust-polars to 0.33 Oct 8, 2023
@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 8, 2023

TODO: add set_random_seed (pola-rs/polars#10388)

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 8, 2023

@sorhawell I think I need to set something like ExprCol when I get an error like this, but I don't know what each one represents. Are they documented somewhere?

   error[E0308]: `?` operator has incompatible types
       --> src/lazy/dsl.rs:2183:24
        |
   2183 |             .ends_with(robj_to!(Raw, sub)?)
        |                        ^^^^^^^^^^^^^^^^^^^ expected `Expr`, found `Vec<u8>`
        |
        = note: `?` operator cannot convert from `Vec<u8>` to `polars::prelude::Expr`
        = note: expected enum `polars::prelude::Expr`
                 found struct `Vec<u8>`

@sorhawell
Copy link
Collaborator

sorhawell commented Oct 8, 2023

I think I need to set something like ExprCol

The r-polars Expr a tupple wrapper around polars::prelude::Expr

A polars method needs the inner native rust-polars Expr. We need to wrap to define our R specifc methods and traits and interface with extendr.

try robj_to(PLExprCol, sub)? which robj_to(ExprCol, sub).0
PL is short for polars (the original rust polars version).

For this simple case the two above are equal.

However it may matter for let Vec<_> = robj_to!(Vec, PLExprCol, exprs)? to avoid a double allocation of a vector.

Almost all conversions are in this single file utils/mod.rs

here is robj_to(PLExpr_col)
https://github.com/pola-rs/r-polars/blob/main/src/rust/src/utils/mod.rs#L859

nearly all macro conversion will call a function called fn robj_to_xyz(robj: Robj) -> RResult<> defined in the same file.

e.g. robj_to_rexpr()

for Expr there 4
PL+ (rust-polars version)/ PL%(r-polars version)
Col+ (str_to_lit = true / Col%(str_to_lit = false)

@eitsupi eitsupi mentioned this pull request Oct 9, 2023
@eitsupi

This comment was marked as resolved.

@eitsupi

This comment was marked as resolved.

@sorhawell

This comment was marked as resolved.

@sorhawell
Copy link
Collaborator

I have access to a computer in 12 hours from now

@eitsupi

This comment was marked as resolved.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 9, 2023

Now all errors are fixed in Rust side. Thanks.

@sorhawell
Copy link
Collaborator

added pl$list_raw() to assist back-forth conversion between R and polars. R raw is a vector of bytes and NAs are not allowed. Polars binary is vector of vectors of bytes, and nulls are allowed.

rpolars_list_raw is an R list of raw(s) or NULLs with class c("rpolars_list_raw", "list")

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 15, 2023

All testthat tests are passed now, thanks @sorhawell!

Is there anything else I need to do before merging?

@eitsupi eitsupi marked this pull request as ready for review October 15, 2023 09:40
@etiennebacher
Copy link
Collaborator

I can't look in detail today but the last commits look good. We probably won't release 0.9.0 right now anyway so I think minor changes can be addressed in other PRs

@sorhawell
Copy link
Collaborator

We might need that "no free braces in docs" pr to pass checks. I can try to finish that in an hour

Alternatively we add something "... braces" to check error filter. We don't need to fix it before latest at next R release

@sorhawell
Copy link
Collaborator

I take the easy way out today and filter "Lost braces" check error on R-devel until #424 is fully done.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 15, 2023

I do not understand what this error means. Why has this suddenly started to occur?

Error: Error in eval(ei, envir) : checking package subdirectories ... NOTE
Problems with news in ‘NEWS.md’:
  Cannot extract version info from the following section titles:
    Breaking changes
    What's changed
    OTHER BREAKING CHANGES
    Other changes
    BREAKING CHANGES
    BREAKING CHANGE
    New Contributors
    What's new
    What's Changed
    Developer changes
    Dev env
    Minor stuff
    New features

@sorhawell
Copy link
Collaborator

It might be a new devel check released today

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 15, 2023

It might be a new devel check released today

I believe the same error occurs in all release R version.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 15, 2023

I do not understand what this error means. Why has this suddenly started to occur?

Error: Error in eval(ei, envir) : checking package subdirectories ... NOTE
Problems with news in ‘NEWS.md’:
  Cannot extract version info from the following section titles:
    Breaking changes
    What's changed
    OTHER BREAKING CHANGES
    Other changes
    BREAKING CHANGES
    BREAKING CHANGE
    New Contributors
    What's new
    What's Changed
    Developer changes
    Dev env
    Minor stuff
    New features

My guess is that the inclusion of the Rust Polars version in the h2 headers confused the Markdown parsing (R bug?).

Hopefully the last commit 479e940 will fix this.

@sorhawell
Copy link
Collaborator

I take the easy way out today and filter "Lost braces" check error on R-devel until #424 is fully done.

Oups I should likely had set the filter as a note/warning not an error. Just as binary size filter.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 15, 2023

In any case, the error is not related to the changes made by this PR, so shouldn't it be possible to ignore it and merge this?

@sorhawell
Copy link
Collaborator

In any case, the error is not related to the changes made by this PR, so shouldn't it be possible to ignore it and merge this?

Is that error still there? I could not see it in the our latest commits. When can filter on any check Note having "Cannot extract version info from the following section titles" if it persists.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 15, 2023

@sorhawell The error "Problems with news in ‘NEWS.md’" was caused by the rust-polars version in the header as I guessed, so it has already been fixed.

Copy link
Collaborator

@sorhawell sorhawell left a comment

Choose a reason for hiding this comment

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

TY for daring a bump @etiennebacher and @eitsupi . Rust part looks very good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not related to this PR, but I feel this is not something that should be stored in the inst folder as it is not used after the package is installed.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 15, 2023

Thanks all!

@eitsupi eitsupi merged commit 6132f51 into main Oct 15, 2023
11 checks passed
@eitsupi eitsupi deleted the rust-0.33.2 branch October 15, 2023 23:21
@etiennebacher
Copy link
Collaborator

Thanks!

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