nealrichardson commented on a change in pull request #8150:
URL: https://github.com/apache/arrow/pull/8150#discussion_r487119708
##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -890,6 +890,7 @@ test_that("Dataset writing: from data.frame", {
select(string = chr, integer = int) %>%
filter(integer > 6 & integer < 11) %>%
collect() %>%
+ ungroup() %>%
Review comment:
This isn't desirable because for `write_dataset()`, `group_by` is used
to convey which columns to partition the dataset on. So I think there needs to
be an `ungroup()` somewhere around L71 in write-dataset.R
##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -124,5 +125,11 @@ test_that("haven types roundtrip via feather", {
test_that("Date/time type roundtrip", {
rb <- record_batch(example_with_times)
expect_is(rb$schema$posixlt$type, "StructType")
- expect_identical(as.data.frame(rb), example_with_times)
+ expect_equal(as.data.frame(rb), example_with_times)
+})
+
+test_that("metadata keeps attribute of top level data frame", {
+ df <- structure(data.frame(x = 1, y = 2), foo = "bar")
+ tab <- Table$create(df)
+ expect_equal(attr(as.data.frame(tab), "foo"), "bar")
Review comment:
why not `expect_identical(as.data.frame(tab), df)`?
##########
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:
I restored that special case and added a comment explaining it.
##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -890,6 +890,7 @@ test_that("Dataset writing: from data.frame", {
select(string = chr, integer = int) %>%
filter(integer > 6 & integer < 11) %>%
collect() %>%
+ ungroup() %>%
Review comment:
This isn't desirable because for `write_dataset()`, `group_by` is used
to convey which columns to partition the dataset on. So I think there needs to
be an `ungroup()` somewhere around L71 in write-dataset.R
##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -124,5 +125,11 @@ test_that("haven types roundtrip via feather", {
test_that("Date/time type roundtrip", {
rb <- record_batch(example_with_times)
expect_is(rb$schema$posixlt$type, "StructType")
- expect_identical(as.data.frame(rb), example_with_times)
+ expect_equal(as.data.frame(rb), example_with_times)
+})
+
+test_that("metadata keeps attribute of top level data frame", {
+ df <- structure(data.frame(x = 1, y = 2), foo = "bar")
+ tab <- Table$create(df)
+ expect_equal(attr(as.data.frame(tab), "foo"), "bar")
Review comment:
why not `expect_identical(as.data.frame(tab), df)`?
##########
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:
I restored that special case and added a comment explaining it.
##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -890,6 +890,7 @@ test_that("Dataset writing: from data.frame", {
select(string = chr, integer = int) %>%
filter(integer > 6 & integer < 11) %>%
collect() %>%
+ ungroup() %>%
Review comment:
This isn't desirable because for `write_dataset()`, `group_by` is used
to convey which columns to partition the dataset on. So I think there needs to
be an `ungroup()` somewhere around L71 in write-dataset.R
##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -124,5 +125,11 @@ test_that("haven types roundtrip via feather", {
test_that("Date/time type roundtrip", {
rb <- record_batch(example_with_times)
expect_is(rb$schema$posixlt$type, "StructType")
- expect_identical(as.data.frame(rb), example_with_times)
+ expect_equal(as.data.frame(rb), example_with_times)
+})
+
+test_that("metadata keeps attribute of top level data frame", {
+ df <- structure(data.frame(x = 1, y = 2), foo = "bar")
+ tab <- Table$create(df)
+ expect_equal(attr(as.data.frame(tab), "foo"), "bar")
Review comment:
why not `expect_identical(as.data.frame(tab), df)`?
##########
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:
I restored that special case and added a comment explaining it.
##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -890,6 +890,7 @@ test_that("Dataset writing: from data.frame", {
select(string = chr, integer = int) %>%
filter(integer > 6 & integer < 11) %>%
collect() %>%
+ ungroup() %>%
Review comment:
This isn't desirable because for `write_dataset()`, `group_by` is used
to convey which columns to partition the dataset on. So I think there needs to
be an `ungroup()` somewhere around L71 in write-dataset.R
##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -124,5 +125,11 @@ test_that("haven types roundtrip via feather", {
test_that("Date/time type roundtrip", {
rb <- record_batch(example_with_times)
expect_is(rb$schema$posixlt$type, "StructType")
- expect_identical(as.data.frame(rb), example_with_times)
+ expect_equal(as.data.frame(rb), example_with_times)
+})
+
+test_that("metadata keeps attribute of top level data frame", {
+ df <- structure(data.frame(x = 1, y = 2), foo = "bar")
+ tab <- Table$create(df)
+ expect_equal(attr(as.data.frame(tab), "foo"), "bar")
Review comment:
why not `expect_identical(as.data.frame(tab), df)`?
##########
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:
I restored that special case and added a comment explaining it.
##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -890,6 +890,7 @@ test_that("Dataset writing: from data.frame", {
select(string = chr, integer = int) %>%
filter(integer > 6 & integer < 11) %>%
collect() %>%
+ ungroup() %>%
Review comment:
This isn't desirable because for `write_dataset()`, `group_by` is used
to convey which columns to partition the dataset on. So I think there needs to
be an `ungroup()` somewhere around L71 in write-dataset.R
##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -124,5 +125,11 @@ test_that("haven types roundtrip via feather", {
test_that("Date/time type roundtrip", {
rb <- record_batch(example_with_times)
expect_is(rb$schema$posixlt$type, "StructType")
- expect_identical(as.data.frame(rb), example_with_times)
+ expect_equal(as.data.frame(rb), example_with_times)
+})
+
+test_that("metadata keeps attribute of top level data frame", {
+ df <- structure(data.frame(x = 1, y = 2), foo = "bar")
+ tab <- Table$create(df)
+ expect_equal(attr(as.data.frame(tab), "foo"), "bar")
Review comment:
why not `expect_identical(as.data.frame(tab), df)`?
##########
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:
I restored that special case and added a comment explaining it.
##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -890,6 +890,7 @@ test_that("Dataset writing: from data.frame", {
select(string = chr, integer = int) %>%
filter(integer > 6 & integer < 11) %>%
collect() %>%
+ ungroup() %>%
Review comment:
This isn't desirable because for `write_dataset()`, `group_by` is used
to convey which columns to partition the dataset on. So I think there needs to
be an `ungroup()` somewhere around L71 in write-dataset.R
##########
File path: r/tests/testthat/test-metadata.R
##########
@@ -124,5 +125,11 @@ test_that("haven types roundtrip via feather", {
test_that("Date/time type roundtrip", {
rb <- record_batch(example_with_times)
expect_is(rb$schema$posixlt$type, "StructType")
- expect_identical(as.data.frame(rb), example_with_times)
+ expect_equal(as.data.frame(rb), example_with_times)
+})
+
+test_that("metadata keeps attribute of top level data frame", {
+ df <- structure(data.frame(x = 1, y = 2), foo = "bar")
+ tab <- Table$create(df)
+ expect_equal(attr(as.data.frame(tab), "foo"), "bar")
Review comment:
why not `expect_identical(as.data.frame(tab), df)`?
##########
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:
I restored that special case and added a comment explaining it.
----------------------------------------------------------------
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]
