[GitHub] [arrow] nealrichardson commented on a change in pull request #11751: ARROW-14694: [R] Let me dput a schema

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org