Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from __future__ import annotations

import base64
from typing import Dict, Iterator, Optional, TYPE_CHECKING
from typing import Any, Dict, Iterator, Optional, TYPE_CHECKING

from google.cloud.bigquery import _versions_helpers
from google.cloud.bigquery import enums
Expand Down Expand Up @@ -113,6 +113,16 @@ def serialization_options(
}


def has_more_rows(response: Dict[str, Any]) -> bool:
"""Return True if rows remain after an ``ARROW`` ``jobs.query`` response.

``ARROW`` responses say how many rows the first page holds in
``pageRowCount``, so compare that with ``totalRows`` instead of checking
for a ``pageToken``.
"""
return int(response.get("pageRowCount", 0)) < int(response.get("totalRows", 0))
Comment on lines +116 to +123

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation of has_more_rows directly calls int() on the result of response.get(). If pageRowCount or totalRows is present in the response but has a None value (or is an empty/invalid string), this will raise a TypeError or ValueError. Using a safer parsing approach with a try-except block ensures the function is robust against unexpected or malformed API responses.

Suggested change
def has_more_rows(response: Dict[str, Any]) -> bool:
"""Return True if rows remain after an ``ARROW`` ``jobs.query`` response.
``ARROW`` responses say how many rows the first page holds in
``pageRowCount``, so compare that with ``totalRows`` instead of checking
for a ``pageToken``.
"""
return int(response.get("pageRowCount", 0)) < int(response.get("totalRows", 0))
def has_more_rows(response: Dict[str, Any]) -> bool:
"""Return True if rows remain after an ``ARROW`` ``jobs.query`` response.
``ARROW`` responses say how many rows the first page holds in
``pageRowCount``, so compare that with ``totalRows`` instead of checking
for a ``pageToken``.
"""
try:
page_row_count = int(response.get("pageRowCount") or 0)
total_rows = int(response.get("totalRows") or 0)
except (ValueError, TypeError):
return False
return page_row_count < total_rows

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pageRowCount and totalRows come from the jobs.query response, where they're always integer strings when present, and the existing code in this module already reads totalRows with int(). I'd rather not add handling for responses the API doesn't send.



def _read_rows_options(options: Optional[Dict[str, str]]) -> Dict[str, str]:
"""Convert ``jobs.query`` ``arrowSerializationOptions`` to the
``ArrowSerializationOptions`` fields that ``ReadRows`` takes."""
Expand Down Expand Up @@ -153,7 +163,6 @@ def download_arrow_from_job(
# Step 2: Process initial inline Arrow response from jobs.query if available.
if row_iterator._first_page_response:
first_page = row_iterator._first_page_response
row_iterator._first_page_response = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this cause problems for non-arrow responses? IIRC, this is here to make sure we don't end up in an infinite loop of looking at the first page.


job_complete = bool(first_page.get("jobComplete", False))
if job_complete:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,10 @@ def query_and_wait(
)
request_body["arrowSerializationOptions"] = arrow_serialization_options

# jobs.getQueryResults only returns STRUCT_ENCODING rows, so while waiting
# for an ARROW query, request none of them.
wait_page_size = 0 if arrow_serialization_options is not None else page_size

# Some API parameters aren't supported by the jobs.query API. In these
# cases, fallback to a jobs.insert call.
if not _supported_by_jobs_query(request_body):
Expand All @@ -553,7 +557,7 @@ def query_and_wait(
api_timeout=api_timeout,
wait_timeout=wait_timeout,
retry=retry,
page_size=page_size,
page_size=wait_page_size,
max_results=max_results,
query_results_format=query_results_format,
arrow_serialization_options=arrow_serialization_options,
Expand Down Expand Up @@ -616,6 +620,8 @@ def do_query():
)
page_token = query_results.page_token
more_pages = page_token is not None
if arrow_serialization_options is not None:
more_pages = _arrow_query.has_more_rows(response)

if more_pages or not query_results.complete:
# TODO(swast): Avoid a call to jobs.get in some cases (few
Expand All @@ -627,7 +633,7 @@ def do_query():
api_timeout=api_timeout,
wait_timeout=wait_timeout,
retry=retry,
page_size=page_size,
page_size=wait_page_size,
max_results=max_results,
query_results_format=query_results_format,
arrow_serialization_options=arrow_serialization_options,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1842,9 +1842,13 @@ def is_job_done():
# RowIterator response if there are any rows in it. This prevents us
# from stopping the iteration early in the cases where we set
# maxResults=0. In that case, we're missing rows and there's no next
# page token.
# page token. ARROW responses from jobs.query carry their rows in
# arrowRecordBatch instead of rows.
first_page_response = self._query_results._properties
if "rows" not in first_page_response:
if (
"rows" not in first_page_response
and "arrowRecordBatch" not in first_page_response
):
first_page_response = None

rows = self._client._list_rows_from_query_results(
Expand Down
Loading
Loading