Skip to content

fix(spanner): decoding spanner rows using SelectAll should map values in correct annotations #13301

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.

Sign up for GitHub

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
rahul2393 merged 7 commits into from
Nov 14, 2025
Merged

fix(spanner): decoding spanner rows using SelectAll should map values in correct annotations #13301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b34aeb
fix(spanner): decoding spanner rows using SelectAll should map values…
rahul2393 Nov 4, 2025
5cc0998
Merge branch 'main' into fix-13299
rahul2393 Nov 14, 2025
0df6d57
fix test
rahul2393 Nov 14, 2025
4a4f87b
incorporate gemini suggestions
rahul2393 Nov 14, 2025
50c9077
change order of fields in embedded struct
rahul2393 Nov 14, 2025
3839f22
add tag to embedded struct
rahul2393 Nov 14, 2025
226c765
incorporate more suggestions on improving test readability
rahul2393 Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 69 additions & 26 deletions spanner/row.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,9 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio

isPrimitive := itemType.Kind() != reflect.Struct
var pointers []interface{}
var pointerIndexes [][]int
var pointerLookup map[string]int
var orderedPointers []interface{}
isFirstRow := true
var err error
return rows.Do(func(row *Row) error {
Expand All @@ -469,9 +472,10 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio
defer func() {
isFirstRow = false
}()
if pointers, err = structPointers(sliceItem.Elem(), row.fields, s.Lenient); err != nil {
if pointers, pointerIndexes, pointerLookup, err = structPointers(sliceItem.Elem(), row.fields, s.Lenient); err != nil {
return err
}
orderedPointers = make([]interface{}, len(pointers))
}
defer func() {
for _, ptr := range pointers {
Expand All @@ -482,28 +486,50 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio
}
}
}()
if len(orderedPointers) != len(row.fields) {
orderedPointers = make([]interface{}, len(row.fields))
}
for i, col := range row.fields {
idx, ok := pointerLookup[strings.ToLower(col.GetName())]
if !ok {
if !s.Lenient {
return errNoOrDupGoField(sliceItem.Elem(), col.GetName())
}
orderedPointers[i] = nil
continue
}
orderedPointers[i] = pointers[idx]
}
} else if isPrimitive {
if len(row.fields) > 1 && !s.Lenient {
return errTooManyColumns()
}
pointers = []interface{}{sliceItem.Interface()}
}
if len(pointers) == 0 {
var rowPointers []interface{}
if !isPrimitive {
rowPointers = orderedPointers
} else {
rowPointers = pointers
}
if len(rowPointers) == 0 {
return nil
}
err = row.Columns(pointers...)
err = row.Columns(rowPointers...)
if err != nil {
return err
}
if !isPrimitive {
e := sliceItem.Elem()
idx := 0
for _, p := range pointers {
for i, p := range pointers {
if p == nil {
continue
}
e.Field(idx).Set(reflect.ValueOf(p).Elem())
idx++
indexPath := pointerIndexes[i]
if len(indexPath) == 0 {
continue
}
e.FieldByIndex(indexPath).Set(reflect.ValueOf(p).Elem())
}
}
Comment on lines 522 to 534
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Nov 4, 2025

Choose a reason for hiding this comment

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

high

This if !isPrimitive block appears to be redundant and can be removed. The row.Columns(rowPointers...) call on line 518 already decodes the row values directly into the correct struct fields via the orderedPointers slice.

The orderedPointers slice is correctly constructed for each row to map column names to the corresponding struct field pointers. Therefore, after row.Columns executes, the sliceItem struct is already correctly populated.

This loop then iterates through the pointers, reads the values that were just decoded into the struct fields, and writes them back to the exact same fields. This is effectively a no-op (field.Set(field.Value())) and adds unnecessary overhead for each row processed.

Removing this block will improve performance and simplify the code.

Copy link
Copy Markdown
Contributor Author

@rahul2393 rahul2393 Nov 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That block is still doing real work. The pointer reuse optimisation keeps a single slice of pointers that always references the struct value built during the first row. On subsequent rows we don’t rebuild those pointers—we simply reshuffle them (orderedPointers) and hand them to row.Columns. Because of that, every call to row.Columns writes into the first row’s struct, not the current sliceItem. The loop you highlighted copies the freshly decoded values from that shared pointer slice into the per-row sliceItem so we can append the correct struct to the destination.

If we drop the loop, only the first row would decode correctly. Later rows would append zero values (or stale data) because sliceItem never receives the decoded data. You can see this by temporarily removing the loop and rerunning TestSelectAll: the new out-of-order tag cases fail immediately.

So the block isn’t redundant—it’s required to make pointer reuse safe, while still benefiting from the optimisation.

Copy link
Copy Markdown
Contributor Author

@rahul2393 rahul2393 Nov 4, 2025

Choose a reason for hiding this comment

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

@gemini-code-assist

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Nov 4, 2025

Choose a reason for hiding this comment

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

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copy link
Copy Markdown
Contributor

@olavloite olavloite Nov 14, 2025

Choose a reason for hiding this comment

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

@gemini-code-assist

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Nov 14, 2025

Choose a reason for hiding this comment

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

Thank you, @rahul2393, for the detailed explanation. Your clarification regarding the pointer reuse optimization and how row.Columns writes into the first row's struct, necessitating this loop to copy values into the current sliceItem, is very clear. I understand now that this block is indeed crucial for correct decoding across all rows and is not redundant. My apologies for the initial misassessment.

I appreciate you taking the time to explain the underlying mechanism and the impact of removing the loop. This insight is valuable.

Comment thread
rahul2393 marked this conversation as resolved.
var elemVal reflect.Value
Expand All @@ -524,40 +550,53 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio
})
}

func structPointers(sliceItem reflect.Value, cols []*sppb.StructType_Field, lenient bool) ([]interface{}, error) {
type fieldInfo struct {
value reflect.Value
indexPath []int
}

func structPointers(sliceItem reflect.Value, cols []*sppb.StructType_Field, lenient bool) ([]interface{}, [][]int, map[string]int, error) {
pointers := make([]interface{}, 0, len(cols))
fieldTag := make(map[string]reflect.Value, len(cols))
initFieldTag(sliceItem, &fieldTag)
indexes := make([][]int, 0, len(cols))
fieldTag := make(map[string]fieldInfo, len(cols))
initFieldTag(sliceItem, &fieldTag, nil)
lookup := make(map[string]int, len(cols))

for i, colName := range cols {
if colName.Name == "" {
return nil, errColNotFound(fmt.Sprintf("column %d", i))
return nil, nil, nil, errColNotFound(fmt.Sprintf("column %d", i))
}

var fieldVal reflect.Value
if v, ok := fieldTag[strings.ToLower(colName.GetName())]; ok {
fieldVal = v
} else {
var info fieldInfo
var ok bool
if info, ok = fieldTag[strings.ToLower(colName.GetName())]; !ok {
if !lenient {
return nil, errNoOrDupGoField(sliceItem, colName.GetName())
return nil, nil, nil, errNoOrDupGoField(sliceItem, colName.GetName())
}
if structField, okField := sliceItem.Type().FieldByName(colName.GetName()); okField {
fieldVal := sliceItem.FieldByIndex(structField.Index)
if fieldVal.CanSet() {
info = fieldInfo{value: fieldVal, indexPath: append([]int(nil), structField.Index...)}
}
}
Comment thread
rahul2393 marked this conversation as resolved.
fieldVal = sliceItem.FieldByName(colName.GetName())
}
if !fieldVal.IsValid() || !fieldVal.CanSet() {
// have to add if we found a column because Columns() requires
// len(cols) arguments or it will error. This way we can scan to
// a useless pointer

if !info.value.IsValid() || !info.value.CanSet() {
pointers = append(pointers, nil)
indexes = append(indexes, nil)
lookup[strings.ToLower(colName.GetName())] = len(pointers) - 1
continue
}

pointers = append(pointers, fieldVal.Addr().Interface())
pointers = append(pointers, info.value.Addr().Interface())
indexes = append(indexes, info.indexPath)
lookup[strings.ToLower(colName.GetName())] = len(pointers) - 1
}
return pointers, nil
return pointers, indexes, lookup, nil
}

// Initialization the tags from struct.
func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]reflect.Value) {
func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]fieldInfo, path []int) {
typ := sliceItem.Type()

for i := 0; i < sliceItem.NumField(); i++ {
Expand All @@ -569,11 +608,12 @@ func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]reflect.Value
if !exported && !fieldType.Anonymous {
continue
}
currentPath := append(path, i)
if fieldType.Type.Kind() == reflect.Struct {
// found an embedded struct
if fieldType.Anonymous {
sliceItemOfAnonymous := sliceItem.Field(i)
initFieldTag(sliceItemOfAnonymous, fieldTagMap)
initFieldTag(sliceItemOfAnonymous, fieldTagMap, currentPath)
continue
}
}
Expand All @@ -584,6 +624,9 @@ func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]reflect.Value
if name == "" {
name = fieldType.Name
}
(*fieldTagMap)[strings.ToLower(name)] = sliceItem.Field(i)
(*fieldTagMap)[strings.ToLower(name)] = fieldInfo{
value: sliceItem.Field(i),
indexPath: append([]int(nil), currentPath...),
}
}
}
123 changes: 110 additions & 13 deletions spanner/row_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2289,10 +2289,16 @@ func TestSelectAll(t *testing.T) {
Col4 time.Time `spanner:"TAG4"`
}

type testStructWithSimpleTag struct {
Col1 int64 `spanner:"tag1"`
Col2 int64 `spanner:"tag2"`
Col3 int64 `spanner:"tag3"`
Col4 int64 `spanner:"tag4"`
}
type Address struct {
Street string
ZipCode string
City string
AddressZip string `spanner:"ZipCode"`
AddressStreet string `spanner:"Street"`
AddressCity string `spanner:"City"`
}

type Person struct {
Expand Down Expand Up @@ -2542,6 +2548,78 @@ func TestSelectAll(t *testing.T) {
{Col1: 2, Col2: 2.2, Col3: "value2", Col4: tm.Add(24 * time.Hour)},
},
},
{
name: "success: using slice of structs with simple spanner tag annotations in different order",
args: args{
destination: &[]testStructWithSimpleTag{},
mock: newMockIterator(
&Row{
[]*sppb.StructType_Field{
{Name: "Tag2", Type: intType()},
{Name: "Tag1", Type: intType()},
{Name: "Tag4", Type: intType()},
{Name: "Tag3", Type: intType()},
},
[]*proto3.Value{intProto(2), intProto(1), intProto(4), intProto(3)},
},
&Row{
[]*sppb.StructType_Field{
{Name: "Tag1", Type: intType()},
{Name: "Tag2", Type: intType()},
{Name: "Tag3", Type: intType()},
{Name: "Tag4", Type: intType()},
},
[]*proto3.Value{intProto(1), intProto(2), intProto(3), intProto(4)},
},
&Row{
[]*sppb.StructType_Field{
{Name: "Tag2", Type: intType()},
{Name: "Tag1", Type: intType()},
{Name: "Tag4", Type: intType()},
{Name: "Tag3", Type: intType()},
},
[]*proto3.Value{intProto(2), intProto(1), intProto(4), intProto(3)},
},
iterator.Done,
),
},
want: &[]testStructWithSimpleTag{
{Col1: 1, Col2: 2, Col3: 3, Col4: 4},
{Col1: 1, Col2: 2, Col3: 3, Col4: 4},
{Col1: 1, Col2: 2, Col3: 3, Col4: 4},
},
},
{
name: "success: using slice of structs with spanner tag annotations in different order",
args: args{
destination: &[]testStructWithTag{},
mock: newMockIterator(
&Row{
[]*sppb.StructType_Field{
{Name: "Tag2", Type: floatType()},
{Name: "Tag1", Type: intType()},
{Name: "Tag4", Type: timeType()},
{Name: "Tag3", Type: stringType()},
},
[]*proto3.Value{floatProto(1.1), intProto(1), timeProto(tm), stringProto("value")},
},
&Row{
[]*sppb.StructType_Field{
{Name: "Tag2", Type: floatType()},
{Name: "Tag1", Type: intType()},
{Name: "Tag4", Type: timeType()},
{Name: "Tag3", Type: stringType()},
},
[]*proto3.Value{floatProto(2.2), intProto(2), timeProto(tm.Add(24 * time.Hour)), stringProto("value2")},
},
iterator.Done,
),
},
want: &[]testStructWithTag{
{Col1: 1, Col2: 1.1, Col3: "value", Col4: tm},
{Col1: 2, Col2: 2.2, Col3: "value2", Col4: tm.Add(24 * time.Hour)},
},
},
{
name: "failure: in case of error destination will have the partial result",
args: args{
Expand Down Expand Up @@ -2675,46 +2753,65 @@ func TestSelectAll(t *testing.T) {
wantErr: true,
},
{
name: "failure: nested unnamed structs",
name: "success: nested unnamed structs",
args: args{
destination: &[]*PersonEmbedded{},
mock: newMockIterator(
&Row{
[]*sppb.StructType_Field{
{Name: "Name", Type: stringType()},
{Name: "Street", Type: stringType()},
{Name: "ZipCode", Type: stringType()},
{Name: "City", Type: stringType()},
{Name: "Street", Type: stringType()},
{Name: "BirthDate", Type: dateType()},
{Name: "ZipCode", Type: stringType()},
},
[]*proto3.Value{
stringProto("Name1"),
stringProto("Street1"),
stringProto("ZipCode1"),
stringProto("City1"),
stringProto("Street1"),
dateProto(civil.Date{Year: 2000, Month: 11, Day: 14}),
stringProto("ZipCode1"),
},
},
&Row{
[]*sppb.StructType_Field{
{Name: "Name", Type: stringType()},
{Name: "Street", Type: stringType()},
{Name: "ZipCode", Type: stringType()},
{Name: "City", Type: stringType()},
{Name: "Street", Type: stringType()},
{Name: "BirthDate", Type: dateType()},
{Name: "ZipCode", Type: stringType()},
},
[]*proto3.Value{
stringProto("Name2"),
stringProto("Street2"),
stringProto("ZipCode2"),
stringProto("City2"),
stringProto("Street2"),
dateProto(civil.Date{Year: 2001, Month: 11, Day: 14}),
stringProto("ZipCode2"),
},
},
iterator.Done,
),
},
wantPanic: true,
want: &[]*PersonEmbedded{
{
Name: "Name1",
Address: Address{
AddressZip: "ZipCode1",
AddressStreet: "Street1",
AddressCity: "City1",
},
BirthDate: civil.Date{Year: 2000, Month: 11, Day: 14},
},
{
Name: "Name2",
Address: Address{
AddressZip: "ZipCode2",
AddressStreet: "Street2",
AddressCity: "City2",
},
BirthDate: civil.Date{Year: 2001, Month: 11, Day: 14},
},
},
},
}
for _, tt := range tests {
Expand Down
Loading