Repository navigation
The page_iterator API proble, and proposed fix #194
Description
Activity
I'm a bit surprised we don't already have a
page_sizeargument forlist_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_sizeargument forlist_tables. I believe other methods in this client do have one, right?Well...
list_rowsdoes, 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_rowswe subclassHTTPIterator(in BigQueryRowIterator) and add behavior, includingpage_size. If theHTTPIteratorhandledpage_size, we wouldn't need to handle it inRowIteratorand I think the code inHTTPIteratorwould be a little clearer.Implementing
page_sizeinHTTPIteratorwould benefit most of the otherlist_methods in BigQuery and any other clients that prefer composition over inheritance. :)I suspect that
list_rowscould be refactored to use composition and would be simpler as a result.(BTW, I'm curious what the motivation for
first_page_responseis.)If we wanted to implement
page_sizeinlist_tables, we'd have to subclassHTTPIteratorand 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_sizecan'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))) 33353Reacted by Tim Sweña (Swast)(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_iteratorimplementation, 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 clientIf 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.
It looks to me like the page_iterator API has a problem.
The
Iteratorclass provides pagination, and yet invites clients to do their own pagination by taking a page token. (The documentation forpage_tokenin the BigQuery list_tables method specifically says this.) I would hope that this isn't the intent.Empirically, the default pagination for
list_tablesis 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 formax_results, then the page size increases to 1000 rows, but no more than 1000 and you get no more than that many results. Usingmax_resultsto influence the page size forces the client ofpage_iteratorto do its own pagination if there is any chance of total number of items exceeding the maximummax_results, which forlist_tablesis 2147483647. Arguably, a dataset wouldn't have more than that many tables, but this interface is also used for list_rows, which also limitsmax_resultsto 2147483647. One wouldn't want to limit table rows to 2147483647 just to affect the pagination, although empirically,max_resultsdoesn't affect pagination in the case oflist_rows.A straightforward way to address this would be to add a
page_sizeargument 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?