ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion#8341
ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion#8341romainfrancois wants to merge 5 commits intoapache:masterfrom
Conversation
|
Should that be a more general option perhaps ? Is this the right name ? |
nealrichardson
left a comment
nealrichardson
left a comment
There was a problem hiding this comment.
Some suggestions. Looking at https://arrow.apache.org/docs/r/articles/arrow.html#arrow-to-r, I don't see that there's a more general option to have right now. We may want another option around dictionary conversion (https://issues.apache.org/jira/browse/ARROW-7657), but it's not obvious to me that these options would necessarily go together. If we later find that we want to add an umbrella option, we can add it.
r/tests/testthat/test-Array.R
Outdated
There was a problem hiding this comment.
Re: naming, we currently have options "arrow.use_threads" (mostly private but some may need to set it given the multithreading bugs we've observed) and "arrow.dev.repo" (not advertised). I don't feel strongly about the naming convention as long as we're consistent and predictable (which, naturally, names(options()) is not).
Searching around for other conventions, I found package.option_name (cf. tidyverse/dplyr#5548) like we did with arrow.use_threads, so maybe let's standardize on that?
There was a problem hiding this comment.
using arrow.int64_downcast, happy to change
nealrichardson
left a comment
nealrichardson
left a comment
There was a problem hiding this comment.
This looks good, just a couple of quick final notes
r/src/array_to_vector.cpp
Outdated
There was a problem hiding this comment.
Your other comment said you were going with arrow.int64_downcast but that's not what is here. I'm ok either way though arrow.int64_downcast is more concise (and the "auto" behavior is really about what the default value is).
r/src/array_to_vector.cpp
Outdated
There was a problem hiding this comment.
Minor thought: should we switch the order of these checks? I have to imagine that GetBoolOption would be faster than doing bounds checking on the full array.
Co-authored-by: Neal Richardson <[email protected]>
nealrichardson
force-pushed
the
ARROW-10093/disable_int64_autoconv
branch
from
2b1269c to
c1b40ec
Compare
2 participants
No description provided.