GH-39138: [R] Fix implicit conversion warnings #39250

paleolimbot · 2023-12-16T18:27:18Z

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 and/or variable declarations were fixed so that fewer implicit integer casts were performed. Fully solving the warnings also requires r-lib/cpp11#349 since some errors occur in those headers.

Are these changes tested?

This particular test we can't do on CI because the MacOS runner we have doesn't have a new enough clang to support the requisite -W flags. I tested this locally by adding PKG_CXXFLAGS=-Wconversion -Wno-sign-conversion -Wsign-compare -Werror to Makevars.in.

Are there any user-facing changes?

No

Closes: [R] Compile with -Wconversion on clang15 results in compiler warnings #39138

github-actions · 2023-12-16T18:27:43Z

⚠️ GitHub issue #39138 has been automatically assigned in GitHub to PR creator.

paleolimbot · 2023-12-18T16:34:32Z

@github-actions crossbow submit test-r-install-local

github-actions · 2023-12-18T16:36:52Z

Revision: 814e712

Submitted crossbow builds: ursacomputing/crossbow @ actions-ba1f272e1d

Task	Status
test-r-install-local

paleolimbot · 2023-12-18T19:03:24Z

@github-actions crossbow submit test-r-install-local

github-actions · 2023-12-18T19:05:37Z

Revision: 97d3713

Submitted crossbow builds: ursacomputing/crossbow @ actions-523036ada2

Task	Status
test-r-install-local

paleolimbot · 2023-12-18T20:29:05Z

@github-actions crossbow submit --group r

github-actions · 2023-12-18T20:32:04Z

Revision: d3b8acc

Submitted crossbow builds: ursacomputing/crossbow @ actions-bfb08e2c71

Task	Status
conda-linux-aarch64-cpu-r42
conda-linux-aarch64-cpu-r43
conda-linux-x64-cpu-r42
conda-linux-x64-cpu-r43
conda-osx-arm64-cpu-r42
conda-osx-arm64-cpu-r43
conda-osx-x64-cpu-r42
conda-osx-x64-cpu-r43
conda-win-x64-cpu-r41
r-binary-packages
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-library-r-base-latest
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-r-sanitizer

assignUser · 2023-12-19T00:14:25Z

@ursabot please benchmark

ursabot · 2023-12-19T00:14:30Z

Benchmark runs are scheduled for commit d3b8acc. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

assignUser · 2023-12-19T04:54:24Z

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/

conbench-apache-arrow · 2023-12-19T06:13:50Z

Thanks for your patience. Conbench analyzed the 6 benchmarking runs that have been run so far on PR commit d3b8acc.

There was 1 benchmark result indicating a performance regression:

Pull Request Run on ursa-thinkcentre-m75q at 2023-12-19 06:04:14Z
- CopyEmptyVector (C++) with params=<SMALL_VECTOR(int)>, source=cpp-micro, suite=arrow-small-vector-benchmark

The full Conbench report has more details.

paleolimbot · 2023-12-19T16:36:52Z

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).

danepitkin · 2023-12-19T19:29:07Z

Overall LGTM! (Caveat: I haven't reviewed much C++ code recently)

danepitkin · 2023-12-19T19:30:21Z

There is a lot of downcasting taking place in Arrow R. Any idea if this was an intentional decision due to some 32bit limitation?

paleolimbot · 2023-12-19T21:03:04Z

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.

assignUser · 2023-12-19T21:10:46Z

I don't see any R-related regressions?

Sorry I forgot which PR this is 🤦‍♂️

felipecrv · 2023-12-19T20:46:59Z

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?


You should consider changing the return type of this function to int64_t instead.

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.

felipecrv · 2023-12-19T20:52:41Z

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); },


Could out be changed to be an int64_t?

felipecrv · 2023-12-19T20:56:48Z

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),


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.

paleolimbot · 2023-12-22T03:03:11Z

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.

### 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 and/or variable declarations were fixed so that fewer implicit integer casts were performed. Fully solving the warnings also requires r-lib/cpp11#349 since some errors occur in those headers. ### Are these changes tested? This particular test we can't do on CI because the MacOS runner we have doesn't have a new enough `clang` to support the requisite `-W` flags. I tested this locally by adding `PKG_CXXFLAGS=-Wconversion -Wno-sign-conversion -Wsign-compare -Werror` to `Makevars.in`. ### Are there any user-facing changes? No * Closes: #39138 Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

conbench-apache-arrow · 2023-12-23T03:54:06Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 7b71156.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them.

### 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 and/or variable declarations were fixed so that fewer implicit integer casts were performed. Fully solving the warnings also requires r-lib/cpp11#349 since some errors occur in those headers. ### Are these changes tested? This particular test we can't do on CI because the MacOS runner we have doesn't have a new enough `clang` to support the requisite `-W` flags. I tested this locally by adding `PKG_CXXFLAGS=-Wconversion -Wno-sign-conversion -Wsign-compare -Werror` to `Makevars.in`. ### Are there any user-facing changes? No * Closes: apache#39138 Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>

paleolimbot added 2 commits December 16, 2023 14:14

first round of updates

c6423e4

a few more

93498de

github-actions bot added Component: R awaiting committer review Awaiting committer review labels Dec 16, 2023

paleolimbot added 6 commits December 18, 2023 10:06

fix a few more

616f0f3

progress

e44b355

even more

6a5372b

fix more warnings

efd719d

formatting

a11dc17

maybe add warnings to a CI job

814e712

paleolimbot added 3 commits December 18, 2023 14:04

fix overload

5a2e710

maybe fix sign-compare

8797d0b

maybe constraint -W flags to MacOS

97d3713

don't bother with CI for now

d3b8acc

paleolimbot mentioned this pull request Dec 18, 2023

Fix errors resulting from -Wconversion -Wno-sign-conversion r-lib/cpp11#349

Closed

paleolimbot marked this pull request as ready for review December 18, 2023 20:25

paleolimbot requested a review from thisisnic as a code owner December 18, 2023 20:25

felipecrv reviewed Dec 19, 2023

View reviewed changes

some dchecks

94fecde

assignUser mentioned this pull request Dec 22, 2023

[R][Docs] pkgdown site docs report version 14.0.1 #38689

Closed

paleolimbot merged commit 7b71156 into apache:main Dec 22, 2023

paleolimbot removed the awaiting committer review Awaiting committer review label Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39138: [R] Fix implicit conversion warnings #39250

GH-39138: [R] Fix implicit conversion warnings #39250

paleolimbot commented Dec 16, 2023 •

edited

Loading

github-actions bot commented Dec 16, 2023

paleolimbot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

paleolimbot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

paleolimbot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

assignUser commented Dec 19, 2023

ursabot commented Dec 19, 2023

assignUser commented Dec 19, 2023

conbench-apache-arrow bot commented Dec 19, 2023

paleolimbot commented Dec 19, 2023

danepitkin commented Dec 19, 2023

danepitkin commented Dec 19, 2023

paleolimbot commented Dec 19, 2023

assignUser commented Dec 19, 2023

felipecrv Dec 19, 2023

felipecrv Dec 19, 2023

felipecrv Dec 19, 2023

felipecrv Dec 19, 2023

paleolimbot commented Dec 22, 2023

conbench-apache-arrow bot commented Dec 23, 2023

GH-39138: [R] Fix implicit conversion warnings #39250

GH-39138: [R] Fix implicit conversion warnings #39250

Conversation

paleolimbot commented Dec 16, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Dec 16, 2023

paleolimbot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

paleolimbot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

paleolimbot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

assignUser commented Dec 19, 2023

ursabot commented Dec 19, 2023

assignUser commented Dec 19, 2023

conbench-apache-arrow bot commented Dec 19, 2023

paleolimbot commented Dec 19, 2023

danepitkin commented Dec 19, 2023

danepitkin commented Dec 19, 2023

paleolimbot commented Dec 19, 2023

assignUser commented Dec 19, 2023

felipecrv Dec 19, 2023

Choose a reason for hiding this comment

felipecrv Dec 19, 2023

Choose a reason for hiding this comment

felipecrv Dec 19, 2023

Choose a reason for hiding this comment

felipecrv Dec 19, 2023

Choose a reason for hiding this comment

paleolimbot commented Dec 22, 2023

conbench-apache-arrow bot commented Dec 23, 2023

paleolimbot commented Dec 16, 2023 •

edited

Loading