Skip to content

GH-44208: [R] Adding test to ensure bit64's new semantic works with arrow #46651

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-44208: [R] Adding test to ensure bit64's new semantic works with arrow #46651

wants to merge 1 commit into from

Conversation

NazilaAk
Copy link

@NazilaAk NazilaAk commented May 30, 2025 •
edited by github-actions bot
Loading

Rationale for this change

Based on the #44208 issue, we want to make sure that we have test for new semantic of bit64

What changes are included in this PR?

New test

Are these changes tested?

NA

Are there any user-facing changes?

No

@github-actions GitHub Actions
Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@NazilaAk NazilaAk changed the title [R] Ensure that bit64's "new semantics" work with arrow GH-44208: [R] Adding test to ensure bit64's new semantic works with arrow May 30, 2025
@github-actions GitHub Actions
Copy link

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

@github-actions GitHub Actions
Copy link

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

@NazilaAk NazilaAk marked this pull request as ready for review May 30, 2025 22:02
jonkeane
jonkeane requested changes Jun 2, 2025
View reviewed changes
Copy link
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! These suggestions should resolve the linting issues in CI.

Do you know if any of the semantics changes mean we should add other tests? Or does this cover the new behavior(s) in bit64?

r/tests/testthat/test-Array.R
@@ -352,6 +352,20 @@ test_that("array supports integer64", {
expect_true(as.vector(is.na(all_na)))
})

test_that("array supports integer64 with new sematics", {
options(integer64_semantics="new")
x <- bit64::as.integer64(1:10) + MAX_INT
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
x <- bit64::as.integer64(1:10) + MAX_INT
x <- bit64::as.integer64(1:10) + MAX_INT

r/tests/testthat/test-Array.R
@@ -352,6 +352,20 @@ test_that("array supports integer64", {
expect_true(as.vector(is.na(all_na)))
})

test_that("array supports integer64 with new sematics", {
options(integer64_semantics="new")
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind wrapping this in withr::with_options(list(integer64_semantics = "new"), {...}

see

arrow/r/tests/testthat/test-Array.R

Line 842 in c2fb0e3

withr::with_options(list(arrow.skip_nul = TRUE), {
for an example. This will ensure the option isn't kept on after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonkeane jonkeane jonkeane requested changes

@thisisnic thisisnic Awaiting requested review from thisisnic thisisnic is a code owner

Assignees
No one assigned
Labels
awaiting review Awaiting review Component: R
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NazilaAk @jonkeane