Skip to content

fix(spanner): row mismatch in SelectAll using custom type #12222

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

Merged
merged 4 commits into from
May 9, 2025

Conversation

sakthivelmanii
Copy link
Contributor

Fixes #11101

Why is it an issue?
We are creating a new pointers for each columns only once while decoding the first row and reusing it in subsequent rows. In the decoding, if customer didn't set certain values, it's picking up values from previous column since pointers always same when DecodeSpanner is called. When we try to call DecodeSpanner, we need to make sure we have to use new memory instead of reusing it.

@sakthivelmanii sakthivelmanii requested review from a team as code owners May 8, 2025 09:46
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label May 8, 2025
@@ -464,14 +464,14 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio
var err error
return rows.Do(func(row *Row) error {
sliceItem := reflect.New(itemType)
if isFirstRow && !isPrimitive {
if !isPrimitive {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand it correctly that we could, instead of the current change, also do the following:

  1. Keep the if statements as is.
  2. Add a defer func() { ... set all pointers to nil ...} to this function.

That would mean that we are re-using the slice of pointers, but they are reset to nil for each new row. Would that be a slightly more efficient solution?

If not, could we otherwise just move the entire declaration of var pointers into this function, as I dont' think we are using it outside of the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think re-using the slice of pointers will be more optimal here and that was why we initially added this

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do benchmark test also like here
#9206

To confirm we are not adding regression

@sakthivelmanii
Copy link
Contributor Author

@rahul2393 @olavloite I changed the approach and set the value using reflection instead of re-initializing memory.

Benchmark results

goos: darwin
goarch: arm64
pkg: cloud.google.com/go/spanner
cpu: Apple M3 Pro
                             │ 10_runs_bench_using_select_all_main.txt │ 10_runs_bench_using_select_all.txt  │
                             │                 sec/op                  │   sec/op     vs base                │
Scan100RowsUsingSelectAll-12                               11.72µ ± 1%   14.78µ ± 0%  +26.06% (p=0.000 n=20)

                             │ 10_runs_bench_using_select_all_main.txt │ 10_runs_bench_using_select_all.txt │
                             │                  B/op                   │      B/op       vs base            │
Scan100RowsUsingSelectAll-12                              12.30Ki ± 0%     12.30Ki ± 0%  ~ (p=1.000 n=20) ¹
¹ all samples are equal

                             │ 10_runs_bench_using_select_all_main.txt │ 10_runs_bench_using_select_all.txt │
                             │                allocs/op                │   allocs/op     vs base            │
Scan100RowsUsingSelectAll-12                                222.0 ± 0%       222.0 ± 0%  ~ (p=1.000 n=20) ¹
¹ all samples are equal

@rahul2393 rahul2393 enabled auto-merge (squash) May 9, 2025 05:21
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2025
@rahul2393 rahul2393 merged commit ce6a23a into main May 9, 2025
9 checks passed
@rahul2393 rahul2393 deleted the fix_row_mismatch branch May 9, 2025 05:34
2FaceS-bit pushed a commit to 2FaceS-bit/google-cloud-go that referenced this pull request May 12, 2025
…#12222)

* fix(spanner): row mismatch in SelectAll using custom type

* change else if to else

* update approach to improve performance
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.

spanner: possible row mismatch in SelectAll using custom type
4 participants