GH-38033: [R] Allow code() to return package name prefix.

[orgadish]

Rationale for this change

#38033

What changes are included in this PR?

• Added get_pkg_ns() helper.
• Added call_name private method to DataType class to store the string name used in the code call. Refactored code() public method to use call_name.
• Converted all $code() call(...) to $code(namespace = FALSE) call2(..., .ns = if(namespace) "arrow") in DataType, Schema, and DictionaryType.
• Added code to Schema docstring.
• Updated expect_code_roundtrip to test roundtrip with and without namespace, and check match/no match for arrow:: depending on namespace argument.

Are these changes tested?

• All tests pass, including lintr checks.

Are there any user-facing changes?

Yes, user-facing changes, but no breaking changes to any public APIs.

• Closes: [R] Option for $schema$code() to explicitly use package name #38033

[github-actions]

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

[orgadish]

@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?

[thisisnic]

Thanks for making this PR! I've added a few initial suggestions - will have a look at the tests in the next round of reviewing!

[orgadish]

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

[orgadish]

Ran all tests on c0a80a9

[ FAIL 0 | WARN 8 | SKIP 32 | PASS 8149 ]

[thisisnic]

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.

[thisisnic]

Thanks for implementing the suggestions there; I've now had a chance to look at the tests and my comments are below. Let me know if you have any questions!

I really appreciate how thorough you've been with these tests, and it looks like you have covered a lot of ground, which is great!

While thorough unit testing is super important, we also have to balance that with maintainability, and the tests here contain lots of lines of code and new functions, which ultimately means that if something does break, it will take a chunk of cognitive effort to engage with as it's harder to skim and immediately get to exactly what's going on.

There's also a lot of ground which is already covered by the tests in expect_code_roundtrip().

Would you mind giving these tests a bit of a refactor, so that the focus is testing that the namespace argument works? It would probably be sufficient testing to write a single test which takes a schema containing fields of a few different types, calls the $code() method with the namespace argument set to TRUE and just checks that the returned code contains the arrow:: prefix.

If there's anything you've covered in your tests here that you think should be added to expect_code_roundtrip(), then that could either be added in in this PR, or for the sake of keeping things simple here we could open an issue for it so it can be done in another PR.

Thanks again for making this PR!

[orgadish]

@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.

[thisisnic]

Looks good to me! I've pushed a couple of final changes to the branch to tweak phrasing and make sure the linter is happy, but otherwise, this is great. Thanks for making this change @orgadish!

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit a0e58f1.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.