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

Implement $sample() for DataFrame #399

Merged
merged 4 commits into from
Sep 26, 2023
Merged

Implement $sample() for DataFrame #399

merged 4 commits into from
Sep 26, 2023

Conversation

etiennebacher
Copy link
Collaborator

No description provided.

@sorhawell
Copy link
Collaborator

sorhawell commented Sep 26, 2023

Thanks @etiennebacher !

This PR as is I think was fine. However I have some minor details.

In the past rust-polars errors where often just converted into a String, but we try to avoid that in any new or refactored code. For complicated trait-rule-reasons we cannot currently just use the ?-operator to convert rust-polars PolarsError to our r-polars RPolarsError (but likely in future version of rust we can). We have to use the polars_to_rpolars_err() function for now, but it will ensure our canonical conversion is used.

avoid stop() outside a result() as it will raise an unstyled error with no context, which can be difficult for user to interpret if coming from somewhere unknown in a large polars syntax query.

result catches any error and upgrades it, so inside a result clause stop() is fine. Otherwise for Err_plain() is effectively the same outcome with fewer internal steps.

> result(stop("oups")) |> unwrap("in some example_function():")
Error: Execution halted with the following contexts
   0: In R: in some example_function():
   0: During function call [unwrap(result(stop("oups")), "in some example_function():")]
   1: oups
   
> Err_plain("oups") |> unwrap("example_function():")
Error: Execution halted with the following contexts
   0: In R: example_function():
   0: During function call [unwrap(Err_plain("oups"), "example_function():")]
   1: oups

all the pcase stuff is not mandatory

@eitsupi eitsupi merged commit 33a56d9 into main Sep 26, 2023
11 checks passed
@eitsupi eitsupi deleted the sample-dataframe branch September 26, 2023 22:28
@eitsupi
Copy link
Collaborator

eitsupi commented Sep 26, 2023

Thanks!

@etiennebacher
Copy link
Collaborator Author

Thanks for finishing this PR @sorhawell

In the past rust-polars errors where often just converted into a String, but we try to avoid that in any new or refactored code. For complicated trait-rule-reasons we cannot currently just use the ?-operator to convert rust-polars PolarsError to our r-polars RPolarsError (but likely in future version of rust we can). We have to use the polars_to_rpolars_err() function for now, but it will ensure our canonical conversion is used.

Yes I feel like you explained this to me before but I didn't remember. We should probably refactor functions on the Rust side to use this (I can try to do this for a couple of them and then ask you to check)

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