FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ARROW-8118: [R] dim method for FileSystemDataset by boshek · Pull Request #6635 · apache/arrow · GitHub

/ arrow Public

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

Closed
boshek wants to merge 23 commits into
apache:masterfrom
boshek:dim_method
ConversationCommits (23)ChecksFiles changed
Closed

ARROW-8118: [R] dim method for FileSystemDataset#6635
boshek wants to merge 23 commits into
apache:masterfrom
boshek:dim_method

Conversation

boshek commented Mar 16, 2020

Copy link
Copy Markdown
Contributor
  • Adds dim methods for both Dataset and arrow_dplyr_query classes.
  • Add unit tests for both methods.

github-actions Bot commented Mar 16, 2020

Copy link
Copy Markdown

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

bkietz reviewed Mar 16, 2020
View reviewed changes
Comment thread r/R/dataset.R Outdated
nealrichardson requested changes Mar 16, 2020
View reviewed changes

nealrichardson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread r/R/dataset.R Outdated
Comment thread r/R/dataset.R Outdated
Comment thread r/R/dataset.R Outdated
Comment thread r/R/dataset.R Outdated
Comment thread r/tests/testthat/test-dataset.R Outdated
nealrichardson reviewed Mar 16, 2020
View reviewed changes
Comment thread r/R/dataset.R Outdated
Comment thread r/R/dataset.R Outdated
nealrichardson reviewed Mar 17, 2020
View reviewed changes
Comment thread r/tests/testthat/test-dataset.R Outdated

boshek commented Mar 17, 2020

Copy link
Copy Markdown
Contributor Author

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

nealrichardson commented Mar 18, 2020

Copy link
Copy Markdown
Member

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

nealrichardson reviewed Mar 18, 2020
View reviewed changes

nealrichardson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread r/R/dataset.R Outdated
Comment thread r/R/dataset.R Outdated
Comment thread r/R/dataset.R Outdated
Comment thread r/tests/testthat/test-dplyr.R Outdated

boshek commented Mar 18, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for sticking with me TBH.

nealrichardson commented Mar 18, 2020

Copy link
Copy Markdown
Member

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

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

nealrichardson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 closed this in 91603d8 Mar 19, 2020
asfimport mentioned this pull request Mar 19, 2020
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bkietz bkietz left review comments
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.

3 participants

Back | FazBrowse Home | New Git URL