Skip to content

fix(bigquery): totalRows returned by fetchCachedPage #11874

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zecke
Copy link

@zecke zecke commented Mar 19, 2025

The RowIterator.TotalRows is wrong when hitting the cached page on the fast path (e.g. not changing start index or updating the page info).

fixes: #11873

@zecke zecke requested review from a team as code owners March 19, 2025 14:24
@zecke zecke requested a review from Neenu1995 March 19, 2025 14:24
@zecke zecke force-pushed the zecke-fix-totalrows branch from 44cd5f2 to 22dfd2b Compare March 19, 2025 14:25
The RowIterator.TotalRows is wrong when hitting the cached page on the
fast path (e.g. not changing start index or updating the page info).

fixes: googleapis#11873
@zecke zecke force-pushed the zecke-fix-totalrows branch from 22dfd2b to af6b753 Compare March 19, 2025 14:26
@alvarowolfx alvarowolfx requested review from shollyman and alvarowolfx and removed request for Neenu1995 March 31, 2025 19:38
@alvarowolfx alvarowolfx changed the title fix(bigquery) Fix totalRows returned by fetchCachedPage fix(bigquery): totalRows returned by fetchCachedPage Apr 1, 2025
@alvarowolfx alvarowolfx added the api: bigquery Issues related to the BigQuery API. label Apr 7, 2025
Copy link
Contributor

@alvarowolfx alvarowolfx left a comment

Choose a reason for hiding this comment

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

Overall looks good, but can you add an integration test that shows that issue ? It happens when running two queries in sequence ?

@zecke
Copy link
Author

zecke commented Apr 9, 2025

Overall looks good, but can you add an integration test that shows that issue ? It happens when running two queries in sequence ?

Sure. Do you have any indication of the cost of running the BigQuery integration_test.go?

@alvarowolfx
Copy link
Contributor

Overall looks good, but can you add an integration test that shows that issue ? It happens when running two queries in sequence ?

Sure. Do you have any indication of the cost of running the BigQuery integration_test.go?

I don't recommended running the full suite, you can run just one test with a command like: go test -run ^TestIntegration_StorageReadClientProject -v. Not sure about the costs of running the full suite, but running just a few tests should be under the free tier.

I can also take the work over and write the tests for it, so I can run the integration tests with my corp account here. Let me know what works best for you.

@zecke
Copy link
Author

zecke commented Apr 12, 2025

I can also take the work over and write the tests for it, so I can run the integration tests with my corp account here. Let me know what works best for you.

Thanks for the offer. Please take this over and write the test/make changes as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bigquery: total rows has incorrect value
2 participants