Skip to content
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

Jump to bottom

ARROW-8118: [R] dim method for FileSystemDataset #6635

Closed
wants to merge 23 commits into from
Closed

ARROW-8118: [R] dim method for FileSystemDataset #6635

wants to merge 23 commits into from

Conversation

boshek
Copy link
Contributor

@boshek boshek commented Mar 16, 2020

  • Adds dim methods for both Dataset and arrow_dplyr_query classes.
  • Add unit tests for both methods.

@github-actions
Copy link

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

bkietz
bkietz reviewed Mar 16, 2020
View reviewed changes
r/R/dataset.R Outdated Show resolved Hide resolved
nealrichardson
nealrichardson requested changes Mar 16, 2020
View reviewed changes
Copy link
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.

Thanks for taking a stab at this! A few suggestions.

r/R/dataset.R Outdated Show resolved Hide resolved
r/R/dataset.R Outdated Show resolved Hide resolved
r/R/dataset.R Outdated Show resolved Hide resolved
r/R/dataset.R Outdated Show resolved Hide resolved
r/tests/testthat/test-dataset.R Outdated Show resolved Hide resolved
nealrichardson
nealrichardson reviewed Mar 16, 2020
View reviewed changes
r/R/dataset.R Outdated Show resolved Hide resolved
r/R/dataset.R Outdated Show resolved Hide resolved
@boshek
Copy link
Contributor Author

boshek commented Mar 17, 2020

I can't quite understand the R CMD check failure. Passes locally. I will have to work on it some more.

@nealrichardson
Copy link
Member

Hmm, maybe try roxygen2::roxygenize()ing? I don't see the NAMESPACE modified and I'd expect that it should be.

nealrichardson
nealrichardson reviewed Mar 18, 2020
View reviewed changes
Copy link
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.

Thanks for hanging with this, looks like we're close

r/R/dataset.R Outdated Show resolved Hide resolved
r/R/dataset.R Outdated Show resolved Hide resolved
r/R/dataset.R Outdated Show resolved Hide resolved
r/tests/testthat/test-dplyr.R Outdated Show resolved Hide resolved
@boshek
Copy link
Contributor Author

boshek commented Mar 18, 2020

Thanks for sticking with me TBH.

@nealrichardson
Copy link
Member

I'll pull this and take a look today, see if I can work out the test failures.

bkietz
bkietz reviewed Mar 18, 2020
View reviewed changes
r/R/dataset.R Outdated Show resolved Hide resolved
boshek and others added 19 commits March 19, 2020 10:56
@boshek @nealrichardson
add dim method to for Dataset objects
55b0fde
@boshek @nealrichardson
add dim method for arrow_dplyr_query that only fails + test
3d17d34
@boshek @nealrichardson
rename dim error test
d3c90b5
@boshek @nealrichardson
test directly on number of row and column integers
5e45e0f
@boshek @nealrichardson
Update r/R/dataset.R
8f8065d 
Co-Authored-By: Neal Richardson <neal.p.richardson@gmail.com>
@boshek @nealrichardson
check that dim method has FileSystemDataset class and is a parquet file
275a5eb
@boshek @nealrichardson
Update r/R/dataset.R
af153fc 
Co-Authored-By: Neal Richardson <neal.p.richardson@gmail.com>
@boshek @nealrichardson
remove the arrow_dplyr_query test since it will change
fe11e33
@boshek @bkietz @nealrichardson
adding FileReader num_rows cpp code.
d5a033f 
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
@boshek @nealrichardson
fix numbers to integers
5a06f6b
@boshek @nealrichardson
return integer from dim and test accordingly
8060c40
@boshek @nealrichardson
expose FileReader num_rows as active binding and use in dim
ab3ae0f
@boshek @nealrichardson
add more flexible dim.arrow_dplyr_query method + tests
741d5de
@boshek @nealrichardson
remove redundant check
0d8ba77
@boshek @nealrichardson
Update r/R/dataset.R
126c48b 
Co-Authored-By: Neal Richardson <neal.p.richardson@gmail.com>
@boshek @nealrichardson
Update r/tests/testthat/test-dplyr.R
a38a32a 
Co-Authored-By: Neal Richardson <neal.p.richardson@gmail.com>
@boshek @nealrichardson
Update r/R/dataset.R
db2f9c3 
Co-Authored-By: Neal Richardson <neal.p.richardson@gmail.com>
@boshek @nealrichardson
add dim method to namespace
8e0516b
@boshek @nealrichardson
better description of test
287a0eb
nealrichardson
nealrichardson approved these changes Mar 19, 2020
View reviewed changes
Copy link
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.

I just rebased, moved a few things around, and changed the fallback behavior for non-parquet datasets to be the same as for arrow_dplyr_query (warn and return NA rows). I'll merge when CI passes. Thanks again @boshek!

@bkietz bkietz closed this in 91603d8 Mar 19, 2020
@asfimport asfimport mentioned this pull request Mar 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkietz bkietz bkietz left review comments

@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.

None yet
3 participants
@boshek @nealrichardson @bkietz