fix(spannerdriver): populate QueryStats in ResultSetStats - #778
Conversation
The last PartialResultSet carries Stats.QueryStats; RowIterator already exposes it as QueryStats. Populate ResultSetStats.QueryStats from that map so database/sql callers receive query_stats (e.g. elapsed_time).
09ab721 to
47245ca
Compare
![gemini-code-assist[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates transaction.go to populate ResultSetStats.QueryStats from the RowIterator's QueryStats map using structpb.NewStruct. The feedback suggests logging a warning when the conversion to structpb.Struct fails, rather than silently ignoring the error, to ensure visibility into potential data issues.
| if s, err := structpb.NewStruct(it.QueryStats); err == nil { | ||
| stats.QueryStats = s | ||
| } |
There was a problem hiding this comment.
The error from structpb.NewStruct is silently ignored. This could hide issues with query stats conversion, leading to incomplete statistics without any indication of a problem. It would be better to log this error for debugging purposes.
| if s, err := structpb.NewStruct(it.QueryStats); err == nil { | |
| stats.QueryStats = s | |
| } | |
| if s, err := structpb.NewStruct(it.QueryStats); err != nil { | |
| slog.Warn("Failed to convert QueryStats to structpb.Struct", "error", err) | |
| } else { | |
| stats.QueryStats = s | |
| } |
apstndb
changed the title
Log a slog warning if structpb.NewStruct cannot represent the map from RowIterator.QueryStats, per review feedback.
|
Addressed @gemini-code-assist review: on |
|
Thank you for the update, @apstndb. Using |
3 participants
Summary
Populate
ResultSetStats.QueryStatsincreateResultSetStats(transaction.go) fromspanner.RowIterator.QueryStats, which is already filled from the lastPartialResultSetin the stream. This letsdatabase/sqlcallers surface execution stats (e.g.elapsed_time) like the native client.Issue
Fixes #779