jonkeane commented on pull request #12357:
URL: https://github.com/apache/arrow/pull/12357#issuecomment-1046033487
This is very very very close! I rebased off of the default branch since
#12447 merged to get that fix.
I also updated the tests to mostly be the more simple R object in the
`call_binding` but kept one that uses `Expression$create(...)` since
`Expressions` are the type that will show up in the dplyr query.
I did notice when testing locally (and I assume this'll pop up in the CI
too), I think there might be a slight problem with #12447. It looks like
`type({Expression object})` is returning the function for getting the type and
not the Type object itself (hence everyone's favorite error message: `object of
type closure is not subsetable`)
I think that
https://github.com/apache/arrow/blob/f9f2c0827e1e605717d954c6899fe07b08f599cb/r/R/type.R#L81
should actually be `type.Expression <- function(x) x$type()`
It might be nice to add a test that asserts not just that `type(x)` and
`x$type` are the same, but that `type(x)` produces the right Type object. So
like this line:
https://github.com/apache/arrow/blob/f9f2c0827e1e605717d954c6899fe07b08f599cb/r/tests/testthat/test-type.R#L238
could also be tested with something like `expect_equal(type(x), int32())`
which will make sure that what's coming out of `type(x)` there is definitely a
type.
It's a bit muddying this PR, but I'm happy for you to make those small
adjustments here rather than a whole new Jira + commit chain + another wait to
rebase after that's fixed.
Other than that, this PR looks ready to go!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
