-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-3604: [R] Support to collect int64s as R integers #2834
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
Conversation
wesm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one probably needs some tests
| // the integer64 sentinel | ||
| static const int64_t NA_INT64 = std::numeric_limits<int64_t>::min(); | ||
| static const int64_t MAX_INT32 = std::numeric_limits<int32_t>::max(); | ||
| static const int64_t MIN_INT32 = std::numeric_limits<int32_t>::min(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change these to constexpr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Does that mean they have to be functions ?
Otherwise, since ::minuend ::maxare themselvesconstexpr` perhaps we don't even need the constants here.
Also, not sure what MIN_INT32 is used for, but we need to be careful because it's also the NA sentinel for int in R.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes they are constexpr, e.g.
arrow/cpp/src/arrow/util/hash.h
Line 32 in b165c86
| static constexpr hash_slot_t kHashSlotEmpty = std::numeric_limits<int32_t>::max(); |
|
This, and #2819 should be discussed more broadly. I'm not a fan of relying on a global I'd rather have those lossy conversions to be explicit. |
|
Yeah I would suggest passing an options list to the conversion entry point |
|
From the |
Fix for, https://issues.apache.org/jira/browse/ARROW-3604
Enabled collecting
int64s as Rintegersby replacing overflows and underflows withNAs and triggering a waring.In
sparklyr, this enables:CC: @romainfrancois