nealrichardson commented on a change in pull request #8150:
URL: https://github.com/apache/arrow/pull/8150#discussion_r485914346
##########
File path: r/R/table.R
##########
@@ -185,6 +185,36 @@ Table <- R6Class("Table", inherit = ArrowObject,
)
)
+bad_attributes <- function(x) {
+ UseMethod("bad_attributes")
+}
+
+bad_attributes.default <- function(x) character()
+bad_attributes.data.frame <- function(x) c("class", "row.names", "names")
Review comment:
Is this safe to do? `sf`, for example, creates a subclass of
`data.frame`: https://github.com/r-spatial/sf/blob/master/R/sf.R#L287
But since this works by inheritance, we'll drop that class attribute.
It's (arguably) uglier, but maybe it's better to have a function with a
bunch of if-else rather than use S3 methods, and then we can distinguish
`identical(class(x), "data.frame")` from `inherits(x, "data.frame")`.
##########
File path: r/R/record-batch.R
##########
@@ -278,7 +278,10 @@ as.data.frame.RecordBatch <- function(x, row.names = NULL,
optional = FALSE, ...
apply_arrow_r_metadata <- function(x, r_metadata) {
tryCatch({
if (!is.null(r_metadata$attributes)) {
- attributes(x) <- r_metadata$attributes
+ attributes(x)[names(r_metadata$attributes)] <- r_metadata$attributes
+ if (inherits(x, "POSIXlt")) {
+ attr(x, "row.names") <- NULL
Review comment:
What fails if you remove this special case?
##########
File path: r/R/table.R
##########
@@ -185,6 +185,36 @@ Table <- R6Class("Table", inherit = ArrowObject,
)
)
+bad_attributes <- function(x) {
+ UseMethod("bad_attributes")
+}
+
+bad_attributes.default <- function(x) character()
+bad_attributes.data.frame <- function(x) c("class", "row.names", "names")
+bad_attributes.factor <- function(x) c("class", "levels")
+bad_attributes.Date <- function(x) "class"
+bad_attributes.integer64 <- function(x) "class"
+bad_attributes.POSIXct <- function(x) c("class", "tzone")
+bad_attributes.hms <- function(x) c("class", "units")
+bad_attributes.difftime <- function(x) c("class", "units")
+
+arrow_attributes <- function(x, only_top_level = FALSE) {
+ att <- attributes(x)
+ att <- att[setdiff(names(att), bad_attributes(x))]
+ if (isTRUE(only_top_level)) {
+ return(att)
+ }
+
+ if (is.data.frame(x)) {
+ columns <- map(x, arrow_attributes)
+ if (length(att) || !all(map_lgl(columns, is.null))) {
+ list(attributes = att, columns = columns)
+ }
+ } else if (length(att)) {
+ list(attributes = att, columns = NULL)
+ }
Review comment:
I get that it's implied, but for readability should this end like this?
```suggestion
} else {
NULL
}
```
----------------------------------------------------------------
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]
