nealrichardson commented on a change in pull request #11751:
URL: https://github.com/apache/arrow/pull/11751#discussion_r758378842
##########
File path: r/R/type.R
##########
@@ -55,6 +59,12 @@ DataType$import_from_c <- ImportType
INTEGER_TYPES <- as.character(outer(c("uint", "int"), c(8, 16, 32, 64),
paste0))
FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float",
"double")
+code_carefully <- function(type, msg) {
Review comment:
What does this do? And why is it only used in some places where `code`
is called? (I'm assuming there are good answers to both, and the answers should
go in code comments in/around this function.)
##########
File path: r/R/dictionary.R
##########
@@ -34,6 +34,19 @@ DictionaryType <- R6Class("DictionaryType",
public = list(
ToString = function() {
prettier_dictionary_type(DataType__ToString(self))
+ },
+ code = function() {
Review comment:
Should `code` be an active binding? As in you get it with `x$code`
instead of `x$code()`?
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -427,3 +427,80 @@ test_that("DataType to C-interface", {
# must clean up the pointer or we leak
delete_arrow_schema(ptr)
})
+
+test_that("DataType$code()", {
Review comment:
What about a test helper here that lets you not only eliminate some
copy-paste but also tests that the call that is generated is valid and does
what it claims:
```r
test_code_roundtrip <- function(x) {
expect_identical(eval(x$code()), x)
}
```
##########
File path: r/R/type.R
##########
@@ -130,14 +174,64 @@ TimeType <- R6Class("TimeType",
unit = function() TimeType__unit(self)
)
)
-Time32 <- R6Class("Time32", inherit = TimeType)
-Time64 <- R6Class("Time64", inherit = TimeType)
+Time32 <- R6Class("Time32",
+ inherit = TimeType,
+ public = list(
+ code = function() {
+ unit <- self$unit()
+ unit <- if (unit == TimeUnit$MILLI) {
+ "ms"
+ } else if (unit == TimeUnit$SECOND) {
+ "s"
+ } else {
+ abort(c(
+ "Invalid unit.",
+ i = 'The `Time32` type only supports "ms" (TimeUnit$MILLI) and "s"
(TimeUnit$SECOND).'
+ ), call = call("code"))
+ }
+ call2("time32", unit = unit)
+ }
+ )
+)
+Time64 <- R6Class("Time64",
+ inherit = TimeType,
+ public = list(
+ code = function() {
+ unit <- self$unit()
+ unit <- if (unit == TimeUnit$NANO) {
+ "ns"
+ } else if (unit == TimeUnit$MICRO) {
+ "us"
+ } else {
+ abort(c(
+ "Invalid unit.",
Review comment:
Do we need to validate here? If the object exists, doesn't that mean it
has already been validated by Arrow?
--
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]
