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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from Jun 6, 2021

Conversation

jimfulton
Copy link
Contributor

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 馃

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Jun 3, 2021
@jimfulton jimfulton changed the title feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter tp control page size feat: list_tables, list_projects, list_datasets, list_models, list_routines, and list_jobs now accept a page_size parameter to control page size Jun 3, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 3, 2021
@jimfulton jimfulton marked this pull request as ready for review June 3, 2021 14:53
@jimfulton jimfulton requested a review from a team June 3, 2021 14:53
@jimfulton jimfulton requested a review from a team as a code owner June 3, 2021 14:53
@jimfulton jimfulton requested review from prash-mi and removed request for a team June 3, 2021 14:53
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

tests/unit/test_list_projects.py Show resolved Hide resolved
google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
@jimfulton
Copy link
Contributor Author

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
Copy link
Contributor

tswast commented Jun 4, 2021

@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.

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A license header is duplicated, but otherwise looks good.

Comment on lines 14 to 26
# 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.
Copy link
Contributor

@plamut plamut Jun 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@plamut
Copy link
Contributor

plamut commented Jun 5, 2021

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.

@jimfulton 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 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 jimfulton merged commit 1f1c4b7 into master Jun 6, 2021
@jimfulton jimfulton deleted the riversnake-page-size branch June 6, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control iterator page size for list_tables.
4 participants