[GitHub] [arrow] jonkeane commented on a change in pull request #8549: ARROW-10386 [R]: List column class attributes not preserved in roundtrip

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

jonkeane commented on a change in pull request #8549:
URL: https://github.com/apache/arrow/pull/8549#discussion_r551552588



##########
File path: r/R/record-batch.R
##########
@@ -286,6 +286,20 @@ as.data.frame.RecordBatch <- function(x, row.names = NULL, optional = FALSE, ...
 
 apply_arrow_r_metadata <- function(x, r_metadata) {
   tryCatch({
+    columns_metadata <- r_metadata$columns
+    if (is.data.frame(x)) {
+      if (length(names(x)) && !is.null(columns_metadata)) {
+        for (name in intersect(names(columns_metadata), names(x))) {
+          x[[name]] <- apply_arrow_r_metadata(x[[name]], columns_metadata[[name]])
+        }
+      }
+    } else if(is.list(x) && !inherits(x, "POSIXlt") && !is.null(columns_metadata)) {
+      x <- map2(x, columns_metadata, function(.x, .y) {
+        apply_arrow_r_metadata(.x, .y)
+      })
+      x
+    }

Review comment:
       Could we move `!is.null(columns_metadata)` up to line 290 instead of having it on both 291 and 296?

##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -134,3 +134,10 @@ test_that("metadata keeps attribute of top level data frame", {
   expect_identical(attr(as.data.frame(tab), "foo"), "bar")
   expect_identical(as.data.frame(tab), df)
 })
+
+test_that("metadata of list elements (ARROW-10386)", {
+  df <- data.frame(x = I(list(structure(1, foo = "bar"), structure(2, foo = "bar"))))
+  tab <- Table$create(df)
+  expect_identical(attr(as.data.frame(tab)$x[[1]], "foo"), "bar")
+  expect_identical(attr(as.data.frame(tab)$x[[2]], "foo"), "bar")

Review comment:
       I wonder if it would be clearer to have different attributes on each item/row to make it super obvious that we're not picking the attributes of the first item/row and copying them for the whole column.




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

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