Copilot commented on code in PR #50178:
URL: https://github.com/apache/arrow/pull/50178#discussion_r3426622535
##########
r/tests/testthat/test-metadata.R:
##########
@@ -496,24 +496,32 @@ test_that("apply_arrow_r_metadata doesn't add in metadata
from plain data.frame
plain_df <- data.frame(x = 1:5)
plain_df_arrow <- arrow_table(plain_df)
- expect_equal(plain_df_arrow$metadata$r$columns, list(x = NULL))
+ expect_equal(plain_df_arrow$metadata[["r"]]$columns, list(x = NULL))
plain_df_no_metadata <- plain_df_arrow$to_data_frame()
- plain_df_with_metadata <- apply_arrow_r_metadata(plain_df_no_metadata,
plain_df_arrow$metadata$r)
+ plain_df_with_metadata <- apply_arrow_r_metadata(plain_df_no_metadata,
plain_df_arrow$metadata[["r"]])
expect_identical(plain_df_no_metadata, plain_df_with_metadata)
# with more complex column metadata - it preserves it
spicy_df_arrow <- arrow_table(haven_data)
expect_equal(
- spicy_df_arrow$metadata$r$columns,
+ spicy_df_arrow$metadata[["r"]]$columns,
list(num = list(attributes = list(format.spss = "F8.2"), columns = NULL),
cat_int = NULL, cat_chr = NULL)
)
spicy_df_no_metadata <- spicy_df_arrow$to_data_frame()
- spicy_df_with_metadata <- apply_arrow_r_metadata(spicy_df_no_metadata,
spicy_df_arrow$metadata$r)
+ spicy_df_with_metadata <- apply_arrow_r_metadata(spicy_df_no_metadata,
spicy_df_arrow$metadata[["r"]])
expect_null(attr(spicy_df_no_metadata$num, "format.spss"))
expect_equal(attr(spicy_df_with_metadata$num, "format.spss"), "F8.2")
})
+
+test_that("metadata keys starting with 'r' don't cause partial matching -
GH-50163", {
+ tbl <- arrow_table(x = 1:3)
+ tbl <- tbl$cast(tbl$schema$WithMetadata(list(rachel = "some_value")))
+
+ expect_no_warning(as.data.frame(tbl))
+ expect_no_warning(collect.ArrowTabular(tbl))
+})
Review Comment:
The new regression test for GH-50163 covers `as.data.frame()` and
`collect()`, but the reported failure also affects `group_vars()` (via
`group_vars.ArrowTabular`). Adding an assertion here would prevent regressions
in the dplyr grouping metadata path.
##########
r/extra-tests/test-read-files.R:
##########
@@ -183,7 +183,7 @@ test_that("Can see the extra metadata (parquet)", {
if (if_version_less_than("3.0.0")) {
expect_warning(
df <- read_parquet(pq_file),
- "Invalid metadata$r",
+ "Invalid metadata",
fixed = TRUE
)
Review Comment:
This `expect_warning()` now matches any warning starting with "Invalid
metadata", which could allow unrelated warnings to satisfy the assertion. Since
the code emits a specific warning for invalid R metadata, it’s better for this
test to match the full message to keep it meaningful.
##########
r/R/metadata.R:
##########
@@ -48,7 +48,7 @@
if (getOption("arrow.debug", FALSE)) {
print(conditionMessage(e))
}
- warning("Invalid metadata$r", call. = FALSE)
+ warning('Invalid metadata$[["r"]]', call. = FALSE)
NULL
Review Comment:
The warning text changed from `Invalid metadata$r` to `Invalid
metadata$[["r"]]`. This is user-visible and can break downstream code/tests
that match on the warning message, which conflicts with the PR description
claiming no user-facing changes. Consider keeping the existing warning text for
backward compatibility (or updating the PR description/release notes if the
change is intentional).
--
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]
