Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the handling of ARROW query results in query_and_wait by requesting zero rows during wait operations (since getQueryResults only returns STRUCT_ENCODING rows) and preserving the first page response for ARROW queries. It also introduces a helper function has_more_rows to check if rows remain by comparing pageRowCount and totalRows. The review feedback recommends making has_more_rows more robust by wrapping the integer conversions in a try-except block to safely handle potential TypeError or ValueError exceptions from malformed API responses.
| 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)) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
6ba439e to
ab029f6
Compare
ab029f6 to
c8fab58
Compare
When an ARROW query's results didn't fit in the jobs.query response, QueryJob.result() dropped that response, because it only kept first pages that had STRUCT_ENCODING rows. The stream then reread every row from offset 0. While waiting for the job, query_and_wait() also passed page_size to jobs.getQueryResults, which returned STRUCT_ENCODING rows that ARROW results never use. For ARROW queries, decide whether rows remain by comparing pageRowCount with totalRows instead of checking for a pageToken, wait with page_size=0 so jobs.getQueryResults is called with maxResults=0, and keep a jobs.query response that has arrowRecordBatch as the first page. RowIterator then returns the first page and streams the rest from the next row, also when it's read again. Queries that don't use ARROW are unchanged.
c8fab58 to
7208b63
Compare
Keeps the
jobs.queryfirst page of Arrow results and streams only the rest, instead of dropping it and rereading every row.pageRowCountandtotalRowsto decide whether more rows remain.maxResults=0, so no unused JSON rows are fetched.QueryJob.result()keeps an Arrow first page, and reading the results twice gives the same rows.