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

@jimfulton jimfulton commented Jun 3, 2021

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

@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 label Jun 3, 2021
@jimfulton jimfulton marked this pull request as ready for review Jun 3, 2021
@jimfulton jimfulton requested review from as code owners Jun 3, 2021
@jimfulton jimfulton requested review from prash-mi and removed request for Jun 3, 2021
Copy link
Contributor

@plamut plamut left a comment

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?

Loading

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

@jimfulton jimfulton commented Jun 4, 2021

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!!!

Loading

@tswast
Copy link
Contributor

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

Loading

Copy link
Contributor

@plamut plamut left a comment

A license header is duplicated, but otherwise looks good.

Loading

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

Loading

Copy link
Contributor Author

@jimfulton jimfulton Jun 5, 2021

Choose a reason for hiding this comment

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

Fixed

Loading

@plamut
Copy link
Contributor

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

Loading

plamut
plamut approved these changes Jun 5, 2021
@jimfulton jimfulton merged commit 1f1c4b7 into master Jun 6, 2021
12 checks passed
Loading
@jimfulton jimfulton deleted the riversnake-page-size branch Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants