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.

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

table data iter gains new members next_paged(), get_schema() and get_rows_fetched() #87

Merged
merged 19 commits into from Feb 24, 2016

Conversation

@krlmlr
Copy link
Member

commented Feb 5, 2016

Allows fetching all rows from a query, extracted DEFAULT_PAGE_SIZE as private variable.

Kirill Müller added some commits Feb 5, 2016

Kirill Müller
@@ -131,7 +133,16 @@ list_tabledata_iter <- function(project, dataset, table, table_info = NULL) {
!is.null(last_response) && rows_fetched >= as.integer(last_response$totalRows)
}

list(next_ = next_, is_complete = is_complete)
all <- function(page_size = DEFAULT_PAGE_SIZE) {

This comment has been minimized.

Copy link
@craigcitro

craigcitro Feb 5, 2016

Collaborator

it might be worth taking this as an argument to list_tabledata_iter, even if just for testing purposes.

This comment has been minimized.

Copy link
@krlmlr

krlmlr Feb 5, 2016

Author Member

Good point. I'll convert this to a functions, which then can be mocked by tests.

This comment has been minimized.

Copy link
@craigcitro

craigcitro Feb 9, 2016

Collaborator

Ah, I actually meant something slightly different -- make this an explicit argument to list_tabledata_iter, and then pass an explicit value in tests. (it's also sometimes useful for end-users, though more rare.)

@craigcitro

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2016

One small nit, and the standard question -- can we add a test? 😁

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2016

#14.

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2016

default_page_size() is now a function. Also added two more members.

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2016

re #87 (comment): I think that it's a bit too much to support an argument to the outer function that specifies the default value (which can be overridden in all() anyway if necessary). For testing, default_page_size() can be mocked.

@@ -52,7 +54,8 @@ list_tabledata <- function(project, dataset, table, page_size = 1e4,
#' @export
list_tabledata_callback <- function(project, dataset, table, callback,
table_info = NULL,
page_size = 1e4, max_pages = 10,
page_size = default_page_size(),

This comment has been minimized.

Copy link
@hadley

hadley Feb 10, 2016

Member

I don't like using non-exported functions for defaults because it's hard to find out what the default value is

This comment has been minimized.

Copy link
@krlmlr

krlmlr Feb 10, 2016

Author Member

I'll add an option instead.

list(next_ = next_, is_complete = is_complete)
all <- function(page_size = default_page_size()) {
ret <- list()
while (!is_complete()) {

This comment has been minimized.

Copy link
@hadley

hadley Feb 10, 2016

Member

This feels a bit icky, but I think doing it right will be a lot of work and is unlikely to have practical performance implication. Instead add a comment that says: "Fetching large data from bigrquery will be slow for other reasons..."

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

@craigcitro: The all() function is needed for DBI, and will be tested there implicitly. We probably want to set the default page size to something smallish there.

@krlmlr krlmlr changed the title table data iter gains new member all() table data iter gains new members next_paged(), get_schema() and get_rows_fetched() Feb 11, 2016

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

Now using an option to define default page size, plus other enhancements. @craigcitro: PTAL.

Kirill Müller

@krlmlr krlmlr referenced this pull request Feb 11, 2016

Merged

DBI backend #78

@hadley

This comment has been minimized.

Copy link
Member

commented Feb 12, 2016

LGTM

@craigcitro

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2016

Just to check, is the diff here picking up anything extra? (In particular, the extra note at the top of NEWS looks like it should be on another PR?)

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2016

The NEWS item was missing from another PR, I added it here for simplicity. Besides that, all changes relate to the list_tabledata_iter() function; I've also switched from integer to numeric to account for results with more than 2G rows.

krlmlr added a commit that referenced this pull request Feb 24, 2016

Merge pull request #87 from krlmlr/feature/iter-all
- table data iterator gains new members `next_paged()`, `get_schema()` and `get_rows_fetched()` (#87. @krlmlr).

@krlmlr krlmlr merged commit 2866909 into r-dbi:master Feb 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krlmlr krlmlr deleted the krlmlr:feature/iter-all branch Feb 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.