fix(bigquery): use first page row count for Arrow query pagination offset

[jinseopkim0]

This PR fixes Arrow query pagination by deriving the initial Storage Read API row offset from the number of rows returned in the first page rather than attempting to parse the REST API's opaque page token as a number.

getPageToken() returns a string, see https://docs.cloud.google.com/bigquery/docs/reference/rest/v2/jobs/query#response-body.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request removes the logic that parses the page token as a numeric row offset for Arrow query pagination, allowing opaque page tokens to be handled correctly. Instead, initialRowOffset is initialized to the size of the first page rows. The tests have been updated to verify that opaque page tokens are successfully processed rather than throwing an exception. Feedback suggests simplifying the condition checking content.getMaxResults() because initialRowOffset is now identical to firstPageRows.size(), making the second part of the OR condition redundant.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request removes the logic that parses the BigQuery page token as a numeric row offset for Arrow query pagination, instead initializing the initial row offset directly to the size of the first page's rows. This change allows the SDK to handle opaque page tokens, and the tests have been updated to reflect this behavior. Feedback on the changes suggests simplifying a redundant conditional check in BigQueryImpl.java where initialRowOffset and firstPageRows.size() are compared against content.getMaxResults(), as they now hold the same value.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request modifies the Arrow query pagination logic in BigQueryImpl to handle opaque (non-numeric) page tokens instead of throwing an exception, updating the corresponding tests to verify this behavior. However, the reviewer identified a logical issue where only parsing the page token when initialRowOffset is zero ignores the token when firstPageRows contains elements, which can lead to incorrect offsets and duplicate or incorrect data. The reviewer suggests always attempting to parse the page token first and falling back to the size of firstPageRows if parsing fails.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request updates Arrow query pagination to support opaque (non-numeric) page tokens rather than throwing an exception. However, the changes introduce a logical bug where valid numeric page tokens are ignored on subsequent pages if firstPageRows is not empty, as the token is only parsed when initialRowOffset is zero. The reviewer suggests parsing the page token first and falling back to the page size only if the token is non-numeric.

I am having trouble creating individual review comments. Click here to see my feedback.

java-bigquery/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQueryImpl.java (2623-2634)

By unconditionally initializing initialRowOffset to firstPageRows.size() and only attempting to parse the page token if initialRowOffset == 0L, any valid numeric page token will be ignored on subsequent pages where firstPageRows is not empty. This will cause pagination to incorrectly reuse the current page's size as the offset instead of the cumulative offset represented by the page token, leading to duplicate rows or incorrect data retrieval.

We can resolve this issue by attempting to parse the page token first, and falling back to firstPageRows.size() only if the token is non-numeric (opaque). A null check on firstPageRows is redundant here as upstream callers should return empty collections instead of null.

    long initialRowOffset = (long) firstPageRows.size();
    if (hasMorePages) {
      Long parsedOffset = Longs.tryParse(results.getPageToken());
      if (parsedOffset != null) {
        initialRowOffset = parsedOffset;
      }
      if (content.getMaxResults() != null && initialRowOffset >= content.getMaxResults()) {
        hasMorePages = false;
      }
    }

References

1. When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.
2. Prefer returning empty objects (such as an empty ErrorDetails or empty collections) instead of null to minimize null pointer risks.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request updates the Arrow query pagination logic in BigQueryImpl to support opaque (non-numeric) page tokens. Instead of throwing a BigQueryException when a page token cannot be parsed as a numeric offset, the code now falls back to using the size of the first page's rows as the initial row offset. Corresponding unit tests have been updated and added to verify behavior with both opaque and numeric page tokens. I have no feedback to provide as the changes are well-implemented and properly tested.

[gemini-code-assist]

Code Review

This pull request updates the pagination logic for Arrow query results in BigQueryImpl to support opaque (non-numeric) page tokens. Instead of throwing a BigQueryException when a page token cannot be parsed as a numeric row offset, the code now gracefully handles it by defaulting the initial row offset to the size of the first page's rows. Corresponding unit tests have been updated and added to verify behavior with both opaque and numeric page tokens. There are no review comments, and I have no additional feedback to provide.