-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
@@ -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 { |
There was a problem hiding this comment.
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:
- Keep the if statements as is.
- 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@rahul2393 @olavloite I changed the approach and set the value using reflection instead of re-initializing memory. Benchmark results
|
…#12222) * fix(spanner): row mismatch in SelectAll using custom type * change else if to else * update approach to improve performance
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.