You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "orgadish (via GitHub)" <gi...@apache.org> on 2023/10/09 02:21:22 UTC
[PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
orgadish opened a new pull request, #38144:
URL: https://github.com/apache/arrow/pull/38144
### Rationale for this change
#38033
### What changes are included in this PR?
- Added `add_pkg_name` helper.
- Added `code_name` private method to `DataType` class to store the string name used in the code call. Refactored `code` public method to use `code_name`.
- Added `explicit_pkg_name` and `add_pkg_name` to implementation of `DataType$code(explicit_pkg_name=FALSE)`.
- Added `explicit_pkg_name` and `add_pkg_name` to `Schema$code(explicit_pkg_name=FALSE)`.
- Added `code` to Scheme docstring.
### Are these changes tested?
I have not been able to test this yet, as I get the following error (even after `brew install apache-arrow --HEAD`).
```
Error in dyn.load(dll_copy_file) :
unable to load shared object '/var/folders/gh/kkjkl7md3ml943xf33hqg6540000gp/T//RtmpFfA62h/pkgload127a31261e174/arrow.so':
dlopen(/var/folders/gh/kkjkl7md3ml943xf33hqg6540000gp/T//RtmpFfA62h/pkgload127a31261e174/arrow.so, 0x0006): symbol not found in flat namespace '__ZN5arrow12ArrayBuilder12AppendScalarERKNS_6ScalarEx'
```
### Are there any user-facing changes?
Yes, user-facing changes, but no breaking changes to any public APIs.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "orgadish (via GitHub)" <gi...@apache.org>.
orgadish commented on code in PR #38144:
URL: https://github.com/apache/arrow/pull/38144#discussion_r1357748460
##########
r/R/type.R:
##########
@@ -665,13 +694,16 @@ list_of <- function(type) list__(type)
LargeListType <- R6Class("LargeListType",
inherit = NestedType,
public = list(
- code = function() {
- call2("large_list_of", self$value_type$code())
+ code = function(explicit_pkg_name = FALSE) {
+ call2(private$call_name(), self$value_type$code(explicit_pkg_name), .ns = get_pkg_ns(explicit_pkg_name))
}
),
active = list(
value_field = function() LargeListType__value_field(self),
value_type = function() LargeListType__value_type(self)
+ ),
+ private = list(
+ call_name = function() "large_list_of"
Review Comment:
Separating out the `call_name` came from a previous implementation where I was doing much more complicated things, before I discovered the `.ns` argument in `call2`. I agree with you that we should revert to the previous setup.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "orgadish (via GitHub)" <gi...@apache.org>.
orgadish commented on PR #38144:
URL: https://github.com/apache/arrow/pull/38144#issuecomment-1760907037
Ran all tests on `c0a80a9`
```
[ FAIL 0 | WARN 8 | SKIP 32 | PASS 8149 ]
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "orgadish (via GitHub)" <gi...@apache.org>.
orgadish commented on code in PR #38144:
URL: https://github.com/apache/arrow/pull/38144#discussion_r1357748075
##########
r/R/type.R:
##########
@@ -665,13 +694,16 @@ list_of <- function(type) list__(type)
LargeListType <- R6Class("LargeListType",
inherit = NestedType,
public = list(
- code = function() {
- call2("large_list_of", self$value_type$code())
+ code = function(explicit_pkg_name = FALSE) {
+ call2(private$call_name(), self$value_type$code(explicit_pkg_name), .ns = get_pkg_ns(explicit_pkg_name))
Review Comment:
1. Great call on using `if(namespace) "arrow"` (or `getPackageName()`) which implies `else NULL`... I always forget that! :+1:
2. I like `namespace` :+1:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #38144:
URL: https://github.com/apache/arrow/pull/38144#issuecomment-1762708308
The CI job which is failing is the one which runs the linter. If you run `styler:::style_active_file()` on the files that have changed, this will fix it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic merged PR #38144:
URL: https://github.com/apache/arrow/pull/38144
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "orgadish (via GitHub)" <gi...@apache.org>.
orgadish commented on PR #38144:
URL: https://github.com/apache/arrow/pull/38144#issuecomment-1763207336
@thisisnic This is my first PR for a major project like this, so I appreciate your patience with me!
I had totally missed that the tests in `test-data-type.R` already covered `$code()`, and per your recommendation I was able to cover almost all the new code by reverting all my changes to testing and adding some changes for `namespace=TRUE`. Let me know if you think that's stretching too far beyond what's appropriate for this PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on code in PR #38144:
URL: https://github.com/apache/arrow/pull/38144#discussion_r1357552848
##########
r/R/type.R:
##########
@@ -665,13 +694,16 @@ list_of <- function(type) list__(type)
LargeListType <- R6Class("LargeListType",
inherit = NestedType,
public = list(
- code = function() {
- call2("large_list_of", self$value_type$code())
+ code = function(explicit_pkg_name = FALSE) {
+ call2(private$call_name(), self$value_type$code(explicit_pkg_name), .ns = get_pkg_ns(explicit_pkg_name))
}
),
active = list(
value_field = function() LargeListType__value_field(self),
value_type = function() LargeListType__value_type(self)
+ ),
+ private = list(
+ call_name = function() "large_list_of"
Review Comment:
The code feels more skimmable to me with the call name written directly inside the `code()` method instead of pulled from a private field, so I'd lean towards leaving it as-is, unless there's another strong reason to add this here?
##########
r/R/type.R:
##########
@@ -665,13 +694,16 @@ list_of <- function(type) list__(type)
LargeListType <- R6Class("LargeListType",
inherit = NestedType,
public = list(
- code = function() {
- call2("large_list_of", self$value_type$code())
+ code = function(explicit_pkg_name = FALSE) {
+ call2(private$call_name(), self$value_type$code(explicit_pkg_name), .ns = get_pkg_ns(explicit_pkg_name))
Review Comment:
```suggestion
code = function(namespace = FALSE) {
call2(private$call_name(), self$value_type$code(explicit_pkg_name), .ns = if(namespace) "arrow")
```
What do you think of `namespace` as a shorter-but-explicit parameter name?
Using the `.ns` parameter to `call2` is the right approach, but how about we simplify it like this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "orgadish (via GitHub)" <gi...@apache.org>.
orgadish commented on PR #38144:
URL: https://github.com/apache/arrow/pull/38144#issuecomment-1760841177
Testing on e0c655f:
```
test-schema.R
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 84 ]
test-type.R
[ FAIL 0 | WARN 0 | SKIP 1 | PASS 245 ]
── Skipped tests (1) ──────────────────────────────────────────
• Work around masking of data type functions (ARROW-12322)
(1): test-type.R:116:3
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "orgadish (via GitHub)" <gi...@apache.org>.
orgadish commented on PR #38144:
URL: https://github.com/apache/arrow/pull/38144#issuecomment-1759062236
@paleolimbot @thisisnic I believe the PR is ready for final review -- it's passing all my tests, including the new ones. Two notes/questions:
1. `map_of` does not do a roundtrip because it's represented as `list_of(struct(key, value))` instead of `map_of(key, value)`. Should I change that in this PR?
2. `explicit_pkg_name` seems like a very long argument name -- any ideas of how to shorten? Perhaps `use_ns` or `with_ns` (bool) to match the `.ns` argument (string) in `call2`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38144:
URL: https://github.com/apache/arrow/pull/38144#issuecomment-1752263951
:warning: GitHub issue #38033 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38033: [R] Allow `code()` to return package name prefix. [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38144:
URL: https://github.com/apache/arrow/pull/38144#issuecomment-1774093305
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit a0e58f18dc643f9bfd4efa32331ddf477643fed2.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17937812848) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org