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

Better handling of int64 #706

Merged
merged 24 commits into from
Jan 20, 2024
Merged

Better handling of int64 #706

merged 24 commits into from
Jan 20, 2024

Conversation

etiennebacher
Copy link
Collaborator

Close #465

@etiennebacher etiennebacher marked this pull request as ready for review January 18, 2024 17:09
@etiennebacher
Copy link
Collaborator Author

etiennebacher commented Jan 18, 2024

@eitsupi @sorhawell I'm not sure this is the right way to address the int64 issue but it works, what do you think?

Note that I took bigint_conversion mostly as a temporary name, no problem to change it (I also picked it because it will be easy to search and replace).

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thank you for working on this.
But I think we should not set options here with pl$set_options(), but use R's standard options() function.
The options function is widely used, and its usage such as temporary switching with something like withr is popular.

R/options.R Outdated Show resolved Hide resolved
@etiennebacher
Copy link
Collaborator Author

But I think we should not set options here with pl$set_options(), but use R's standard options() function.

I'd like to continue this discussion but this is out of scope for this PR. Can you open an issue for this?

arrow uses arrow.int64_downcast

This name is associated with TRUE/FALSE (do we downcast int64? yes or no). Here we have more choices so I don't think this name is suitable.

@eitsupi
Copy link
Collaborator

eitsupi commented Jan 19, 2024

I'd like to continue this discussion but this is out of scope for this PR. Can you open an issue for this?

Done #713

This name is associated with TRUE/FALSE (do we downcast int64? yes or no). Here we have more choices so I don't think this name is suitable.

Of course I am not suggesting that it should be named as such, just an example of the use of the options function.
bigint_conversion is not bad for me, but int64_conversion might be better as I think Polars terminology is int64 not bigint.

@etiennebacher
Copy link
Collaborator Author

bigint_conversion is not bad for me, but int64_conversion might be better as I think Polars terminology is int64 not bigint.

Fine for me, I changed both the value "float" and the arg name

@etiennebacher
Copy link
Collaborator Author

I would like to have @sorhawell's opinion before merging

R/expr__expr.R Outdated Show resolved Hide resolved
src/rust/src/conversion_s_to_r.rs Outdated Show resolved Hide resolved
Comment on lines +176 to +177
"acceptable_choices" = "`int64_conversion ` must be one of \"float\", \"string\", \"bit64\".",
"bit64_is_attached" = "Package `bit64` must be attached to use `int64_conversion = \"bit64\"`."
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about use raw strings here?

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 don't think it would add much

@etiennebacher etiennebacher merged commit 2529ae7 into main Jan 20, 2024
31 checks passed
@etiennebacher etiennebacher deleted the handle-int64 branch January 20, 2024 10:37
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.

Improve handling of Polars Int64 to R
2 participants