Skip to content

ARROW-17737: [R] Groups before conversion to a Table must not be restored after collect() #14175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nealrichardson merged 22 commits into from
Oct 14, 2022
Merged

ARROW-17737: [R] Groups before conversion to a Table must not be restored after collect() #14175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
6659080
add tests for compute
eitsupi Oct 1, 2022
55d803d
rename file to match name of file to be tested
eitsupi Oct 1, 2022
cd8fa1d
fix tests
eitsupi Oct 1, 2022
9e1c73b
remove unused line
eitsupi Oct 1, 2022
71c170f
Revert "rename file to match name of file to be tested"
eitsupi Oct 1, 2022
fd4b15e
move test to the other file and rename the test case
eitsupi Oct 1, 2022
2fab634
add tests for compute and collect
eitsupi Oct 6, 2022
7830d2c
more test
eitsupi Oct 6, 2022
dc19476
use NULL for empty group vars, and remove group vars metadata from .data
eitsupi Oct 7, 2022
cae1f26
restore group vars from arrow dplyr query to Table
eitsupi Oct 7, 2022
73edf12
arrow_dplyr_query$group_by_vars should character, and metadata$r$attr…
eitsupi Oct 7, 2022
f6d2a0d
fix
eitsupi Oct 7, 2022
da80d76
use as_arrow_table instead of collect
eitsupi Oct 7, 2022
3012a54
fix typo
eitsupi Oct 7, 2022
d0c545f
add tests
eitsupi Oct 7, 2022
ce5d581
add tests
eitsupi Oct 7, 2022
e117e8c
ensure not to convert to a grouped_df
eitsupi Oct 7, 2022
36674d1
change to manipulate groups only when necessary
eitsupi Oct 7, 2022
8b5ef46
separate the test case
eitsupi Oct 7, 2022
8a1f7cd
fix rebasing
eitsupi Oct 7, 2022
72a5ab6
ensure ungroup .data if it is a Table
eitsupi Oct 11, 2022
a706268
more simplify
eitsupi Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions r/R/dplyr-collect.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,18 @@ restore_dplyr_features <- function(df, query) {
# An arrow_dplyr_query holds some attributes that Arrow doesn't know about
# After calling collect(), make sure these features are carried over

if (length(query$group_by_vars) > 0) {
# Preserve groupings, if present
if (length(dplyr::group_vars(query))) {
if (is.data.frame(df)) {
df <- dplyr::grouped_df(
# Preserve groupings, if present
df <- dplyr::group_by(
df,
dplyr::group_vars(query),
drop = dplyr::group_by_drop_default(query)
!!!syms(dplyr::group_vars(query)),
.drop = dplyr::group_by_drop_default(query),
.add = FALSE
)
} else {
# This is a Table, via compute() or collect(as_data_frame = FALSE)
df$metadata$r$attributes$.group_vars <- query$group_by_vars
df$metadata$r$attributes$.group_vars <- dplyr::group_vars(query)
}
}
df
Expand Down
8 changes: 7 additions & 1 deletion r/R/dplyr.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ arrow_dplyr_query <- function(.data) {
if (inherits(.data, "data.frame")) {
.data <- Table$create(.data)
}
# ARROW-17737: If .data is a Table, remove groups from metadata
# (we've already grabbed the groups above)
if (inherits(.data, "ArrowTabular")) {
.data <- ungroup.ArrowTabular(.data)
}

# Evaluating expressions on a dataset with duplicated fieldnames will error
dupes <- duplicated(names(.data))
if (any(dupes)) {
Expand Down Expand Up @@ -182,7 +188,7 @@ dim.arrow_dplyr_query <- function(x) {
# Query on in-memory Table, so evaluate the filter
# Don't need any columns
x <- select.arrow_dplyr_query(x, NULL)
rows <- nrow(compute.arrow_dplyr_query(x))
rows <- nrow(as_arrow_table(x))
Copy link
Contributor Author

@eitsupi eitsupi Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because manipulating metadata for a table with no rows will cause the size to be updated to 0 x 0.

mtcars |> arrow::arrow_table() |> dplyr::select(NULL) |> arrow::as_arrow_table()
#> Table
#> 32 rows x 0 columns
#>
#>
#> See $metadata for additional Schema metadata
mtcars |> arrow::arrow_table() |> dplyr::select(NULL) |> arrow::as_arrow_table() |> dplyr::ungroup()
#> Table
#> 0 rows x 0 columns
#>
#>
#> See $metadata for additional Schema metadata

Created on 2022-10-07 with reprex v2.0.2

I don't know if this (handling of tables with no rows) is a problem.
A table with 0 rows and multiple columns appears to be quite exceptional, since creating a table from a data frame with no rows results in 0 x 0.

mtcars |> dplyr::select(NULL) |> arrow::arrow_table()
#> Table
#> 0 rows x 0 columns
#>
#>
#> See $metadata for additional Schema metadata

Created on 2022-10-07 with reprex v2.0.2

}
c(rows, cols)
}
Expand Down
33 changes: 33 additions & 0 deletions r/tests/testthat/test-dplyr-group-by.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,25 @@ test_that("ungroup", {
)
})

test_that("Groups before conversion to a Table must not be restored after collect() (ARROW-17737)", {
compare_dplyr_binding(
.input %>%
group_by(chr, .add = FALSE) %>%
ungroup() %>%
collect(),
tbl %>%
group_by(int)
)
compare_dplyr_binding(
.input %>%
group_by(chr, .add = TRUE) %>%
ungroup() %>%
collect(),
tbl %>%
group_by(int)
)
})

test_that("group_by then rename", {
compare_dplyr_binding(
.input %>%
Expand Down Expand Up @@ -196,6 +215,20 @@ test_that("group_by() with .add", {
collect(),
tbl
)
compare_dplyr_binding(
.input %>%
group_by(.add = FALSE) %>%
collect(),
tbl %>%
group_by(dbl2)
)
compare_dplyr_binding(
.input %>%
group_by(.add = TRUE) %>%
collect(),
tbl %>%
group_by(dbl2)
)
compare_dplyr_binding(
.input %>%
group_by(chr, .add = FALSE) %>%
Expand Down