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:
[email protected]
