Skip to content

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

Merged
jinseopkim0 merged 1 commit into
mainfrom
fix-arrow-query-page-token
Sep 21, 2026
Merged

jinseopkim0 merged 1 commit into
mainfrom
fix-arrow-query-page-token

Conversation

@jinseopkim0

@jinseopkim0 jinseopkim0 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist review

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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
jinseopkim0 force-pushed the fix-arrow-query-page-token branch from ad735c7 to ac610fd Compare September 21, 2026 19:02
@jinseopkim0

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist review

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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
jinseopkim0 force-pushed the fix-arrow-query-page-token branch from ac610fd to 3bbcbae Compare September 21, 2026 19:04
@jinseopkim0

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist review

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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
jinseopkim0 force-pushed the fix-arrow-query-page-token branch from 3bbcbae to 61d11e7 Compare September 21, 2026 19:07
@jinseopkim0

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist review

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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)

high

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

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist review

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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 gemini-code-assist Bot left a comment

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.

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.

@jinseopkim0
jinseopkim0 force-pushed the fix-arrow-query-page-token branch from 61d11e7 to 7e9d8a3 Compare September 21, 2026 20:06
@jinseopkim0
jinseopkim0 marked this pull request as ready for review September 21, 2026 20:10
@jinseopkim0
jinseopkim0 requested review from a team as code owners September 21, 2026 20:10
@jinseopkim0
jinseopkim0 merged commit 9d10dd0 into main Sep 21, 2026
209 checks passed
@jinseopkim0
jinseopkim0 deleted the fix-arrow-query-page-token branch September 21, 2026 21:04
blakeli0 pushed a commit that referenced this pull request Sep 23, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>1.92.0</summary>

##
[1.92.0](v1.91.0...v1.92.0)
(2026-09-23)


### Features

* **bigquery-jdbc:** add `EnableTimestampPicos` connection property and
its plumbing
([#14284](#14284))
([b4aa5ac](b4aa5ac))
* **bigquery-jdbc:** implement picosecond temporal math and formatting
engine
([#14286](#14286))
([2a9612a](2a9612a))
* **bigquery-jdbc:** support picosecond in REST JSON path and nested
types
([#14334](#14334))
([15ffe4a](15ffe4a))
* **bigquery-jdbc:** support picosecond in `PreparedStatement`
parameters and batching
([#14373](#14373))
([c1aac66](c1aac66))
* **bigquery-jdbc:** support picosecond timestamp in `ResultSetMetaData`
and `DatabaseMetaData`
([#14358](#14358))
([43acdd3](43acdd3))
* **bigquery-jdbc:** support picosecond timestamps in Arrow Storage Read
API and nested types
([#14332](#14332))
([b5d9aca](b5d9aca))
* **bigquery-jdbc:** support qualified project delimiter in
`DefaultDataset` property
([#14240](#14240))
([6e8d6c8](6e8d6c8))
* **bigquery:** accelerate row-based query() with Arrow wire format
([#14405](#14405))
([8d12a8f](8d12a8f))
* **bigquery:** add ArrowDeserializer helper utility
([#13943](#13943))
([d9a298b](d9a298b))
* **bigquery:** add ArrowQueryPageFetcher for Arrow query result
pagination
([#14404](#14404))
([615409f](615409f))
* **bigquery:** add ArrowQueryResult and ArrowQueryResultImpl for Arrow
result streaming
([#13944](#13944))
([a62fdf8](a62fdf8))
* **bigquery:** add Storage Read API slow-path fallback for row-based
query()
([#14409](#14409))
([26e568a](26e568a))
* **bigquery:** add zero-copy queryArrow API for Arrow VectorSchemaRoot
streaming
([#14402](#14402))
([b44ffe8](b44ffe8))
* **bigquery:** make BigQuery AutoCloseable with default no-op close
method
([#14434](#14434))
([00bf3de](00bf3de))
* **firestore:** add support for BSON types
([#13189](#13189))
([8a123d9](8a123d9))
* **gax:** add ApiCallContext and request-level settings overloads to
ResumableUploadCallable
([#14251](#14251))
([e8cbd42](e8cbd42))
* **gax:** add globalTimeout settings field to
ResumableUploadCallSettings
([#14253](#14253))
([438cda6](438cda6))
* **gax:** add resumable upload error classification and retry algorithm
([#14419](#14419))
([b70396d](b70396d))
* **gax:** add ResumableUploadCallable creation to Callables and
HttpJsonCallableFactory
([#14242](#14242))
([7de24de](7de24de))
* **gax:** implement baseline Callable and Future for resumable uploads
([#14241](#14241))
([5a54db9](5a54db9))
* **generator:** add model flag and allowlist parser for resumable
upload RPCs
([#14317](#14317))
([acc1856](acc1856))
* **generator:** emit resumable upload client surface
([#14319](#14319))
([a9fed00](a9fed00))
* **generator:** emit resumable upload settings and HttpJson upload stub
([#14321](#14321))
([c122474](c122474))
* **generator:** enable resumable upload generation for showcase
([#14325](#14325))
([f9ebd79](f9ebd79))
* **generator:** switch resumable upload specialized stubs to package
private
([#14471](#14471))
([0d4e875](0d4e875))
* **generator:** wire transport stub delegation to resumable upload
stubs
([#14322](#14322))
([cc4b980](cc4b980))
* **google/cloud/backupdr/v1beta:** add backupdr
([#14410](#14410))
([a4a47da](a4a47da))
* **google/cloud/networkservices/v1beta1:** add networkservices
([#14407](#14407))
([21c4955](21c4955))
* **pubsub:** add publish telemetry headers for publish attempt
observability
([#14338](#14338))
([c167ab8](c167ab8))
* **pubsub:** implement publish hedging to reduce tail latency
([#13735](#13735))
([b302615](b302615))
* **spanner:** Support dynamic TLS certificate and key rotation for
Spanner Omni
([#14456](#14456))
([ffc745c](ffc745c))
* **storage/control:** add delete folder recursive sample
([#13642](#13642))
([f4b1b46](f4b1b46))
* **storage/control:** add delete folder recursive sample
([#14397](#14397))
([2c01d55](2c01d55))


### Bug Fixes

* **auth:** restore transportFactory upon deserialization in
InternalAwsSecurityCredentialsSupplier
([#14340](#14340))
([beea42f](beea42f))
* **bigquery-jdbc:** ensure row ordering in PCNT IT
([#14330](#14330))
([a16f048](a16f048))
* **bigquery-jdbc:** fix htapi fallback due to permission logic
([#14418](#14418))
([21e6dc8](21e6dc8))
* **bigquery-jdbc:** fix Timestamp assertions
([#14290](#14290))
([533ba14](533ba14))
* **bigquery-jdbc:** handle null parameters in Storage Write API bulk
inserts
([#14270](#14270))
([dd2c41a](dd2c41a)),
refs
[#14066](#14066)
* **bigquery-jdbc:** handle SQL NULLs in ResultSet primitive getters
([#14383](#14383))
([8e464fe](8e464fe)),
refs
[#14371](#14371)
* **bigquery:** default Arrow pagination stream location to US instead
of global
([#14458](#14458))
([2775eb1](2775eb1))
* **bigquery:** preserve page token and paginate correctly in Arrow
query when maxResults is set
([#14469](#14469))
([f5601f4](f5601f4))
* **bigquery:** use first page row count for Arrow query pagination
offset
([#14466](#14466))
([9d10dd0](9d10dd0))
* **bigtable:** don't notify config listeners while holding the manager
lock
([#14294](#14294))
([4426ccd](4426ccd))
* **bigtable:** fall back to classic path when per-RPC CallCredentials
are set on session path
([#14477](#14477))
([57bacb0](57bacb0))
* **bigtable:** fix abnormal session closures and scale-up in session
pool
([#14431](#14431))
([6361ecd](6361ecd))
* **biqguery:** fix undeclared QueryParameter wiring in QueryStatistics
([#14401](#14401))
([64cf1d3](64cf1d3))
* **bom:** restore google-cloud-spanner-jdbc to libraries-bom
([#14362](#14362))
([bc7be5e](bc7be5e)),
refs
[#14347](#14347)
* **spanner:** honor maxAttempts and totalTimeout in streaming resume
loop
([#14370](#14370))
([305f47d](305f47d))
* **spanner:** only set snapshot isolation read timestamp for SI or
optimistic txns in CloudClientExecutor
([#14346](#14346))
([54c0d0f](54c0d0f))
* **spanner:** prevent statement cancellation race in
AbstractBaseUnitOfWork
([#14283](#14283))
([d9a8eef](d9a8eef))
* **spanner:** re-enable ITInstanceAdminTest on cloud-devel and
cloud-staging
([#14281](#14281))
([89a8268](89a8268))


### Performance Improvements

* **spanner:** stop re-parsing the request id on every RPC
([#14353](#14353))
([46108f4](46108f4))


### Documentation

* Add a Http/Json Post-Quantum Cryptography Guide
([#13963](#13963))
([fcc65b0](fcc65b0))
* **bigquery:** add QueryArrow code sample and document JDK 17+ JVM
requirements
([#14437](#14437))
([bd363f6](bd363f6))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants