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

Implement list_tabledata_iter() #77

Merged
merged 6 commits into from Jan 26, 2016

Conversation

@krlmlr
Copy link
Member

commented Jan 25, 2016

The "iterator" pattern for tabledata, extracted from list_tabledata_callback() (which is now using this function).

Kirill Müller
Implement list_tabledata_iter()
The "iterator" pattern for tabledata, extracted from `list_tabledata_callback()` (which is now using this function).
cur_page <- 0L

while(cur_page < max_pages && !iter$is_complete()) {
if (iter$is_complete())

This comment has been minimized.

Copy link
@craigcitro

craigcitro Jan 26, 2016

Collaborator

didn't we just check this on the previous line?

query <- list(maxResults = n)
query$pageToken <- req$pageToken

# Record only page token and total number of rows to reduce memory consmption

This comment has been minimized.

Copy link
@craigcitro

craigcitro Jan 26, 2016

Collaborator

nit: consumption

query$pageToken <- req$pageToken

# Record only page token and total number of rows to reduce memory consmption
req <- bq_get(url, query = query)

This comment has been minimized.

Copy link
@craigcitro

craigcitro Jan 26, 2016

Collaborator

maybe response instead of request?


# Record only page token and total number of rows to reduce memory consmption
req <- bq_get(url, query = query)
req <<- req[c("pageToken", "totalRows")]

This comment has been minimized.

Copy link
@craigcitro

craigcitro Jan 26, 2016

Collaborator

don't we want to pull out req$rows first? (maybe i'm missing something?)

table_info <- table_info %||% get_table(project, dataset, table)
schema <- table_info$schema

url <- sprintf("projects/%s/datasets/%s/tables/%s/data", project, dataset,

This comment has been minimized.

Copy link
@craigcitro

craigcitro Jan 26, 2016

Collaborator

overkill for this PR, but maybe worth pulling out a function that takes a table and gives back that url (well, without the /data part)?

Kirill Müller added some commits Jan 26, 2016

@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2016

Thanks for the review. Removed the unneeded check, now using two variables "response" and "last_response".

Kirill Müller
@craigcitro

This comment has been minimized.

Copy link
Collaborator

commented Jan 26, 2016

LGTM other than the travis failure

Kirill Müller
@krlmlr

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2016

Should be good now, forgot to document.

hadley added a commit that referenced this pull request Jan 26, 2016

Merge pull request #77 from krlmlr/feature/iter
Implement list_tabledata_iter()

@hadley hadley merged commit 2a8c906 into r-dbi:master Jan 26, 2016

1 check passed

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

@krlmlr krlmlr deleted the krlmlr:feature/iter branch Jan 29, 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.