You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/12/16 18:27:21 UTC
[PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
paleolimbot opened a new pull request, #39250:
URL: https://github.com/apache/arrow/pull/39250
### Rationale for this change
We have failing CRAN checks because this warning occurs on one check machine.
### What changes are included in this PR?
Implicit integer casts are made explicit
### Are these changes tested?
Not yet (working on where to add these flags)
### Are there any user-facing changes?
No
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1868194195
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 7b71156d99557168d46292c010f82b812947ffb8.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/19910275299) has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1861362081
Revision: 97d3713f6b05c0e82d06e9af7e3faa27df83cdbe
Submitted crossbow builds: [ursacomputing/crossbow @ actions-523036ada2](https://github.com/ursacomputing/crossbow/branches/all?query=actions-523036ada2)
|Task|Status|
|----|------|
|test-r-install-local|[](https://github.com/ursacomputing/crossbow/actions/runs/7252631731/job/19757512297)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1860990120
Revision: 814e712917a05c19e5f28691133d65fff37c6147
Submitted crossbow builds: [ursacomputing/crossbow @ actions-ba1f272e1d](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ba1f272e1d)
|Task|Status|
|----|------|
|test-r-install-local|[](https://github.com/ursacomputing/crossbow/actions/runs/7251029446/job/19752512443)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1862184853
Thanks for your patience. Conbench analyzed the 6 benchmarking runs that have been run so far on PR commit d3b8accedae9f0866656f027c29d4a5c98b63cca.
There was 1 benchmark result indicating a performance regression:
- Pull Request Run on `ursa-thinkcentre-m75q` at [2023-12-19 06:04:14Z](https://conbench.ursa.dev/compare/runs/46cd0c8be1024eeba29298d0bd03df3b...be2296baa276444fbd4ddc8545598b8b/)
- [`CopyEmptyVector` (C++) with params=<SMALL_VECTOR(int)>, source=cpp-micro, suite=arrow-small-vector-benchmark](https://conbench.ursa.dev/compare/benchmarks/0657de4863da723180003a3b92fe1e71...065812046bbe716a80006fb1b5549930)
The [full Conbench report](https://github.com/apache/arrow/runs/19772895606) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1867175401
Thank you @felipecrv and @danepitkin for the feedback (and sorry to everybody for taking a few days to circle back here...I wanted to make sure I addressed the comments properly!).
I added a `ARROW_R_DCHECK()` guarded by a special `ARROW_R_DEBUG`...we don't have any precedent in the package so far for using the existing DCHECK macros and I'm a little worried it will add `cerr` or `abort` symbols. I'm probably the only person that will have it enabled but at least it gives us a place to put some of these checks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1861566756
Revision: d3b8accedae9f0866656f027c29d4a5c98b63cca
Submitted crossbow builds: [ursacomputing/crossbow @ actions-bfb08e2c71](https://github.com/ursacomputing/crossbow/branches/all?query=actions-bfb08e2c71)
|Task|Status|
|----|------|
|conda-linux-aarch64-cpu-r42|[](https://github.com/ursacomputing/crossbow/runs/19760155425)|
|conda-linux-aarch64-cpu-r43|[](https://github.com/ursacomputing/crossbow/runs/19760157600)|
|conda-linux-x64-cpu-r42|[](https://github.com/ursacomputing/crossbow/runs/19760156740)|
|conda-linux-x64-cpu-r43|[](https://github.com/ursacomputing/crossbow/runs/19760156668)|
|conda-osx-arm64-cpu-r42|[](https://github.com/ursacomputing/crossbow/runs/19760157613)|
|conda-osx-arm64-cpu-r43|[](https://github.com/ursacomputing/crossbow/runs/19760156659)|
|conda-osx-x64-cpu-r42|[](https://github.com/ursacomputing/crossbow/runs/19760157987)|
|conda-osx-x64-cpu-r43|[](https://github.com/ursacomputing/crossbow/runs/19760157102)|
|conda-win-x64-cpu-r41|[](https://github.com/ursacomputing/crossbow/runs/19760157619)|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476449/job/19760171132)|
|test-fedora-r-clang-sanitizer|[](https://github.com/ursacomputing/crossbow/runs/19760154914)|
|test-r-arrow-backwards-compatibility|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476436/job/19760152824)|
|test-r-depsource-bundled|[](https://github.com/ursacomputing/crossbow/runs/19760155440)|
|test-r-depsource-system|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476614/job/19760154150)|
|test-r-dev-duckdb|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476440/job/19760152831)|
|test-r-devdocs|[](https://github.com/ursacomputing/crossbow/actions/runs/7253477048/job/19760155216)|
|test-r-gcc-11|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476780/job/19760154461)|
|test-r-gcc-12|[](https://github.com/ursacomputing/crossbow/actions/runs/7253477496/job/19760156260)|
|test-r-install-local|[](https://github.com/ursacomputing/crossbow/actions/runs/7253477081/job/19760155851)|
|test-r-install-local-minsizerel|[](https://github.com/ursacomputing/crossbow/actions/runs/7253477028/job/19760155226)|
|test-r-library-r-base-latest|[](https://github.com/ursacomputing/crossbow/runs/19760155506)|
|test-r-linux-as-cran|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476334/job/19760154464)|
|test-r-linux-rchk|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476618/job/19760154049)|
|test-r-linux-valgrind|[](https://github.com/ursacomputing/crossbow/runs/19760157646)|
|test-r-minimal-build|[](https://github.com/ursacomputing/crossbow/runs/19760156168)|
|test-r-offline-maximal|[](https://github.com/ursacomputing/crossbow/actions/runs/7253477498/job/19760156631)|
|test-r-offline-minimal|[](https://github.com/ursacomputing/crossbow/runs/19760154621)|
|test-r-rhub-debian-gcc-devel-lto-latest|[](https://github.com/ursacomputing/crossbow/runs/19760157129)|
|test-r-rhub-debian-gcc-release-custom-ccache|[](https://github.com/ursacomputing/crossbow/runs/19760156826)|
|test-r-rhub-ubuntu-gcc-release-latest|[](https://github.com/ursacomputing/crossbow/runs/19760157584)|
|test-r-rstudio-r-base-4.1-opensuse153|[](https://github.com/ursacomputing/crossbow/runs/19760155426)|
|test-r-rstudio-r-base-4.2-centos7-devtoolset-8|[](https://github.com/ursacomputing/crossbow/runs/19760154617)|
|test-r-rstudio-r-base-4.2-focal|[](https://github.com/ursacomputing/crossbow/runs/19760156742)|
|test-r-ubuntu-22.04|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476409/job/19760152747)|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions/runs/7253476593/job/19760155192)|
|test-ubuntu-r-sanitizer|[](https://github.com/ursacomputing/crossbow/runs/19760155488)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1861559828
@github-actions crossbow submit --group r
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1862116232
The benchmark that is through seems to show some regressions? But I also don't have practice in interpreting these: https://conbench.ursa.dev/compare/runs/e13bb0a5533349c094f88b39316be8e3...0d58238a60974b3facaf13de17fad7f7/
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1858900430
:warning: GitHub issue #39138 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1863106570
I don't see any R-related regressions? (Even if there were, it would have been because a check was added that increases safety, although I don't think I added any additional out-of-bounds checks in places with tight loops).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1861356863
@github-actions crossbow submit test-r-install-local
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1861896234
@ursabot please benchmark
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #39250:
URL: https://github.com/apache/arrow/pull/39250
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1860984524
@github-actions crossbow submit test-r-install-local
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1863476897
> I don't see any R-related regressions?
Sorry I forgot which PR this is 🤦♂️
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1863356221
Overall LGTM! (Caveat: I haven't reviewed much C++ code recently)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1861896319
Benchmark runs are scheduled for commit d3b8accedae9f0866656f027c29d4a5c98b63cca. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1863358111
There is a lot of downcasting taking place in Arrow R. Any idea if this was an intentional decision due to some 32bit limitation?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39138: [R] Fix implicit conversion warnings [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #39250:
URL: https://github.com/apache/arrow/pull/39250#issuecomment-1863467919
> There is a lot of downcasting taking place in Arrow R
By default, I think you would be hard pressed to find loss of precision happening. The main place that this could happen without explicit user intervention is (1) when converting timestamps to R with subsecond precision (limitation of R, we store timestamps as seconds with double precision) and (2) factors with more then INT_MAX elements in a dictionary (limitation of R, factors are int under the hood). Other downcasting that happens requires intervention from the user (i.e., `Array$create(1.2345, int32())` will truncate `1.234`).
I think we can do better in all cases, but we still wouldn't do that checking or erroring at the element level (i.e., where the `static_cast<>()` happens)...we'd want to do something like `if (AnyConversionMayBeLossy(<big long thing>)) return Status::Invalid(...)` before we get to that point.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org