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

[rahul2393]

Fix: #13299

[gemini-code-assist]

Summary of Changes

Hello @rahul2393, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical issue in the Spanner client library's SelectAll function where decoding database rows into Go structs could result in incorrect value assignments if the column order from the database did not align with the field order in the Go struct, especially when spanner struct tags were used. The changes ensure that the library robustly handles such discrepancies, guaranteeing that data is accurately mapped to its intended struct fields, thereby improving the reliability of data retrieval operations.

Highlights

• Corrected SelectAll Decoding Logic: The SelectAll function now correctly maps Spanner column values to Go struct fields, even when the order of columns returned by the database differs from the order of fields in the Go struct, particularly when using spanner struct tags.
• Enhanced Field Mapping with indexPath: Introduced a new fieldInfo struct and indexPath tracking to accurately identify and set values for struct fields, including those within nested anonymous structs.
• Refactored Pointer Generation: The structPointers function has been updated to return not only the pointers to struct fields but also their corresponding indexPath and a lookup map, which are used to ensure values are scanned into the correct fields based on the database column order.
• Improved Test Coverage: New test cases have been added to spanner/row_test.go to specifically validate the correct decoding behavior when database column order does not match the struct field order.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a bug in SelectAll where Spanner columns were not being correctly mapped to struct fields when spanner tags were used and column order did not match struct field order. The changes introduce a name-based mapping mechanism that correctly handles out-of-order columns and embedded structs, which is a solid improvement. The new test cases are comprehensive and validate the fix effectively. My review includes one suggestion to remove a redundant block of code which, while not affecting correctness, impacts performance and maintainability.

[gemini-code-assist]

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.

[rahul2393]

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.

[rahul2393]

@gemini-code-assist

[gemini-code-assist]

Warning

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

[olavloite]

@gemini-code-assist

[gemini-code-assist]

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.

[rahul2393]

/gemini review

[gemini-code-assist]

Code Review

This pull request correctly fixes a bug in SelectAll where decoding Spanner rows into structs failed if the column order did not match the struct field order. The solution cleverly remaps pointers for each row based on column names, ensuring correct data placement. A significant improvement is the added support for decoding into embedded structs, which is enabled by tracking field index paths and using FieldByIndex. The new test cases are thorough and validate both the main fix and the new embedded struct functionality. I have a couple of suggestions to further refine the code by removing a redundant block and optimizing a reflection-based lookup.