-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
ARROW-17620: [R] as_arrow_array() ignores type for StructArrays #14047
Conversation
|
With this fix, 2 tests were failing. Previously, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! One small suggestion, otherwise looking good.
r/tests/testthat/test-Array.R
Outdated
## as_arrow_array respects `type` argument (ARROW-17620) | ||
expect_type_equal(a, as_arrow_array(df, type = type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have a few other tests for the functionality of as_arrow_array()
, and this is testing one of the function's arguments, I think it's significant enough to live in its own test_that()
block. Would you mind making that change, and moving it down (I think those tests start near line 1021 of this file)?
Thanks @thisisnic! I moved the test to its own block. Let me know if you need anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great; thanks!
Benchmark runs are scheduled for baseline = 8c76855 and contender = 3ddf69e. 3ddf69e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…he#14047) As described in https://issues.apache.org/jira/browse/ARROW-17620, `as_arrow_array()` ignores user-provided types passed by the `type` argument. This PR explicitly adds the `type` name argument to the `Map` call. Currently, the argument gets passed to the ellipsis and gets ignored. I also added a test. Let me know if there is a better place for it (or a better test). Lead-authored-by: François Michonneau <[email protected]> Co-authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
…he#14047) As described in https://issues.apache.org/jira/browse/ARROW-17620, `as_arrow_array()` ignores user-provided types passed by the `type` argument. This PR explicitly adds the `type` name argument to the `Map` call. Currently, the argument gets passed to the ellipsis and gets ignored. I also added a test. Let me know if there is a better place for it (or a better test). Lead-authored-by: François Michonneau <[email protected]> Co-authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
As described in https://issues.apache.org/jira/browse/ARROW-17620,
as_arrow_array()
ignores user-provided types passed by thetype
argument.This PR explicitly adds the
type
name argument to theMap
call. Currently, the argument gets passed to the ellipsis and gets ignored.I also added a test. Let me know if there is a better place for it (or a better test).