Skip to content

Conversation

@thisisnic
Copy link
Member

No description provided.

@github-actions
Copy link

Comment on lines 358 to 362
array_data <- Array$create(data)

expect_equal(as.vector(any(array_data > 5)), any(data > 5))
expect_equal(as.vector(any(array_data < 1)), any(data < 1))
expect_equal(as.vector(any(array_data < 1, na.rm = TRUE)), any(data < 1, na.rm = TRUE))
Copy link
Member

@ianmcook ianmcook Apr 19, 2021

Choose a reason for hiding this comment

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

These tests could be simplified by using the helper function expect_vector_equal() (which is defined in helper-expectation.R). E.g.:

Suggested change
array_data <- Array$create(data)
expect_equal(as.vector(any(array_data > 5)), any(data > 5))
expect_equal(as.vector(any(array_data < 1)), any(data < 1))
expect_equal(as.vector(any(array_data < 1, na.rm = TRUE)), any(data < 1, na.rm = TRUE))
expect_vector_equal(any(input > 5), data)
expect_vector_equal(any(input < 1), data)
expect_vector_equal(any(input < 1, na.rm = TRUE), data)

Copy link
Member

@ianmcook ianmcook Apr 19, 2021

Choose a reason for hiding this comment

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

Note that expect_vector_equal() tests both Array and ChunkedArray so you'll be able to reduce the number of tests used here

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated!

@ianmcook
Copy link
Member

@thisisnic it looks like when you ran devtools::document(), it built some docs changes unrelated to this PR. This happens sometimes (e.g. when someone else commits a PR without running devtools::document()) and it's no big deal, but it'd be a good idea to push a commit to undo those unrelated docs changes, to avoid any merge conflicts.

@thisisnic thisisnic force-pushed the ARROW-12185-any_all branch from 6f81a57 to defeac1 Compare April 20, 2021 08:29
@thisisnic thisisnic force-pushed the ARROW-12185-any_all branch from defeac1 to 12cb6cb Compare April 20, 2021 08:34
@thisisnic
Copy link
Member Author

@thisisnic it looks like when you ran devtools::document(), it built some docs changes unrelated to this PR. This happens sometimes (e.g. when someone else commits a PR without running devtools::document()) and it's no big deal, but it'd be a good idea to push a commit to undo those unrelated docs changes, to avoid any merge conflicts.

Fixed now!

Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

Looks good! I'm going to add some inline comments to explain the three-valued logic (as well as tag @rok about doing it correctly in C++), then merge.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants