Skip to content

ARROW-9387: [R] Use new C++ table select method#8125

Closed
romainfrancois wants to merge 5 commits intoapache:masterfrom
romainfrancois:ARROW-9387/Table_SelectColumns
Closed

ARROW-9387: [R] Use new C++ table select method#8125
romainfrancois wants to merge 5 commits intoapache:masterfrom
romainfrancois:ARROW-9387/Table_SelectColumns

Conversation

@romainfrancois
Copy link
Copy Markdown
Contributor

@romainfrancois romainfrancois commented Sep 7, 2020 •
edited
Loading

R follow up from #7272

The current $select() uses a more familiar (though more expensive) tidyselect interface:

library(arrow, warn.conflicts = FALSE)

tab <- Table$create(x1 = 1:2, x2 = 3:4, y = 5:6)
# lower level 0-based indices
tab$SelectColumns(0:1)
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>

# higher level tidyselect based
tab$select(starts_with("x"))
#> Table
#> 2 rows x 2 columns
#> $x1 <int32>
#> $x2 <int32>

Created on 2020-09-07 by the reprex package (v0.3.0.9001)

Do we want both ? $select() is used e.g. by the read_csv(col_select=) argument:

tab <- reader$Read()$select(!!enquo(col_select))

@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 7, 2020

https://issues.apache.org/jira/browse/ARROW-9387

nealrichardson
nealrichardson reviewed Sep 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

One suggestion but otherwise LGTM.

r/tests/testthat/test-Table.R
# List-like column slicing
expect_data_frame(tab[2:4], tbl[2:4])
expect_data_frame(tab[c(1, 0)], tbl[c(1, 0)])
expect_data_frame(tab[c(2, 1)], tbl[c(2, 1)])
Copy link
Copy Markdown
Member

@nealrichardson nealrichardson Sep 11, 2020

Choose a reason for hiding this comment

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

Should we maintain the base R behavior for subsetting on column 0 (it is ignored)? I can't imagine why someone would do such a thing on purpose.

nealrichardson
nealrichardson approved these changes Sep 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

Will merge when green

@asfimport asfimport mentioned this pull request Sep 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nealrichardson nealrichardson nealrichardson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@romainfrancois @nealrichardson