feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter to control page size by jimfulton · Pull Request #686 · googleapis/python-bigquery · GitHub

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #685 🦕

Code-wise it looks good, HTTPIterator() instances are instantiated with the new page_size argument. 👍 I only found missing license headers and a punctuation nit.

Two observations to discuss:

RowIterator subclasses HTTPIterator, but does not pass page_size to it in __init__(), although it does store it a few lines after, overriding the value the superclass sets. Should we nevertheless explicitly pass page_size to the superclass in case the latter's logic changes in a non-trivial way?
The new google-api-core version pin is narrowed. We had problems in the past when different clout libraries pinned incompatible version ranges, causing conflicts, thus we tend to keep the ranges wide, if possible.
A quick glance over the core issue tracker does not reveal anything significant that would require capping the python-api-core version in other libraries, thus it's probably fine, just mentioning it as a sanity check.

@tswast Are you aware of any current issues in api-core that require a temporary version cap in any of the other libraries?

Code-wise it looks good, HTTPIterator() instances are instantiated with the new page_size argument. +1 I only found missing license headers and a punctuation nit.

Doh!

Two observations to discuss:

* `RowIterator` subclasses HTTPIterator, but does not pass `page_size` to it in [__init__()](https://github.com/googleapis/python-bigquery/blob/dea2402ef62bcc00f2a392b16330a595db38ffb7/google/cloud/bigquery/table.py#L1467-L1478), although it does store it a few lines after, overriding the value the superclass sets. Should we nevertheless explicitly pass `page_size` to the superclass in case the latter's logic changes in a non-trivial way?

RowIterator is excluded from this because it already implements page size in a slightly different way that is specific to its needs.

* The new `google-api-core` version pin is narrowed. We had problems in the past when different clout libraries pinned incompatible version ranges, causing conflicts, thus we tend to keep the ranges wide, if possible.
  A quick glance over the core issue tracker does not reveal anything significant that would require capping the `python-api-core` version in other libraries, thus it's probably fine, just mentioning it as a sanity check.

Of course, I only upped the minimum version, because we depend on a change there.

Thanks for the review!!!

@tswast Are you aware of any current issues in api-core that require a temporary version cap in any of the other libraries?

I'm not aware of any, though Yoshi chat might have better visibility on that. The main issue is that people can end up pinning older versions in their applications / environments. Previously, this only gave a warning in pip, but with later versions it should show an error.

A license header is duplicated, but otherwise looks good.

Duplicated license? :)

Suggested change

      
            # Copyright 2021 Google LLC
          
            # Licensed under the Apache License, Version 2.0 (the "License");
          
            # you may not use this file except in compliance with the License.
          
            # You may obtain a copy of the License at
          
            #     https://www.apache.org/licenses/LICENSE-2.0
          
            # Unless required by applicable law or agreed to in writing, software
          
            # distributed under the License is distributed on an "AS IS" BASIS,
          
            # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          
            # See the License for the specific language governing permissions and
          
            # limitations under the License.

Fixed

TL; DR - updating the google-api-core is probably fine, we just need to resolve the duplicate license header and that should be it.

Of course, I only upped the minimum version, because we depend on a change there.

Yeah, it's just that this is now the latest and greatest version, while at a few occasions some of the libraries actually had to cap the maximum google-api-core version temporary as a workaround due to a regression. This would cause a version conflict, but then again, nothing stands out from the issue tracker, thus we're probably fine.

The main issue is that people can end up pinning older versions in their applications / environments. Previously, this only gave a warning in pip, but with later versions it should show an error.

WIth the new pip resolver, wouldn't that just not install the latest google-cloud-bigquery version, because it depends on the newer api-core version than the one pinned, and would just pick a less recent google-cloud-bigquery? Besides, as long as they can update the pin without causing a version conflict with another GCloud library, that would be easy to resolve.

Jim Fulton added 12 commits June 3, 2021 08:47

Require latest api-core to get page_size parameter in HTTPIterator

a88a823

Added page_size for list_tables

422e880

Moved list_project tests to separate file and pytest

e77854b

Added page_size for list_projects

aa1696a

Moved list_datasets tests to separate file and pytest

daabfd7

Added page_size for list_datasets

0966b00

Added page_size for list_routines

19f6958

Added page_size for list_models

3ff1f3d

Moved list_jobs tests to separate file and pytest

e3d63b6

Added page_size for list_jobs

920d729

blacken/lint

3a1ff97

updated docs

a6558d8

product-auto-label Bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Jun 3, 2021

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 3, 2021

jimfulton marked this pull request as ready for review June 3, 2021 14:53

jimfulton requested review from a team and prash-mi and removed request for a team June 3, 2021 14:53

jimfulton mentioned this pull request Jun 3, 2021

dataset.list_tables max_results parameter googleapis/python-bigquery-sqlalchemy#173

Closed

plamut reviewed Jun 4, 2021

View reviewed changes

Comment thread tests/unit/test_list_projects.py

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

Jim Fulton added 2 commits June 4, 2021 11:45

Added missing license headers to new source file.

23bd498

Tightened some doc language.

eb986fb

plamut suggested changes Jun 5, 2021

View reviewed changes

removed duplicate license headers

a7a8182

plamut approved these changes Jun 5, 2021

View reviewed changes

jimfulton added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jun 5, 2021

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 5, 2021

jimfulton merged commit 1f1c4b7 into master Jun 6, 2021

jimfulton deleted the riversnake-page-size branch June 6, 2021 14:18

release-please Bot mentioned this pull request Jan 4, 2022

chore(main): release python-bigquery 1.27.1 #1097

Closed

feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter to control page size by jimfulton · Pull Request #686 · googleapis/python-bigquery · GitHub

Uh oh!

feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter to control page size - #686

feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter to control page size #686
jimfulton merged 15 commits into
masterfrom
riversnake-page-size

jimfulton commented Jun 3, 2021

Uh oh!

plamut left a comment

Uh oh!

Uh oh!

Uh oh!

jimfulton commented Jun 4, 2021

Uh oh!

tswast commented Jun 4, 2021

Uh oh!

plamut left a comment

Uh oh!

plamut Jun 5, 2021 •
edited
Loading

Uh oh!

Uh oh!

jimfulton Jun 5, 2021

Uh oh!

plamut commented Jun 5, 2021 •
edited
Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter to control page size by jimfulton · Pull Request #686 · googleapis/python-bigquery · GitHub

Uh oh!

feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter to control page size - #686

Conversation

jimfulton commented Jun 3, 2021

Uh oh!

plamut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jimfulton commented Jun 4, 2021

Uh oh!

tswast commented Jun 4, 2021

Uh oh!

plamut left a comment

Choose a reason for hiding this comment

Uh oh!

plamut Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimfulton Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

plamut commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

plamut Jun 5, 2021 •
edited
Loading

plamut commented Jun 5, 2021 •
edited
Loading