Move BigQuery list_ methods to use iterators by dhermes · Pull Request #2565 · googleapis/google-cloud-python · GitHub

Follow up to #2561. This only covers Dataset.list_tables() and Table.fetch_data(). It may also be "correct" to use an Iterator in

QueryResults.fetch_data
QueryResults.run (via QueryResults._build_resource)

but I wanted to get eyes on this change first / opinion from the original author (@tseaver) before moving forward.

@tseaver Can you weigh in here? I'm especially curious how you feel about fetch_data (both in Table and QueryResults) and QueryResults.run.

I'm especially curious how you feel about fetch_data (both in Table and QueryResults) and QueryResults.run.

+0 for adding it to fetch_data: those methods currently return a 3-tuple, row_data, total_rows, page_token, which means we would have to choose what to do with the total_rows count.

-1 for adding it to run: that doesn't return row data, paged or not.

+0 for adding it to fetch_data: those methods currently return a 3-tuple, row_data, total_rows, page_token, which means we would have to choose what to do with the total_rows count.

Check out the implementation, I put total_rows on iterator.page.

LGTM, once you decide about my question about where total_rows should be exposed.

@tseaver I'll put total_rows on both once back at keyboard. Now that you've seen the implementation, does it seem appropriate to change QueryResults.fetch_data over as well?

@tseaver I started writing the code to put total_rows on the Iterator instance and realized something. If while paging, the first request has totalRows: 1200 and the second request somehow has the key missing, then on the second page we'd either have to del iterator.total_rows or iterator.total_rows = None or have an invalid value.

Given this scenario, it seems like my_iter.page.total_rows is the only real correct place for total_rows since it is actually a page-specific value (it may even change between pages if the backend is processing more rows between requests?)

@dhermes For queries, the docs say totalRows is only present on a page of results if the query has already completed: we can therefore presume that i will not change.

For tables, the docs say only that totalRows represents "[t]he total number of rows in the complete table." We would have to ask the back-end team whether the page_token encodes a "timestamped" cursor which would make totalRows constant across pages, even if new values were inserted into the table in the meanwhile.

@fhoffa Can you chime in on how stable the totalRows value is for tabledata?

In particular, isolating the logic useful to work on a single row.

Also fixing BigQuery system test in the process.

@tseaver I put total_rows only on the Iterator and just allowed it to be None if missing in the response.

Move BigQuery list_ methods to use iterators

dhermes added the api: bigquery Issues related to the BigQuery API. label Oct 19, 2016

dhermes assigned daspecster and tseaver Oct 19, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 19, 2016

daspecster reviewed Oct 19, 2016

View reviewed changes

tseaver reviewed Oct 20, 2016

View reviewed changes

Comment thread bigquery/google/cloud/bigquery/table.py Outdated

"""

total_rows = response.get('totalRows')

if total_rows is not None:

page.total_rows = int(total_rows)

This comment was marked as spam.

Sign in to view

tseaver added the backend label Oct 25, 2016

dhermes added 4 commits October 31, 2016 22:30

Making BigQuery dataset.list_tables() into an iterator.

5b0f9c8

Refactoring _rows_from_json BigQuery helper.

092dd09

In particular, isolating the logic useful to work on a single row.

Making BigQuery table.fetch_data() into an iterator.

0e7b01f

Rebase fixes.

d3a6dff

dhermes force-pushed the bigquery-iterators-2 branch from e439de8 to d3a6dff Compare November 1, 2016 05:53

Review feedback: moving total_rows from Page to Iterator.

76fa05c

Also fixing BigQuery system test in the process.

tseaver approved these changes Nov 1, 2016

View reviewed changes

dhermes merged commit 4b4023f into googleapis:master Nov 1, 2016

dhermes deleted the bigquery-iterators-2 branch November 1, 2016 15:55

dhermes mentioned this pull request Nov 14, 2016

Upgrading core to version to 0.21.0. #2733

Merged

richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017

Merge pull request googleapis#2565 from dhermes/bigquery-iterators-2

9d10874

Move BigQuery list_ methods to use iterators

theacodes unassigned daspecster Sep 28, 2018

               TEST_RC_REPLACEMENTS = {
                   'FORMAT': {
-                      'max-module-lines': 1960,
+                      'max-module-lines': 2000,

+                                          page_token=page_token, max_results=max_results,
+                                          page_start=_rows_page_start)
+                      iterator.schema = self._schema
+                      # Over-ride the key used to retrieve the next page token.

-                                                                      max_results=MAX,
-                                                                      page_token=TOKEN)
+                      iterator = table.fetch_data(
+                          client=client2, max_results=MAX, page_token=TOKEN)



		def _row_from_json(row, schema):
		"""Convert JSON row data to row w/ appropriate types.

+                  """Convert JSON row data to row w/ appropriate types.
+                  :type row: dict
+                  :param row:

		return _row_from_json(resource, iterator.schema)


		# pylint: disable=unused-argument

+                  """
+                  total_rows = response.get('totalRows')
+                  if total_rows is not None:
+                      page.total_rows = int(total_rows)

Move BigQuery list_ methods to use iterators by dhermes · Pull Request #2565 · googleapis/google-cloud-python · GitHub

Uh oh!

Move BigQuery list_ methods to use iterators - #2565

Move BigQuery list_ methods to use iterators#2565dhermes merged 5 commits intogoogleapis:mastergoogleapis/google-cloud-python:masterfrom dhermes:bigquery-iterators-2dhermes/google-cloud-python:bigquery-iterators-2Copy head branch name to clipboard

Conversation

dhermes commented Oct 19, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes commented Oct 19, 2016

Uh oh!

tseaver commented Oct 20, 2016

Uh oh!

dhermes commented Oct 20, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

tseaver commented Oct 20, 2016

Uh oh!

dhermes commented Oct 20, 2016

Uh oh!

dhermes commented Oct 20, 2016

Uh oh!

tseaver commented Oct 20, 2016

Uh oh!

tseaver commented Oct 25, 2016

Uh oh!

dhermes commented Nov 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Move BigQuery list_ methods to use iterators#2565
dhermes merged 5 commits into
googleapis:masterfrom
dhermes:bigquery-iterators-2