Skip to content

Conversation

@andrewro-google
Copy link
Contributor

@andrewro-google andrewro-google commented Aug 29, 2025

Tested by running multiple_rows_cpu_benchmark --project=${GOOGLE_CLOUD_PROJECT} --instance=${GOOGLE_CLOUD_CPP_SPANNER_TEST_INSTANCE_ID} --table-size=1000000 --maximum-channels=8 --maximum-threads=16 --iteration-duration=5 --samples=60 --experiment=select-string --use-only-clients=true --query-size=100000 (larger --query-size than in the README). The data is noisy but seems like a ~20% win on CpuTime. I don't expect this to help much with queries that return few results. For the use case I care about (fetching a whole table), we were limited by CPU when using all the threads on a machine.

improvement

We still have a copy when generating Row in PartialResultSetSource::NextRow() but we'd need a new API to avoid that.


This change is Reviewable

@andrewro-google andrewro-google requested a review from a team as a code owner August 29, 2025 20:40
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Aug 29, 2025
@andrewro-google
Copy link
Contributor Author

@scotthart could you take a look / do the gcbrun incantations

@scotthart
Copy link
Member

/gcbrun

@codecov
Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 99.67105% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.04%. Comparing base (612245c) to head (5df9ce8).
⚠️ Report is 55 commits behind head on main.

Files with missing lines Patch % Lines
...loud/spanner/internal/partial_result_set_resume.cc 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15441   +/-   ##
=======================================
  Coverage   93.04%   93.04%           
=======================================
  Files        2403     2403           
  Lines      219553   219644   +91     
=======================================
+ Hits       204273   204368   +95     
+ Misses      15280    15276    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andrewro-google
Copy link
Contributor Author

I updated the code to fix the clang tidy warning

@scotthart
Copy link
Member

/gcbrun

@diegomarquezp
Copy link
Collaborator

diegomarquezp commented Sep 8, 2025

We should merge main, which brings a fix for the failing MacOS test: #15448

@andrewro-google
Copy link
Contributor Author

Done, thanks

Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

@scotthart reviewed 11 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrewro-google)

@scotthart
Copy link
Member

/gcbrun

@scotthart
Copy link
Member

/gcbrun

@scotthart scotthart merged commit 1cbaa96 into googleapis:main Sep 12, 2025
70 of 79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants