gcvctor: StringBasedValueOf and spanenc nullable constructors (#232, #207)#234
Conversation
Add StringBasedValueOf as the string-payload parallel to BytesBasedValueOf, unblocking PG-dialect wire-string GCV assembly for spanenc. Extend the FromNullable family with NUMERIC, JSON, INTERVAL, PGNumeric, and PGJsonB wrappers plus IntervalFromPtr; document CommitTimestamp boundary on timestamp helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds several new helper functions to construct spanner.GenericColumnValue from nullable types and pointers (such as IntervalFromPtr, NumericFromNullable, JSONFromNullable, IntervalFromNullable, PGNumericFromNullable, and PGJSONBFromNullable), along with a new StringBasedValueOf constructor. Feedback on the changes highlights a critical issue in PGJSONBFromNullable where passing spanner.PGJsonB.Value (which is already a JSON string) to PGJSONBValue results in double-encoding. Additionally, passing a map to spanner.PGJsonB in the tests will cause a compilation error. It is recommended to use StringBasedValueOf directly and simplify the function signature by removing the error return.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
apstndb
left a comment
apstndb
left a comment
There was a problem hiding this comment.
Reviewed from the spanenc consumer side (the encodeValue mirror this unblocks), with semantics cross-checked against cloud.google.com/go/spanner v1.91.0:
PGNumericFromNullable(raw wire string when Valid, typed NULL otherwise) andJSONFromNullable(gcvctor's unescaped wire form — spanenc's documented divergence) match exactly what spanenc hand-rolls today. After this lands, spanenc can deleteencodePGNumeric,encodePGJsonB,encodeNullJSON, and its inline INTERVAL nullable handling;encodeNullNumericreduces to its per-call precision validation +NumericFromNullable.- The CommitTimestamp boundary notes on the Timestamp helpers are exactly the requested wording. Thanks!
Three non-blocking inline notes below.
When PGJsonB.Value is already a JSON string, store it via StringBasedValueOf instead of re-marshaling through PGJSONBValue. Non-string Value types still marshal like PGJSONBValue. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds several new helper functions to construct spanner.GenericColumnValue from pointers and Spanner null wrappers (such as IntervalFromPtr, NumericFromNullable, JSONFromNullable, IntervalFromNullable, PGNumericFromNullable, and PGJSONBFromNullable). It also introduces StringBasedValueOf to handle string-compatible types with wire strings stored as-is, and refactors existing functions to use it. The review feedback highlights an issue in JSONFromNullable where raw JSON strings could be double-marshaled, and suggests handling string values directly and adding corresponding test coverage.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
When NullJSON.Value is already a JSON string, store it via StringBasedValueFromCode instead of re-marshaling through JSONValue. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several new helper functions in the gcvctor package to handle nullable inputs for Spanner types, including IntervalFromPtr, NumericFromNullable, JSONFromNullable, IntervalFromNullable, PGNumericFromNullable, and PGJSONBFromNullable. It also adds StringBasedValueOf to construct a GenericColumnValue with an arbitrary sppb.Type and wire string, refactoring existing functions to utilize it. Corresponding tests and documentation updates have been added. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Summary
StringBasedValueOf(typ, string)as the string-payload parallel toBytesBasedValueOf, withStringBasedValueFromCodedelegating to it (gcvctor: StringBasedValueOf(typ, string) — string-payload constructor for arbitrary Types (parallel to BytesBasedValueOf) #232).FromNullable/FromPtrfamily for spanenc:NumericFromNullable,JSONFromNullable,IntervalFromNullable,IntervalFromPtr,PGNumericFromNullable,PGJSONBFromNullable(gcvctor: constructor coverage gaps (FromNullable NUMERIC/JSON/INTERVAL, Must* variants, PG_OID, UUID/JSON string inputs) #207 spanenc-critical subset).CommitTimestampboundary onTimestampFromPtr/TimestampFromNullableper spanenc feedback.Unblocks spanenc to delete hand-rolled
encodePGNumeric,encodePGJsonB,encodeNullNumeric,encodeNullJSON, and inline INTERVAL nullable checks.Test plan
make checkStringBasedValueOfand extended nullable helpersCloses #232
Partially addresses #207 (Must*, PGOID, UUID/JSON string validators deferred)
Made with Cursor