Skip to content

GH-50014: [R] Replace imported symbol from bit64#50015

Open
MichaelChirico wants to merge 2 commits into
apache:mainfrom
MichaelChirico:unexport-s3
Open

GH-50014: [R] Replace imported symbol from bit64#50015
MichaelChirico wants to merge 2 commits into
apache:mainfrom
MichaelChirico:unexport-s3

Conversation

@MichaelChirico
Copy link
Copy Markdown
Contributor

@MichaelChirico MichaelChirico commented May 21, 2026

I opted to delete the 'reexports' file because the symbols are not actually re-exported. IMHO, it's more appropriate to just add this hook in arrow-package.R as done here, but of course if your strong preference is to retain the reexports-bit64.R file I'll restore it.

integer64() should be a fine choice -- I basically wanted a part of the API that has essentially 0 chance of ever changing.

As of today, R CMD check doesn't complain if you import a symbol and then don't reference it in the sources. Nor do I think such a check will ever happen.

@github-actions
Copy link
Copy Markdown

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

@github-actions
Copy link
Copy Markdown

⚠️ GitHub issue #50014 has no components, please add labels for components.

@MichaelChirico
Copy link
Copy Markdown
Contributor Author

The GitHub codespace where I edited this doesn't have enough resources to compile {arrow} reliably :\

So I can't re-run the test of attempting to detach {bit64} with an existing integer64 column.

@MichaelChirico
Copy link
Copy Markdown
Contributor Author

MichaelChirico commented May 21, 2026

OK, I managed to install with cmake finally. I adapted the original bug report given the update in arrow API, please double-check it's done faithfully:

# NB: IIUC, as_vector() and/or arrow_array() will
#   not retain 64-bit representation of the original 1:10,
#   so force that with a non-32-bit input
x = lim.integer64()
# was array()
a <- arrow_array(x)
a$as_vector()
# integer64
# [1] -9223372036854775807 9223372036854775807 
detach("package:bit64", unload = TRUE)
# Warning message:
# ‘bit64’ namespace cannot be unloaded:
#   namespace ‘bit64’ is imported by ‘arrow’ so cannot be unloaded 

Is it recommended to put this in a test? The most natural way to do so would be a new file in r/tests reproducing the above -- I am a bit loath to put a detach() in the testthat suite. WDYT?

@jonkeane
Copy link
Copy Markdown
Member

@github-actions crossbow submit -g r

@github-actions
Copy link
Copy Markdown

Revision: 16c7459

Submitted crossbow builds: ursacomputing/crossbow @ actions-ccf035b329

Task Status
r-binary-packages GitHub Actions
r-recheck-most GitHub Actions
test-r-alpine-linux-cran GitHub Actions
test-r-arrow-backwards-compatibility GitHub Actions
test-r-depsource-system GitHub Actions
test-r-dev-duckdb GitHub Actions
test-r-devdocs GitHub Actions
test-r-extra-packages GitHub Actions
test-r-fedora-clang GitHub Actions
test-r-gcc-11 GitHub Actions
test-r-gcc-12 GitHub Actions
test-r-install-local GitHub Actions
test-r-install-local-minsizerel GitHub Actions
test-r-linux-as-cran GitHub Actions
test-r-linux-rchk GitHub Actions
test-r-linux-sanitizers GitHub Actions
test-r-linux-valgrind GitHub Actions
test-r-m1-san GitHub Actions
test-r-macos-as-cran GitHub Actions
test-r-offline-maximal GitHub Actions
test-r-ubuntu-22.04 GitHub Actions
test-r-versions GitHub Actions
test-r-wasm GitHub Actions

Copy link
Copy Markdown
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I'm ok without having an explicit test for this, since really it's more testing R's blocking of any imported package being detached (and that we have imported at least one thing IIUC).

I've triggered our full nightly run just in case there's something in those that bonk, but this is good so long as that's fine.

Comment thread r/R/reexports-bit64.R
@@ -1,22 +0,0 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with removing the reexports file here.

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants