felipecrv commented on code in PR #39250:
URL: https://github.com/apache/arrow/pull/39250#discussion_r1431927777
##########
r/src/altrep.cpp:
##########
@@ -718,7 +723,8 @@ struct AltrepFactor : public AltrepVectorBase<AltrepFactor>
{
VisitArraySpanInline<Type>(
*array->data(),
- /*valid_func=*/[&](index_type index) { *out++ = transpose(index) + 1;
},
+ /*valid_func=*/
+ [&](index_type index) { *out++ = static_cast<int>(transpose(index) +
1); },
Review Comment:
Could `out` be changed to be an `int64_t`?
##########
r/src/altrep.cpp:
##########
@@ -802,7 +808,8 @@ struct AltrepVectorString : public
AltrepVectorBase<AltrepVectorString<Type>> {
}
nul_was_stripped_ = true;
- return Rf_mkCharLenCE(stripped_string_.data(), stripped_len, CE_UTF8);
+ return Rf_mkCharLenCE(stripped_string_.data(),
static_cast<int>(stripped_len),
Review Comment:
Here, a `DCHECK_LE(stripped_len, std::numeric_limits<int>::max())` would be
useful since there is no way to change what `Rf_mkCharLenCE` receives.
##########
r/src/altrep.cpp:
##########
@@ -613,11 +615,14 @@ struct AltrepFactor : public
AltrepVectorBase<AltrepFactor> {
case Type::INT32:
return indices->data()->GetValues<int32_t>(1)[j] + 1;
case Type::UINT32:
- return indices->data()->GetValues<uint32_t>(1)[j] + 1;
+ // TODO: check index?
Review Comment:
And you can postpone all the refactoring by renaming this function and
adding a wrapper with this signature with the sole purpose of converting the
`int64_t` returned by this modified function to `int`. Then you have to write
the casts and check in a single place.
##########
r/src/altrep.cpp:
##########
@@ -613,11 +615,14 @@ struct AltrepFactor : public
AltrepVectorBase<AltrepFactor> {
case Type::INT32:
return indices->data()->GetValues<int32_t>(1)[j] + 1;
case Type::UINT32:
- return indices->data()->GetValues<uint32_t>(1)[j] + 1;
+ // TODO: check index?
Review Comment:
You should consider changing the return type of this function to `int64_t`
instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
