The page_iterator API proble, and proposed fix

It looks to me like the page_iterator API has a problem.

The Iterator class provides pagination, and yet invites clients to do their own pagination by taking a page token. (The documentation for page_token in the BigQuery list_tables method specifically says this.) I would hope that this isn't the intent.

Empirically, the default pagination for list_tables is 50 rows per page, which is arguably too low, causing many REST API calls if there are many tables in a dataset. If you pass a largish number for max_results, then the page size increases to 1000 rows, but no more than 1000 and you get no more than that many results. Using max_results to influence the page size forces the client of page_iterator to do its own pagination if there is any chance of total number of items exceeding the maximum max_results, which for list_tables is 2147483647. Arguably, a dataset wouldn't have more than that many tables, but this interface is also used for list_rows, which also limits max_results to 2147483647. One wouldn't want to limit table rows to 2147483647 just to affect the pagination, although empirically, max_results doesn't affect pagination in the case of list_rows.

A straightforward way to address this would be to add a page_size argument to the iterator. Of course, to get the benefit, the option would need to be added to higher-level libraries.

I'd be happy to create a PR to add this argument.

BTW, it's weird to take page_token. Is this a holdover from an earlier design? Should it be deprecated?

I'm a bit surprised we don't already have a page_size argument for list_tables. I believe other methods in this client do have one, right?

Is this filed on the correct repo? I think we handle this in BigQuery by populating the "extra headers" property on the iterator.

I agree re: page_token. It is confusing, as it provides a pass-through to the lower-level API. I'm not sure when this would be desired, but also not 100% sure it should be deprecated.

I'm a bit surprised we don't already have a page_size argument for list_tables. I believe other methods in this client do have one, right?

Well... list_rows does, and the internal _list_rows_from_query_results. That's interesting and the only case.

Other list_ methods don't.

Is this filed on the correct repo? I think we handle this in BigQuery by populating the "extra headers" property on the iterator.

For list_tables, we just call HTTPIterator, which is in this repo.

For list_rows we subclass HTTPIterator(in BigQuery RowIterator) and add behavior, including page_size. If the HTTPIterator handled page_size, we wouldn't need to handle it in RowIterator and I think the code in HTTPIterator would be a little clearer.

Implementing page_size in HTTPIterator would benefit most of the other list_ methods in BigQuery and any other clients that prefer composition over inheritance. :)

I suspect that list_rows could be refactored to use composition and would be simpler as a result.

(BTW, I'm curious what the motivation for first_page_response is.)

If we wanted to implement page_size in list_tables, we'd have to subclass HTTPIterator and duplicate code.

I agree re: page_token. It is confusing, as it provides a pass-through to the lower-level API. I'm not sure when this would be desired, but also not 100% sure it should be deprecated.

It invites users to invent higher-level pagination. Don't we want to discourage that?

Fun fact, for list_rows, page_size can't be used to get bigger pages -- only smaller ones. :)

>>> it = client.list_rows('bigquery-public-data.cymbal_investments.trade_capture_report').pages
>>> len(list(next(it)))
33353
>>> len(list(next(it)))
33321
>>> it = client.list_rows('bigquery-public-data.cymbal_investments.trade_capture_report', page_size=9999).pages
>>> len(list(next(it)))
9999
>>> it = client.list_rows('bigquery-public-data.cymbal_investments.trade_capture_report', page_size=99999).pages
>>> len(list(next(it)))
33353

(BTW, I'm curious what the motivation for first_page_response is.)

I suspect it'll be useful when we implement googleapis/python-bigquery#589 It's actually leftover from my initial (failed) attempt at using that API as recommended by the backend team, but resulted in worse performance in too many cases. I reverted most of the code, so that's dead right now, but may be revived when I resume work on that.

If we wanted to implement page_size in list_tables, we'd have to subclass HTTPIterator and duplicate code.

Makes sense to add it here to me, in that case.

Hi folks,

I am not very familiar with the page_iterator implementation, and it looks like it isn't used by the GAPICs. I am onboard with whatever changes work best for the handwritten clients that use this API.

In case it's useful for comparison, here's one paged method in google-cloud-automl, a fully generated client

If we wanted to implement page_size in list_tables, we'd have to subclass HTTPIterator and duplicate code.

Makes sense to add it here to me, in that case.

Cool. I'll work up a PR.

Navigation Menu

The page_iterator API proble, and proposed fix #194

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

The page_iterator API proble, and proposed fix #194

Description

Activity

tswast commented on May 24, 2021

jimfulton commented on May 25, 2021

jimfulton commented on May 25, 2021

tswast commented on May 25, 2021

tswast commented on May 25, 2021

busunkim96 commented on May 25, 2021

jimfulton commented on May 26, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions