nealrichardson commented on a change in pull request #11277:
URL: https://github.com/apache/arrow/pull/11277#discussion_r719378308
##########
File path: r/R/metadata.R
##########
@@ -142,7 +142,8 @@ arrow_attributes <- function(x, only_top_level = FALSE) {
}
columns <- NULL
- if (is.list(x) && !inherits(x, "POSIXlt")) {
+ attempt_to_save_row_level <- getOption("arrow.preserve_row_level_metadata",
FALSE)
+ if (attempt_to_save_row_level && is.list(x) && !inherits(x, "POSIXlt")) {
# However, if we are inside of a dplyr collection (including all datasets),
# we cannot apply this row-level metadata, since the order of the rows is
# not guaranteed to be the same, so don't even try, but warn what's going
on
Review comment:
Since we're no longer writing row-level metadata at all, can this logic
below be removed?
##########
File path: r/R/metadata.R
##########
@@ -142,7 +142,8 @@ arrow_attributes <- function(x, only_top_level = FALSE) {
}
columns <- NULL
- if (is.list(x) && !inherits(x, "POSIXlt")) {
+ attempt_to_save_row_level <- getOption("arrow.preserve_row_level_metadata",
FALSE)
Review comment:
Why is this an option at all?
##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -214,15 +214,30 @@ test_that("metadata drops readr's problems attribute", {
expect_null(attr(as.data.frame(tab), "problems"))
})
-test_that("metadata of list elements (ARROW-10386)", {
+test_that("Row-level metadata (does not by default) roundtrip", {
+ # First tracked at ARROW-10386, though it was later determined that row-level
+ # metadata should be handled separately ARROW-14020, ARROW-XXX
df <- data.frame(x = I(list(structure(1, foo = "bar"), structure(2, baz =
"qux"))))
tab <- Table$create(df)
- expect_identical(attr(as.data.frame(tab)$x[[1]], "foo"), "bar")
- expect_identical(attr(as.data.frame(tab)$x[[2]], "baz"), "qux")
+ r_metadata <- .unserialize_arrow_r_metadata(tab$metadata$r)
+ expect_null(r_metadata$columns$x$columns)
+
+ # But we can re-enable this / read data that has already been written with
+ # row-level metadata
+ withr::with_options(
+ list("arrow.preserve_row_level_metadata" = TRUE),
+ {
Review comment:
`withr::local_options()` can be used in here instead, and that will
eliminate the check warning (if in fact this should be optional).
--
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]
